Bug 195584 – [iOS WK2] REGRESSION (r242687): Programmatic scroll of overflow scroll results in bad rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Simon Fraser (smfr)]

Programmatic scroll of overflow:scroll does the wrong thing (it clobbers the UIScrollView contentOffset with 0,0).

[Simon Fraser (smfr)]

Marked compositing/ios/overflow-scroll-update-overlap.html as failing for now in https://trac.webkit.org/changeset/242758/webkit

[Simon Fraser (smfr)]

We need the same logic we have for "requestedScrollPositionUpdate" for overflow that we have for Frames.

[Simon Fraser (smfr)]

Created attachment 364342 [details]
Work in progress

[Simon Fraser (smfr)]

Need to do some cleanup work before this to move isHandlingProgrammaticScroll from ScrollingTree to the scrolling node, and make frames/overflow more similar for programmatic scroll.

[Radar WebKit Bug Importer]

<rdar://problem/48795561>

[Darryl Pogue]

Might the fix for this also improve the situation for bug 192564 where the UIScrollView contentOffset changing on keyboard closing doesn't result in re-rendering of layers?

[Simon Fraser (smfr)]

I don't think so; this is a regression from recent work.

[Simon Fraser (smfr)]

Created attachment 366779 [details]
Patch

[alan]

Comment on attachment 366779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366779&action=review

> Source/WebCore/rendering/RenderLayer.cpp:2817
> +        return 0;

0 is a special value?

[Simon Fraser (smfr)]

(In reply to zalan from comment #9)
> Comment on attachment 366779 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366779&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:2817
> > +        return 0;
> 
> 0 is a special value?

Yes, a valid ScrollingNodeID is never 0.

[alan]

(In reply to Simon Fraser (smfr) from comment #10)
> (In reply to zalan from comment #9)
> > Comment on attachment 366779 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=366779&action=review
> > 
> > > Source/WebCore/rendering/RenderLayer.cpp:2817
> > > +        return 0;
> > 
> > 0 is a special value?
> 
> Yes, a valid ScrollingNodeID is never 0.
optionals don't work?

[EWS Watchlist]

Comment on attachment 366779 [details]
Patch

Attachment 366779 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11772321

New failing tests:
fast/html/marquee-scrollamount.html

[EWS Watchlist]

Created attachment 366786 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 366779 [details]
Patch

Attachment 366779 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11772408

New failing tests:
fast/html/marquee-scrollamount.html

[EWS Watchlist]

Created attachment 366790 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/243926/webkit

[Antti Koivisto]

Comment on attachment 366779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366779&action=review

> Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:163
> +    scrollingTree().setIsHandlingProgrammaticScroll(scrollType == ScrollType::Programmatic);

Using a tree-global bit as a side channel is bug-prone and confusing. We should probably have some sort of context object that we pass around.

[Simon Fraser (smfr)]

(In reply to Antti Koivisto from comment #17)
> Comment on attachment 366779 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366779&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp:163
> > +    scrollingTree().setIsHandlingProgrammaticScroll(scrollType == ScrollType::Programmatic);
> 
> Using a tree-global bit as a side channel is bug-prone and confusing. We
> should probably have some sort of context object that we pass around.

Agreed that might be nicer, and probably not too bad in the scrolling tree.