Bug 199915

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:

Keywords:

InRadar

Version:

Safari 11

Hardware:

Mac

OS:

macOS 10.13

Attachments:

Description	Flags
screen recording	none
screen recording (safari tech preview)	none
Testcase	none
Patch	none
Patch	none
Patch	none

max

Reported 2019-07-18 13:44:22 PDT

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> ```

Attachments
screen recording (1.67 MB, video/quicktime) 2019-07-18 13:44 PDT, max	no flags	Details
screen recording (safari tech preview) (1.85 MB, video/quicktime) 2019-07-22 03:43 PDT, max	no flags	Details
Testcase (447 bytes, text/html) 2019-07-22 11:54 PDT, Simon Fraser (smfr)	no flags	Details
Patch (15.16 KB, patch) 2021-10-11 08:07 PDT, Martin Robinson	no flags	Details Formatted Diff Diff
Patch (13.52 KB, patch) 2021-10-12 05:43 PDT, Martin Robinson	no flags	Details Formatted Diff Diff
Patch (14.86 KB, patch) 2021-10-12 07:46 PDT, Martin Robinson	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

max

Comment 1 2019-07-19 01:45:12 PDT

By the way, this case works right in iframe. You can check it out on jsfiddle: https://jsfiddle.net/gthjnvbk/

Radar WebKit Bug Importer

Comment 2 2019-07-21 16:37:54 PDT

<rdar://problem/53375284>

Simon Fraser (smfr)

Comment 3 2019-07-21 20:39:56 PDT

Does this reproduce in Safari Tech Preview?

max

Comment 4 2019-07-22 03:43:11 PDT

Created attachment 374593 [details] screen recording (safari tech preview)

max

Comment 5 2019-07-22 03:44:41 PDT

(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.

Simon Fraser (smfr)

Comment 6 2019-07-22 11:54:23 PDT

Created attachment 374618 [details] Testcase

Martin Robinson

Comment 7 2021-10-11 08:07:23 PDT

Created attachment 440794 [details] Patch

Simon Fraser (smfr)

Comment 8 2021-10-11 09:59:54 PDT

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.

Martin Robinson

Comment 9 2021-10-12 05:43:50 PDT

Created attachment 440921 [details] Patch

Martin Robinson

Comment 10 2021-10-12 05:48:48 PDT

(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.

Martin Robinson

Comment 11 2021-10-12 07:46:24 PDT

Created attachment 440930 [details] Patch

EWS

Comment 12 2021-10-13 00:59:21 PDT

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].

Martin Robinson

Comment 13 2021-11-09 03:52:39 PST

*** Bug 231493 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.