Fix anchor-positioned scroll buttons after dialog resize [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka voted and added 1 comment![]( "Open in Gerrit")
Votes added by Helmut Januschka
| Commit-Queue | +1 |
1 comment
@jar...@chromium.org thanks in advance for your time, feel free to re-route in case you dont find time, no time pressure from me.
i hope i found the root-area to fix it, please let me know if you want me to address anything.
a sampler page is here: https://static.januschka.com/i-419755710/index.html
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 added 1 comment![]( "Open in Gerrit")
i'm not very familiar with this code, i think rune or daniil would know more
Related details
- Daniil Sakhapov
- Helmut Januschka
- 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 3 comments![]( "Open in Gerrit")
// ::scroll-button() layout tree position is determined by
// LayoutTreeBuilderTraversal relative to the originating element and
// sibling pseudos. Reattach so the traversal logic chooses the correct
// parent and sibling rather than reinserting next to the old parent.I don't understand. Why doesn't ParentLayoutObject() and NextSiblingLayoutObject() below find the correct boxes for re-insertion?
// between that marker group and their originating element:This does not seem to match reality - that scroll buttons change whether they appear before or after the originating element in the box tree depending on the `scroll-marker-group` property?
Could you elaborate on the motivation for the traversal change?
<!DOCTYPE html>Can't this test be simplified to a case where you only change the property that causes NeedsReinsertLayoutTree() to return true?
Do you need media queries at all?
Is anchor positioning necessary to show the issue?
Related details
- Daniil Sakhapov
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
thanks for review, please let me know, not sure if we should remove the unittests, is the WPT enough?
// ::scroll-button() layout tree position is determined by
// LayoutTreeBuilderTraversal relative to the originating element and
// sibling pseudos. Reattach so the traversal logic chooses the correct
// parent and sibling rather than reinserting next to the old parent.I don't understand. Why doesn't ParentLayoutObject() and NextSiblingLayoutObject() below find the correct boxes for re-insertion?
tried it and removed the special-case and now rely on the normal `ParentLayoutObject()` / `NextSiblingLayoutObject()` reinsertion path.
// between that marker group and their originating element:This does not seem to match reality - that scroll buttons change whether they appear before or after the originating element in the box tree depending on the `scroll-marker-group` property?
Could you elaborate on the motivation for the traversal change?
PS5 changed the code.
Can't this test be simplified to a case where you only change the property that causes NeedsReinsertLayoutTree() to return true?
Do you need media queries at all?
Is anchor positioning necessary to show the issue?
i thought i'd need the media queries, as it was in the original reproducer, removed it, and simplified it, still verifies the fix :)
Related details
- Daniil Sakhapov
- 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 2 comments![]( "Open in Gerrit")
return LayoutTreeBuilderTraversal::NextSibling(*originating_element);This obviously makes this function inconsistent in box ordering. Could you please explain what you're trying to do here instead of sending arbitrary code that seemingly fixes the bug?
<!DOCTYPE html>Helmut JanuschkaCan't this test be simplified to a case where you only change the property that causes NeedsReinsertLayoutTree() to return true?
Do you need media queries at all?
Is anchor positioning necessary to show the issue?
i thought i'd need the media queries, as it was in the original reproducer, removed it, and simplified it, still verifies the fix :)
It's still way too complicated for testing what you're trying to fix. First, understand what the issue is, then write a test that's checking that specifically.
Related details
- Daniil Sakhapov
- Helmut Januschka
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. |