Changeset 198455 in webkit


Ignore:
Timestamp:
Mar 18, 2016, 5:00:46 PM (9 years ago)
Author:
Simon Fraser
Message:

Sideways-scrollable RTL document has wrong initial and reload offset in WKWebView
​https://bugs.webkit.org/show_bug.cgi?id=155660
Source/WebCore:

rdar://problem/22212662

Reviewed by Tim Horton.

There were two problems with the scroll position of RTL documents on initial and reload
in WKWebView.

First, in the delegatesScrolling() code path, ScrollView::updateScrollbars() needs to
tell someone that the scroll origin changed, to trigger a scroll to the page origin.

Secondly, WKWebView had scrollPosition/scrollOffset confusion in various places.

Test: fast/scrolling/rtl-initial-scroll-position.html

  • platform/ScrollView.cpp:

(WebCore::ScrollView::updateScrollbars):

Source/WebKit2:

Reviewed by Tim Horton.

There were two problems with the scroll position of RTL documents on initial and reload
in WKWebView.

First, in the delegatesScrolling() code path, ScrollView::updateScrollbars() needs to
tell someone that the scroll origin changed, to trigger a scroll to the page origin.

Secondly, WKWebView had scrollPosition/scrollOffset confusion in various places. In
the restorePageState() code path, WebCore passes an exposedRect; this patch makes
it explicit that it's an exposedContentRect, and passes scrollOrigin so that it can
be mapped into scrollOffset-relative coordinates that the UIScrollView wants.

When reloading an RTL page, there was an additional issue; restorePageState()
restored the exposedRect, but then we'd see the origin change as a programmatic scroll
to 0,0, clobbering the exposedRect. Fix by using a "_commitDidRestoreExposedRect" flag
on the WKWebView that is used to ignore the programmatic scroll in that case.

Ideally these changes would fix fast/scrolling/scroll-position-on-reload-rtl.html, but the
test still fails because of timing differences between OS X and iOS.

  • UIProcess/API/Cocoa/WKWebView.mm:

(-[WKWebView _didCommitLayerTree:]):
(-[WKWebView _layerTreeCommitComplete]):
(-[WKWebView _restorePageStateToExposedRect:scrollOrigin:scale:]):
(-[WKWebView _scrollToContentScrollPosition:scrollOrigin:]):
(-[WKWebView _restorePageStateToExposedRect:scale:]): Deleted.
(-[WKWebView _scrollToContentOffset:scrollOrigin:]): Deleted.

  • UIProcess/API/Cocoa/WKWebViewInternal.h:
  • UIProcess/PageClient.h:
  • UIProcess/WebPageProxy.h:
  • UIProcess/WebPageProxy.messages.in:
  • UIProcess/ios/PageClientImplIOS.h:
  • UIProcess/ios/PageClientImplIOS.mm:

(WebKit::PageClientImpl::requestScroll):
(WebKit::PageClientImpl::layerTreeCommitComplete):
(WebKit::PageClientImpl::restorePageState):

  • UIProcess/ios/WKContentView.h:
  • UIProcess/ios/WKContentView.mm:

(-[WKContentView _layerTreeCommitComplete]):

  • UIProcess/ios/WebPageProxyIOS.mm:

(WebKit::WebPageProxy::layerTreeCommitComplete):
(WebKit::WebPageProxy::restorePageState):

  • UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:

(WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):

  • WebProcess/WebPage/ios/WebPageIOS.mm:

(WebKit::WebPage::restorePageState):

LayoutTests:

Reviewed by Tim Horton.

Test for initial scroll position in an RTL page.

  • fast/scrolling/rtl-initial-scroll-position-expected.html: Added.
  • fast/scrolling/rtl-initial-scroll-position.html: Added.
Location:
trunk
Files:
2 added
16 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r198451 r198455  
     12016-03-18  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Sideways-scrollable RTL document has wrong initial and reload offset in WKWebView
     4        https://bugs.webkit.org/show_bug.cgi?id=155660
     5
     6        Reviewed by Tim Horton.
     7       
     8        Test for initial scroll position in an RTL page.
     9
     10        * fast/scrolling/rtl-initial-scroll-position-expected.html: Added.
     11        * fast/scrolling/rtl-initial-scroll-position.html: Added.
     12
    1132016-03-18  Darin Adler  <darin@apple.com>
    214

  • trunk/Source/WebCore/ChangeLog

    r198454 r198455  
     12016-03-18  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Sideways-scrollable RTL document has wrong initial and reload offset in WKWebView
     4        https://bugs.webkit.org/show_bug.cgi?id=155660
     5        rdar://problem/22212662
     6
     7        Reviewed by Tim Horton.
     8       
     9        There were two problems with the scroll position of RTL documents on initial and reload
     10        in WKWebView.
     11
     12        First, in the delegatesScrolling() code path, ScrollView::updateScrollbars() needs to
     13        tell someone that the scroll origin changed, to trigger a scroll to the page origin.
     14
     15        Secondly, WKWebView had scrollPosition/scrollOffset confusion in various places.
     16
     17        Test: fast/scrolling/rtl-initial-scroll-position.html
     18
     19        * platform/ScrollView.cpp:
     20        (WebCore::ScrollView::updateScrollbars):
     21
    1222016-03-18  Ryan Haddad  <ryanhaddad@apple.com>
    223

  • trunk/Source/WebCore/platform/ScrollView.cpp

    r198369 r198455  
    583583    LOG_WITH_STREAM(Scrolling, stream << "ScrollView::updateScrollbars " << desiredPosition);
    584584
    585     if (m_inUpdateScrollbars || prohibitsScrolling() || platformWidget() || delegatesScrolling())
    586         return;
     585    if (m_inUpdateScrollbars || prohibitsScrolling() || platformWidget())
     586        return;
     587   
     588    if (delegatesScrolling()) {
     589        if (scrollOriginChanged()) {
     590            ScrollableArea::scrollToOffsetWithoutAnimation(scrollOffsetFromPosition(desiredPosition));
     591            resetScrollOriginChanged();
     592        }
     593        return;
     594    }
    587595
    588596    bool hasOverlayScrollbars = (!m_horizontalScrollbar || m_horizontalScrollbar->isOverlayScrollbar()) && (!m_verticalScrollbar || m_verticalScrollbar->isOverlayScrollbar());

  • trunk/Source/WebKit2/ChangeLog

    r198453 r198455  
     12016-03-18  Simon Fraser  <simon.fraser@apple.com>
     2
     3        Sideways-scrollable RTL document has wrong initial and reload offset in WKWebView
     4        https://bugs.webkit.org/show_bug.cgi?id=155660
     5
     6        Reviewed by Tim Horton.
     7
     8        There were two problems with the scroll position of RTL documents on initial and reload
     9        in WKWebView.
     10
     11        First, in the delegatesScrolling() code path, ScrollView::updateScrollbars() needs to
     12        tell someone that the scroll origin changed, to trigger a scroll to the page origin.
     13
     14        Secondly, WKWebView had scrollPosition/scrollOffset confusion in various places. In
     15        the restorePageState() code path, WebCore passes an exposedRect; this patch makes
     16        it explicit that it's an exposedContentRect, and passes scrollOrigin so that it can
     17        be mapped into scrollOffset-relative coordinates that the UIScrollView wants.
     18
     19        When reloading an RTL page, there was an additional issue; restorePageState()
     20        restored the exposedRect, but then we'd see the origin change as a programmatic scroll
     21        to 0,0, clobbering the exposedRect. Fix by using a "_commitDidRestoreExposedRect" flag
     22        on the WKWebView that is used to ignore the programmatic scroll in that case.
     23
     24        Ideally these changes would fix fast/scrolling/scroll-position-on-reload-rtl.html, but the
     25        test still fails because of timing differences between OS X and iOS.
     26
     27        * UIProcess/API/Cocoa/WKWebView.mm:
     28        (-[WKWebView _didCommitLayerTree:]):
     29        (-[WKWebView _layerTreeCommitComplete]):
     30        (-[WKWebView _restorePageStateToExposedRect:scrollOrigin:scale:]):
     31        (-[WKWebView _scrollToContentScrollPosition:scrollOrigin:]):
     32        (-[WKWebView _restorePageStateToExposedRect:scale:]): Deleted.
     33        (-[WKWebView _scrollToContentOffset:scrollOrigin:]): Deleted.
     34        * UIProcess/API/Cocoa/WKWebViewInternal.h:
     35        * UIProcess/PageClient.h:
     36        * UIProcess/WebPageProxy.h:
     37        * UIProcess/WebPageProxy.messages.in:
     38        * UIProcess/ios/PageClientImplIOS.h:
     39        * UIProcess/ios/PageClientImplIOS.mm:
     40        (WebKit::PageClientImpl::requestScroll):
     41        (WebKit::PageClientImpl::layerTreeCommitComplete):
     42        (WebKit::PageClientImpl::restorePageState):
     43        * UIProcess/ios/WKContentView.h:
     44        * UIProcess/ios/WKContentView.mm:
     45        (-[WKContentView _layerTreeCommitComplete]):
     46        * UIProcess/ios/WebPageProxyIOS.mm:
     47        (WebKit::WebPageProxy::layerTreeCommitComplete):
     48        (WebKit::WebPageProxy::restorePageState):
     49        * UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm:
     50        (WebKit::RemoteLayerTreeDrawingAreaProxy::commitLayerTree):
     51        * WebProcess/WebPage/ios/WebPageIOS.mm:
     52        (WebKit::WebPage::restorePageState):
     53
    1542016-03-18  Brady Eidson  <beidson@apple.com>
    255

  • trunk/Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm

    r198298 r198455  
    113113#import <WebCore/InspectorOverlay.h>
    114114#import <WebCore/QuartzCoreSPI.h>
     115#import <WebCore/ScrollableArea.h>
    115116
    116117#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000
    … …  
    241242
    242243    BOOL _needsToRestoreExposedRect;
     244    BOOL _commitDidRestoreExposedRect;
    243245    WebCore::FloatRect _exposedRectToRestore;
    244246    BOOL _needsToRestoreUnobscuredCenter;
    … …  
    12341236
    12351237            changeContentOffsetBoundedInValidRange(_scrollView.get(), exposedPosition);
     1238            _commitDidRestoreExposedRect = YES;
    12361239            if (_gestureController)
    12371240                _gestureController->didRestoreScrollPosition();
    … …  
    12601263    if (WebKit::RemoteLayerTreeScrollingPerformanceData* scrollPerfData = _page->scrollingPerformanceData())
    12611264        scrollPerfData->didCommitLayerTree([self visibleRectInViewCoordinates]);
     1265}
     1266
     1267- (void)_layerTreeCommitComplete
     1268{
     1269    _commitDidRestoreExposedRect = NO;
    12621270}
    12631271
    … …  
    12891297}
    12901298
    1291 - (void)_restorePageStateToExposedRect:(WebCore::FloatRect)exposedRect scale:(double)scale
     1299- (void)_restorePageStateToExposedRect:(WebCore::FloatRect)exposedRect scrollOrigin:(WebCore::IntPoint)scrollOrigin scale:(double)scale
    12921300{
    12931301    if (_dynamicViewportUpdateMode != DynamicViewportUpdateMode::NotResizing)
    … …  
    13001308    _needsToRestoreExposedRect = YES;
    13011309    _firstTransactionIDAfterPageRestore = downcast<WebKit::RemoteLayerTreeDrawingAreaProxy>(*_page->drawingArea()).nextLayerTreeTransactionID();
     1310   
     1311    // Move the exposed rect into scrollView coordinates.
     1312    exposedRect.move(toFloatSize(scrollOrigin));
    13021313    _exposedRectToRestore = exposedRect;
    13031314    _scaleToRestore = scale;
    … …  
    14091420}
    14101421
    1411 - (void)_scrollToContentOffset:(WebCore::FloatPoint)contentOffsetInPageCoordinates scrollOrigin:(WebCore::IntPoint)scrollOrigin
    1412 {
    1413     if (_dynamicViewportUpdateMode != DynamicViewportUpdateMode::NotResizing)
     1422- (void)_scrollToContentScrollPosition:(WebCore::FloatPoint)scrollPosition scrollOrigin:(WebCore::IntPoint)scrollOrigin
     1423{
     1424    if (_commitDidRestoreExposedRect || _dynamicViewportUpdateMode != DynamicViewportUpdateMode::NotResizing)
    14141425        return;
    14151426
    1416     WebCore::FloatPoint contentOffsetRespectingOrigin = scrollOrigin + toFloatSize(contentOffsetInPageCoordinates);
    1417 
    1418     WebCore::FloatPoint scaledOffset = contentOffsetRespectingOrigin;
     1427    WebCore::FloatPoint contentOffset = WebCore::ScrollableArea::scrollOffsetFromPosition(scrollPosition, toFloatSize(scrollOrigin));
     1428
     1429    WebCore::FloatPoint scaledOffset = contentOffset;
    14191430    CGFloat zoomScale = contentZoomScale(self);
    14201431    scaledOffset.scale(zoomScale, zoomScale);

  • trunk/Source/WebKit2/UIProcess/API/Cocoa/WKWebViewInternal.h

    r197986 r198455  
    7676- (void)_didCommitLoadForMainFrame;
    7777- (void)_didCommitLayerTree:(const WebKit::RemoteLayerTreeTransaction&)layerTreeTransaction;
     78- (void)_layerTreeCommitComplete;
    7879
    7980- (void)_dynamicViewportUpdateChangedTargetToScale:(double)newScale position:(CGPoint)newScrollPosition nextValidLayerTreeTransactionID:(uint64_t)nextValidLayerTreeTransactionID;
    8081- (void)_couldNotRestorePageState;
    81 - (void)_restorePageStateToExposedRect:(WebCore::FloatRect)exposedRect scale:(double)scale;
    82 - (void)_restorePageStateToUnobscuredCenter:(WebCore::FloatPoint)center scale:(double)scale;
     82- (void)_restorePageStateToExposedRect:(WebCore::FloatRect)exposedRect scrollOrigin:(WebCore::IntPoint)scrollOrigin scale:(double)scale;
     83- (void)_restorePageStateToUnobscuredCenter:(WebCore::FloatPoint)center scale:(double)scale; // FIXME: needs scroll origin?
    8384
    8485- (PassRefPtr<WebKit::ViewSnapshot>)_takeViewSnapshot;
    8586
    86 - (void)_scrollToContentOffset:(WebCore::FloatPoint)contentOffset scrollOrigin:(WebCore::IntPoint)scrollOrigin;
     87- (void)_scrollToContentScrollPosition:(WebCore::FloatPoint)scrollPosition scrollOrigin:(WebCore::IntPoint)scrollOrigin;
    8788- (BOOL)_scrollToRect:(WebCore::FloatRect)targetRect origin:(WebCore::FloatPoint)origin minimumScrollDistance:(float)minimumScrollDistance;
    8889- (void)_scrollByContentOffset:(WebCore::FloatPoint)offset;

  • trunk/Source/WebKit2/UIProcess/PageClient.h

    r197461 r198455  
    289289
    290290    virtual void didCommitLayerTree(const RemoteLayerTreeTransaction&) = 0;
     291    virtual void layerTreeCommitComplete() = 0;
     292
    291293    virtual void dynamicViewportUpdateChangedTarget(double newScale, const WebCore::FloatPoint& newScrollPosition, uint64_t transactionID) = 0;
    292294    virtual void couldNotRestorePageState() = 0;
    293     virtual void restorePageState(const WebCore::FloatRect&, double) = 0;
     295    virtual void restorePageState(const WebCore::FloatRect& exposedContentRect, const WebCore::IntPoint& scrollOrigin, double scale) = 0;
    294296    virtual void restorePageCenterAndScale(const WebCore::FloatPoint&, double) = 0;
    295297

  • trunk/Source/WebKit2/UIProcess/WebPageProxy.h

    r198306 r198455  
    527527#endif
    528528    void didCommitLayerTree(const WebKit::RemoteLayerTreeTransaction&);
     529    void layerTreeCommitComplete();
    529530
    530531#if USE(COORDINATED_GRAPHICS_MULTIPROCESS)
    … …  
    14071408    void dynamicViewportUpdateChangedTarget(double newTargetScale, const WebCore::FloatPoint& newScrollPosition, uint64_t dynamicViewportSizeUpdateID);
    14081409    void couldNotRestorePageState();
    1409     void restorePageState(const WebCore::FloatRect&, double scale);
     1410    void restorePageState(const WebCore::FloatRect& exposedContentRect, const WebCore::IntPoint& scrollOrigin, double scale);
    14101411    void restorePageCenterAndScale(const WebCore::FloatPoint&, double scale);
    14111412

  • trunk/Source/WebKit2/UIProcess/WebPageProxy.messages.in

    r198306 r198455  
    359359    DynamicViewportUpdateChangedTarget(double newTargetScale, WebCore::FloatPoint newScrollPosition, uint64_t dynamicViewportSizeUpdateID)
    360360    CouldNotRestorePageState()
    361     RestorePageState(WebCore::FloatRect exposedRect, double scale)
     361    RestorePageState(WebCore::FloatRect exposedContentRect, WebCore::IntPoint scrollOrigin, double scale)
    362362    RestorePageCenterAndScale(WebCore::FloatPoint unobscuredCenter, double scale)
    363363    DidGetTapHighlightGeometries(uint64_t requestID, WebCore::Color color, Vector<WebCore::FloatQuad> geometries, WebCore::IntSize topLeftRadius, WebCore::IntSize topRightRadius, WebCore::IntSize bottomLeftRadius, WebCore::IntSize bottomRightRadius)

  • trunk/Source/WebKit2/UIProcess/ios/PageClientImplIOS.h

    r197563 r198455  
    119119
    120120    void didCommitLayerTree(const RemoteLayerTreeTransaction&) override;
     121    void layerTreeCommitComplete() override;
     122
    121123    void dynamicViewportUpdateChangedTarget(double newScale, const WebCore::FloatPoint& newScrollPosition, uint64_t transactionID) override;
    122124    void couldNotRestorePageState() override;
    123     void restorePageState(const WebCore::FloatRect&, double) override;
     125    void restorePageState(const WebCore::FloatRect&, const WebCore::IntPoint&, double) override;
    124126    void restorePageCenterAndScale(const WebCore::FloatPoint&, double) override;
    125127

  • trunk/Source/WebKit2/UIProcess/ios/PageClientImplIOS.mm

    r196679 r198455  
    148148{
    149149    UNUSED_PARAM(isProgrammaticScroll);
    150     [m_webView _scrollToContentOffset:scrollPosition scrollOrigin:scrollOrigin];
     150    [m_webView _scrollToContentScrollPosition:scrollPosition scrollOrigin:scrollOrigin];
    151151}
    152152
    … …  
    513513}
    514514
     515void PageClientImpl::layerTreeCommitComplete()
     516{
     517    [m_contentView _layerTreeCommitComplete];
     518}
     519
    515520void PageClientImpl::dynamicViewportUpdateChangedTarget(double newScale, const WebCore::FloatPoint& newScrollPosition, uint64_t nextValidLayerTreeTransactionID)
    516521{
    … …  
    523528}
    524529
    525 void PageClientImpl::restorePageState(const WebCore::FloatRect& exposedRect, double scale)
    526 {
    527     [m_webView _restorePageStateToExposedRect:exposedRect scale:scale];
     530void PageClientImpl::restorePageState(const WebCore::FloatRect& exposedContentRect, const WebCore::IntPoint& scrollOrigin, double scale)
     531{
     532    [m_webView _restorePageStateToExposedRect:exposedContentRect scrollOrigin:scrollOrigin scale:scale];
    528533}
    529534

  • trunk/Source/WebKit2/UIProcess/ios/WKContentView.h

    r195651 r198455  
    8989- (void)_didCommitLoadForMainFrame;
    9090- (void)_didCommitLayerTree:(const WebKit::RemoteLayerTreeTransaction&)layerTreeTransaction;
     91- (void)_layerTreeCommitComplete;
    9192
    9293- (void)_setAccessibilityWebProcessToken:(NSData *)data;

  • trunk/Source/WebKit2/UIProcess/ios/WKContentView.mm

    r195424 r198455  
    521521}
    522522
     523- (void)_layerTreeCommitComplete
     524{
     525    [_webView _layerTreeCommitComplete];
     526}
     527
    523528- (void)_setAcceleratedCompositingRootView:(UIView *)rootView
    524529{

  • trunk/Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm

    r197986 r198455  
    373373}
    374374
     375void WebPageProxy::layerTreeCommitComplete()
     376{
     377    m_pageClient.layerTreeCommitComplete();
     378}
     379
    375380void WebPageProxy::selectWithGesture(const WebCore::IntPoint point, WebCore::TextGranularity granularity, uint32_t gestureType, uint32_t gestureState, bool isInteractingWithAssistedNode, std::function<void (const WebCore::IntPoint&, uint32_t, uint32_t, uint32_t, CallbackBase::Error)> callbackFunction)
    376381{
    … …  
    815820}
    816821
    817 void WebPageProxy::restorePageState(const WebCore::FloatRect& exposedRect, double scale)
    818 {
    819     m_pageClient.restorePageState(exposedRect, scale);
     822void WebPageProxy::restorePageState(const WebCore::FloatRect& exposedContentRect, const WebCore::IntPoint& scrollOrigin, double scale)
     823{
     824    m_pageClient.restorePageState(exposedContentRect, scrollOrigin, scale);
    820825}
    821826

  • trunk/Source/WebKit2/UIProcess/mac/RemoteLayerTreeDrawingAreaProxy.mm

    r194496 r198455  
    230230    }
    231231
     232    m_webPageProxy.layerTreeCommitComplete();
     233
    232234#if PLATFORM(IOS)
    233235    [m_displayLinkHandler schedule];

  • trunk/Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm

    r198446 r198455  
    291291    m_userHasChangedPageScaleFactor = !historyItem.scaleIsInitial();
    292292
     293    FrameView& frameView = *m_page->mainFrame().view();
     294
    293295    FloatSize currentMinimumLayoutSizeInScrollViewCoordinates = m_viewportConfiguration.minimumLayoutSize();
    294296    if (historyItem.minimumLayoutSizeInScrollViewCoordinates() == currentMinimumLayoutSizeInScrollViewCoordinates) {
    … …  
    298300        m_drawingArea->setExposedContentRect(historyItem.exposedContentRect());
    299301
    300         send(Messages::WebPageProxy::RestorePageState(historyItem.exposedContentRect(), boundedScale));
     302        send(Messages::WebPageProxy::RestorePageState(historyItem.exposedContentRect(), frameView.scrollOrigin(), boundedScale));
    301303    } else {
    302304        IntSize oldContentSize = historyItem.contentSize();
    303         FrameView& frameView = *m_page->mainFrame().view();
    304305        IntSize newContentSize = frameView.contentsSize();
    305306        double visibleHorizontalFraction = static_cast<float>(historyItem.unobscuredContentRect().width()) / oldContentSize.width();
Note: See TracChangeset for help on using the changeset viewer.