Bug 199915 - Sticky element inside another sticky element does not redraw properly on scroll
Summary: Sticky element inside another sticky element does not redraw properly on scroll
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: Safari 11
Hardware: Mac macOS 10.13
: P2 Normal
Assignee: Martin Robinson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-18 13:44 PDT by max
Modified: 2021-10-13 00:59 PDT (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description max 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>
```
Comment 1 max 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/
Comment 2 Radar WebKit Bug Importer 2019-07-21 16:37:54 PDT
<rdar://problem/53375284>
Comment 3 Simon Fraser (smfr) 2019-07-21 20:39:56 PDT
Does this reproduce in Safari Tech Preview?
Comment 4 max 2019-07-22 03:43:11 PDT
Created attachment 374593 [details]
screen recording (safari tech preview)
Comment 5 max 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.
Comment 6 Simon Fraser (smfr) 2019-07-22 11:54:23 PDT
Created attachment 374618 [details]
Testcase
Comment 7 Martin Robinson 2021-10-11 08:07:23 PDT
Created attachment 440794 [details]
Patch
Comment 8 Simon Fraser (smfr) 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.
Comment 9 Martin Robinson 2021-10-12 05:43:50 PDT
Created attachment 440921 [details]
Patch
Comment 10 Martin Robinson 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.
Comment 11 Martin Robinson 2021-10-12 07:46:24 PDT
Created attachment 440930 [details]
Patch
Comment 12 EWS 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].