Account for popover/dialog styles without overlay property [chromium/src : main]
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
Rune LillesveenHey yall, I realized that there are a bunch of tests unexpectedly failing after I made this patch to remove the overlay property: https://chromium-review.googlesource.com/c/chromium/src/+/7133219
You can see what the failures look like: https://chromium-layout-test-archives.storage.googleapis.com/results.html?json=chromium/try/linux-rel/2493697/headless_shell_wpt_tests%20%28with%20patch%29/full_results_jsonp.js
It looks like some tests which are supposed to have scroll markers don't have them anymore, and some tests which weren't supposed to have a scrollbar now have a scrollbar. Did I miss something in my patch to put overlay behind a flag?
Daniil SakhapovMy guess is that it's a side-effect of this one now being IsEmpty() more often now:
Adding sakhapov@ for what the proper condition for creating a ::scroll-marker is.
Joey ArharThis is probably the same reason as - https://issues.chromium.org/issues/464010034, will fix.
Rune LillesveenThanks for the patch! It looks like there are still some more tests failing
Joey ArharThanks for the patch! It looks like there are still some more tests failing
That's probably expected. There's a second CL needed which hasn't landed yet.
Daniil SakhapovThanks for the second patch!
I still see 29 tests failing, including one scroll marker test: https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2510728/test-results
Joey ArharThe last scroll marker test is on my list.
The other tests are unknown to me, but given the (probably) common root issue I'll try to fix them as well.
Stay tuned for the next CLs! 😊
Joey ArharThank you Daniil!
Joey ArharFrom offline discussion, it looks like the remaining failures like missing scrollbars are due to other issues which are different from the scroll marker ones.
I tried to debug one of the test failures, menu-elements-default-style.html, which looks like the anchor positioned element is positioned in the top left corner instead of under its anchor. However, when I open the test in chrome or content_shell, the top layer element is positioned correctly - the issue only happens when running it as a test, and it didn't help to add a double rAF before taking the screenshot.
I'm not sure what's wrong with this or all the other failures, so now I'm looking at other ways to get the performance gain: https://chromium-review.googlesource.com/c/chromium/src/+/7277167
Joey ArharNevermind, I'm starting to make more progress :)
I've fixed a bunch of them with the latest patch set, but I'll have another patch to continue working on the remaining ones
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
bool SupportsTopLayerStyle() const override;What does SupportsTopLayerStyle() mean? Documentation?
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
bool SupportsTopLayerStyle() const override;What does SupportsTopLayerStyle() mean? Documentation?
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
bool SupportsTopLayerStyle() const override;Joey ArharWhat does SupportsTopLayerStyle() mean? Documentation?
I added a comment to point to the comment in element.h
I don't think that name matches what it is.
Isn't this close to what ComputedStyle::IsRenderedInTopLayer() is when when the overlay property is supported? Perhaps a similar to that would be better?
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar added 1 comment![]( "Open in Gerrit")
bool SupportsTopLayerStyle() const override;Joey ArharWhat does SupportsTopLayerStyle() mean? Documentation?
Rune LillesveenI added a comment to point to the comment in element.h
I don't think that name matches what it is.
Isn't this close to what ComputedStyle::IsRenderedInTopLayer() is when when the overlay property is supported? Perhaps a similar to that would be better?
Thanks, I renamed it to IsRenderedInTopLayer. My next best idea is MatchesOverlayAutoStyles. What do you think?
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen voted Code-Review+1![]( "Open in Gerrit")
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joey Arhar
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
bool SupportsTopLayerStyle() const override;Joey ArharWhat does SupportsTopLayerStyle() mean? Documentation?
Rune LillesveenI added a comment to point to the comment in element.h
Joey ArharI don't think that name matches what it is.
Isn't this close to what ComputedStyle::IsRenderedInTopLayer() is when when the overlay property is supported? Perhaps a similar to that would be better?
Thanks, I renamed it to IsRenderedInTopLayer. My next best idea is MatchesOverlayAutoStyles. What do you think?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+2![]( "Open in Gerrit")
- Joey Arhar
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 1 comment![]( "Open in Gerrit")
Related details
- Joey Arhar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joey Arhar (Gerrit)
Joey Arhar voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Account for popover/dialog styles without overlay property
The original patch to put overlay behind a flag replaced checks for
overlay:auto with the element top layer flag, which didn't match the
overlay:auto logic in the UA stylesheet. This patch replicates the logic
in the UA stylesheet as a replacement for overlay:auto checks.
There are still more failing tests when the overlay flag is disabled, I
will continue to work on them in subsequent patches.
- M third_party/blink/renderer/core/css/resolver/style_adjuster.cc
- M third_party/blink/renderer/core/dom/element.h
- M third_party/blink/renderer/core/html/html_element.cc
- M third_party/blink/renderer/core/html/html_element.h
- M third_party/blink/renderer/core/html/resources/html.css
- M third_party/blink/renderer/core/style/computed_style.cc
- M third_party/blink/web_tests/VirtualTestSuites
Code-Review: +1 by Philip Rogers, +1 by Rune Lillesveen, +1 by Joey Arhar
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |