Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-position] Added 4 new sticky tests #32612

Merged
merged 6 commits into from
Jun 22, 2022

Conversation

@TalbotG TalbotG requested review from frivoal and fantasai January 31, 2022 00:30
@TalbotG TalbotG self-assigned this Jan 31, 2022
@wpt-pr-bot wpt-pr-bot requested a review from astearns January 31, 2022 00:30
@fantasai
Copy link
Contributor

fantasai commented Mar 7, 2022

  • position-sticky-stacking-context-002.html - I think this is a good test, but it would be better if we had another div to check the overlap on top. One way to do this would be to make the sticky item half green and half red, and to make the third div a half-sized div that overlaps the red half.

  • position-sticky-scrolled-remove-horiz-sibling.html - Instead of adding a new file, please incorporate this test into the original file.

  • position-sticky-fixed-ancestor-002/3.html - I believe these tests are correct in that they should pass as described... but I don't understand what the assert is trying to say. Also, are there equivalent tests for other values of 'position', given they should all behave the same here?

Copy link
Contributor

@fantasai fantasai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(See above)

@TalbotG
Copy link
Contributor Author

TalbotG commented Mar 7, 2022

  • position-sticky-stacking-context-002.html - I think this is a good test, but it would be better if we had another div to check the overlap on top. One way to do this would be to make the sticky item half green and half red, and to make the third div a half-sized div that overlaps the red half.

Understood. On my to-do-list now.

  • position-sticky-scrolled-remove-horiz-sibling.html - Instead
    of adding a new file, please incorporate this test into the original
    file.

Understood. On my to-do-list now.

  • position-sticky-fixed-ancestor-002/3.html - I believe
    these tests are correct in that they should pass as described...
    but I don't understand what the assert is trying to say. Also,
    are there equivalent tests for other values of 'position', given
    they should all behave the same here?

position-sticky-fixed-ancestor-002.html (3 selectors and 10 declarations) is a simplified version of
http://wpt.live/css/css-position/sticky/position-sticky-fixed-ancestor.html (8 selectors and 26 declarations)

position-sticky-fixed-ancestor-003.html (3 selectors and 10 declarations) is a variation of
http://wpt.live/css/css-position/sticky/position-sticky-fixed-ancestor.html

I will improve the assert text, otherwise remove the current one. Right now, removal of current assert text is my preference.

Right now, there are no additional tests for other values of 'position' for (or with) this kind of code scenario.

@TalbotG
Copy link
Contributor Author

TalbotG commented Apr 3, 2022

Both
position-sticky-scrolled-remove-horiz-sibling.html
and
position-sticky-scrolled-remove-sibling.html
have been merged into 1 single test. At my website:

http://www.gtalbot.org/BrowserBugsSection/CSS3PositionedLayout/sticky/position-sticky-scrolled-remove-sibling-new.html

I have no idea why the nested
requestAnimationFrame(()=>{...}
are important or required and what
they are supposed to do. I will add them back
if requested. Adding @mstensho as he
would certainly know.

@TalbotG
Copy link
Contributor Author

TalbotG commented Apr 3, 2022

At my website, the improved
position-sticky-stacking-context-002
test is:

http://www.gtalbot.org/BrowserBugsSection/CSS3PositionedLayout/sticky/position-sticky-stacking-context-002-new.html

@TalbotG TalbotG requested a review from mstensho April 3, 2022 20:38
@mstensho
Copy link
Contributor

mstensho commented Apr 4, 2022

I have no idea why the nested requestAnimationFrame(()=>{...} are important or required and what they are supposed to do. I will add them back if requested. Adding @mstensho as he would certainly know.

I don't really know (I've just accepted to (almost) always use double RAF in tests that require the page to be painted before making dynamic changes). Maybe see https://bugs.chromium.org/p/chromium/issues/detail?id=675795 ?

@TalbotG
Copy link
Contributor Author

TalbotG commented May 7, 2022

@fantasai fantasai merged commit 72d95bb into web-platform-tests:master Jun 22, 2022
@TalbotG TalbotG deleted the CSS3Position-GT-PR1 branch June 22, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants