| Summary: | Sticky element inside another sticky element does not redraw properly on scroll | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | max | ||||||||||||||
| Component: | Scrolling | Assignee: | Martin Robinson <mrobinson> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | cmarcelo, ews-watchlist, fred.wang, jamesr, jtran040, luiz, max, mrobinson, simon.fraser, tonikitoo, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | Safari 11 | ||||||||||||||||
| Hardware: | Mac | ||||||||||||||||
| OS: | macOS 10.13 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
By the way, this case works right in iframe. You can check it out on jsfiddle: https://jsfiddle.net/gthjnvbk/ Does this reproduce in Safari Tech Preview? Created attachment 374593 [details]
screen recording (safari tech preview)
(In reply to max from comment #4) > Created attachment 374593 [details] > screen recording (safari tech preview) (In reply to Simon Fraser (smfr) from comment #3) > Does this reproduce in Safari Tech Preview? Yes, i have added screen recording. Looks the same. Created attachment 374618 [details]
Testcase
Created attachment 440794 [details]
Patch
Comment on attachment 440794 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=440794&action=review > Source/WebCore/page/scrolling/ScrollingStateStickyNode.h:61 > + FloatSize m_stickyOffset; It's confusing that you store some state here on the State node, but this is not copied to the Scrolling node on commit; it looks like if feeds into the layer position. We generally don't do this; if the state node needs this m_stickyOffset, can it be pushed onto the node from layout? > Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.h:60 > + FloatSize m_stickyOffset; Maybe we should have a shared ScrollingTreeStickyNode base class that Cocoa and Nicosia can both use. Created attachment 440921 [details]
Patch
(In reply to Simon Fraser (smfr) from comment #8) > Comment on attachment 440794 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=440794&action=review > > > Source/WebCore/page/scrolling/ScrollingStateStickyNode.h:61 > > + FloatSize m_stickyOffset; > > It's confusing that you store some state here on the State node, but this is > not copied to the Scrolling node on commit; it looks like if feeds into the > layer position. We generally don't do this; if the state node needs this > m_stickyOffset, can it be pushed onto the node from layout? > This is a good point. I don't think it makes sense to push any state back to layout, since it already handles this situation correctly. Instead, I've rewritten the patch to not store any state and to always just calculate the correct sticky offset based on the current delta from layout of the sticky ancestors. > > Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNode.h:60 > > + FloatSize m_stickyOffset; > > Maybe we should have a shared ScrollingTreeStickyNode base class that Cocoa > and Nicosia can both use. Great idea. I've opened https://bugs.webkit.org/show_bug.cgi?id=231571 for this and can tackle it next. The same could probably be done for fixed nodes as well. Created attachment 440930 [details]
Patch
Committed r284084 (242912@main): <https://commits.webkit.org/242912@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 440930 [details]. *** Bug 231493 has been marked as a duplicate of this bug. *** |
Created attachment 374410 [details] screen recording Looks like webkit calculates layout, but do not repaint sticky element on scroll. HTML to reproduce: ``` <html> <head> <meta charset="utf8" /> <style> .sticky { position: -webkit-sticky; position: sticky; top: 0; } .parent { background: gray; height: 150vh; } .child { background: white; } </style> </head> <body> <div class="sticky parent"> <div class="sticky child"><a href="#">sticky</a></div> </div> </body> </html> ```