Bug 195584 - [iOS WK2] REGRESSION (r242687): Programmatic scroll of overflow scroll results in bad rendering
Summary: [iOS WK2] REGRESSION (r242687): Programmatic scroll of overflow scroll result...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on: 196424
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-11 17:06 PDT by Simon Fraser (smfr)
Modified: 2019-04-04 23:16 PDT (History)
10 users (show)

See Also:


Attachments
Work in progress (22.20 KB, patch)
2019-03-11 20:22 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (32.54 KB, patch)
2019-04-04 17:20 PDT, Simon Fraser (smfr)
zalan: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.70 MB, application/zip)
2019-04-04 18:40 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.56 MB, application/zip)
2019-04-04 19:17 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-03-11 17:06:54 PDT
Programmatic scroll of overflow:scroll does the wrong thing (it clobbers the UIScrollView contentOffset with 0,0).
Comment 1 Simon Fraser (smfr) 2019-03-11 17:12:50 PDT
Marked compositing/ios/overflow-scroll-update-overlap.html as failing for now in https://trac.webkit.org/changeset/242758/webkit
Comment 2 Simon Fraser (smfr) 2019-03-11 19:38:34 PDT
We need the same logic we have for "requestedScrollPositionUpdate" for overflow that we have for Frames.
Comment 3 Simon Fraser (smfr) 2019-03-11 20:22:57 PDT
Created attachment 364342 [details]
Work in progress
Comment 4 Simon Fraser (smfr) 2019-03-11 20:24:22 PDT
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.
Comment 5 Radar WebKit Bug Importer 2019-03-11 20:25:16 PDT
<rdar://problem/48795561>
Comment 6 Darryl Pogue 2019-04-01 12:21:02 PDT
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?
Comment 7 Simon Fraser (smfr) 2019-04-01 12:29:55 PDT
I don't think so; this is a regression from recent work.
Comment 8 Simon Fraser (smfr) 2019-04-04 17:20:39 PDT
Created attachment 366779 [details]
Patch
Comment 9 zalan 2019-04-04 17:24:23 PDT
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?
Comment 10 Simon Fraser (smfr) 2019-04-04 17:30:58 PDT
(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.
Comment 11 zalan 2019-04-04 17:34:29 PDT
(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?
Comment 12 EWS Watchlist 2019-04-04 18:40:07 PDT
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
Comment 13 EWS Watchlist 2019-04-04 18:40:09 PDT
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
Comment 14 EWS Watchlist 2019-04-04 19:17:18 PDT
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
Comment 15 EWS Watchlist 2019-04-04 19:17:20 PDT
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
Comment 16 Simon Fraser (smfr) 2019-04-04 22:19:08 PDT
https://trac.webkit.org/changeset/243926/webkit
Comment 17 Antti Koivisto 2019-04-04 22:20:17 PDT
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.
Comment 18 Simon Fraser (smfr) 2019-04-04 23:16:51 PDT
(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.