⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 194405 in webkit


Ignore:
Timestamp:
Dec 23, 2015, 4:35:04 PM (11 years ago)
Author:
Simon Fraser
Message:

REGRESSION (r187593): Scroll position jumps when selecting text in an iframe
​https://bugs.webkit.org/show_bug.cgi?id=152541
rdar://problem/23886181

Reviewed by Tim Horton.

Source/WebCore:

r154382 added code that modifies parentLayer traversal, looking for ancestor
scrollable layers. However, it confusingly added another code path in which
the ancestor layer traversal cross a frame boundary, when RenderLayer::scrollRectToVisible()
already has one. I fixed this new location to adjust the rect coordinates in r187593,
but then code that hit both crossing points double-mapped the coordinates, causing
autoscroll jumping.

Fix by reverting r154382 and r187593, going back to doing the ancestor walk in
one place. Re-fix r154382 by implementing RenderLayer::allowsCurrentScroll(),
which contains the logic for line clamp, autoscroll and ensuring that overflow:hidden
can be programmatically scrolled.

Form controls are special; they can have overflow:hidden but still be user-scrollable
during autoscroll; this is handled via the confusingly-named canBeProgramaticallyScrolled().
RenderTextControlSingleLine implements this to ensure that readonly text inputs
autoscroll (which is exercised by a test).

The frame-to-parent-frame rect mapping in RenderLayer::scrollRectToVisible() is
fixed to use the coordinate mapping functions from Widget/ScrollView, with the
addition of a new utility function contentsToContainingViewContents().

A "Scrolling" logging channel is added with a few log points.

Test: fast/events/autoscroll-in-iframe-body.html

  • page/scrolling/ScrollingCoordinator.cpp:

(WebCore::ScrollingCoordinator::absoluteNonFastScrollableRegionForFrame):
use contentsToContainingViewContents().

  • platform/Logging.h:
  • platform/ScrollView.cpp:

(WebCore::ScrollView::contentsToContainingViewContents):

  • platform/ScrollView.h:
  • platform/graphics/IntPoint.cpp:

(WebCore::IntPoint::constrainedBetween): New helper to constrain a point between
two other points.

  • platform/graphics/IntPoint.h:

(WebCore::IntPoint::expandedTo):
(WebCore::IntPoint::shrunkTo):

  • rendering/RenderBox.cpp:
  • rendering/RenderLayer.cpp:

(WebCore::parentLayerCrossFrame):
(WebCore::RenderLayer::enclosingScrollableLayer):
(WebCore::frameElementAndViewPermitScroll):
(WebCore::RenderLayer::allowsCurrentScroll):
(WebCore::RenderLayer::scrollRectToVisible):

  • rendering/RenderLayer.h:
  • rendering/RenderTextControlSingleLine.h:

LayoutTests:

New test for autoscrolling iframe contents (an existing test scrolled an overflow:scroll
inside an iframe, and didn't catch the bug).

  • fast/events/autoscroll-in-iframe-body-expected.txt: Added.
  • fast/events/autoscroll-in-iframe-body.html: Added.
  • fast/forms/input-readonly-autoscroll.html: Fix a missing double quote.
Location:
trunk
Files:
2 added
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r194404 r194405  
     12015-12-23  Simon Fraser  <simon.fraser@apple.com>
     2
     3        REGRESSION (r187593): Scroll position jumps when selecting text in an iframe
     4        https://bugs.webkit.org/show_bug.cgi?id=152541
     5        rdar://problem/23886181
     6
     7        Reviewed by Tim Horton.
     8       
     9        New test for autoscrolling iframe contents (an existing test scrolled an overflow:scroll
     10        inside an iframe, and didn't catch the bug).
     11
     12        * fast/events/autoscroll-in-iframe-body-expected.txt: Added.
     13        * fast/events/autoscroll-in-iframe-body.html: Added.
     14        * fast/forms/input-readonly-autoscroll.html: Fix a missing double quote.
     15
    1162015-12-22  Simon Fraser  <simon.fraser@apple.com>
    217

  • trunk/LayoutTests/fast/forms/input-readonly-autoscroll.html

    r121008 r194405  
    3939    </head>
    4040    <body onload="test()">
    41         <p>Test for <a href=http://bugs.webkit.org/show_bug.cgi?id=11534">bug 11534</a>.</p>
     41        <p>Test for <a href="http://bugs.webkit.org/show_bug.cgi?id=11534">bug 11534</a>.</p>
    4242        <p>Readonly text fields don't scroll when selecting content.</p>
    4343        <input id="tf" readonly value="abcdefghijklmnopqrstuvwxyz"></input>

  • trunk/Source/WebCore/ChangeLog

    r194404 r194405  
     12015-12-23  Simon Fraser  <simon.fraser@apple.com>
     2
     3        REGRESSION (r187593): Scroll position jumps when selecting text in an iframe
     4        https://bugs.webkit.org/show_bug.cgi?id=152541
     5        rdar://problem/23886181
     6
     7        Reviewed by Tim Horton.
     8       
     9        r154382 added code that modifies parentLayer traversal, looking for ancestor
     10        scrollable layers. However, it confusingly added another code path in which
     11        the ancestor layer traversal cross a frame boundary, when RenderLayer::scrollRectToVisible()
     12        already has one. I fixed this new location to adjust the rect coordinates in r187593,
     13        but then code that hit both crossing points double-mapped the coordinates, causing
     14        autoscroll jumping.
     15       
     16        Fix by reverting r154382 and r187593, going back to doing the ancestor walk in
     17        one place. Re-fix r154382 by implementing RenderLayer::allowsCurrentScroll(),
     18        which contains the logic for line clamp, autoscroll and ensuring that overflow:hidden
     19        can be programmatically scrolled.
     20       
     21        Form controls are special; they can have overflow:hidden but still be user-scrollable
     22        during autoscroll; this is handled via the confusingly-named canBeProgramaticallyScrolled().
     23        RenderTextControlSingleLine implements this to ensure that readonly text inputs
     24        autoscroll (which is exercised by a test).
     25       
     26        The frame-to-parent-frame rect mapping in RenderLayer::scrollRectToVisible() is
     27        fixed to use the coordinate mapping functions from Widget/ScrollView, with the
     28        addition of a new utility function contentsToContainingViewContents().
     29       
     30        A "Scrolling" logging channel is added with a few log points.
     31
     32        Test: fast/events/autoscroll-in-iframe-body.html
     33
     34        * page/scrolling/ScrollingCoordinator.cpp:
     35        (WebCore::ScrollingCoordinator::absoluteNonFastScrollableRegionForFrame):
     36        use contentsToContainingViewContents().
     37        * platform/Logging.h:
     38        * platform/ScrollView.cpp:
     39        (WebCore::ScrollView::contentsToContainingViewContents):
     40        * platform/ScrollView.h:
     41        * platform/graphics/IntPoint.cpp:
     42        (WebCore::IntPoint::constrainedBetween): New helper to constrain a point between
     43        two other points.
     44        * platform/graphics/IntPoint.h:
     45        (WebCore::IntPoint::expandedTo):
     46        (WebCore::IntPoint::shrunkTo):
     47        * rendering/RenderBox.cpp:
     48        * rendering/RenderLayer.cpp:
     49        (WebCore::parentLayerCrossFrame):
     50        (WebCore::RenderLayer::enclosingScrollableLayer):
     51        (WebCore::frameElementAndViewPermitScroll):
     52        (WebCore::RenderLayer::allowsCurrentScroll):
     53        (WebCore::RenderLayer::scrollRectToVisible):
     54        * rendering/RenderLayer.h:
     55        * rendering/RenderTextControlSingleLine.h:
     56
    1572015-12-22  Simon Fraser  <simon.fraser@apple.com>
    258

  • trunk/Source/WebCore/page/scrolling/ScrollingCoordinator.cpp

    r192193 r194405  
    161161        Region subframeRegion = absoluteNonFastScrollableRegionForFrame(*subframe);
    162162        // Map from the frame document to our document.
    163         IntPoint offset = subframeView->contentsToView(IntPoint());
    164         offset = subframeView->convertToContainingView(offset);
    165         offset = frameView->viewToContents(offset);
     163        IntPoint offset = subframeView->contentsToContainingViewContents(IntPoint());
    166164
    167165        // FIXME: this translation ignores non-trival transforms on the frame.

  • trunk/Source/WebCore/platform/Logging.h

    r191474 r194405  
    7878    M(SVG) \
    7979    M(Services) \
     80    M(Scrolling) \
    8081    M(SpellingAndGrammar) \
    8182    M(StorageAPI) \

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

    r194210 r194405  
    3030#include "GraphicsLayer.h"
    3131#include "HostWindow.h"
     32#include "Logging.h"
    3233#include "PlatformMouseEvent.h"
    3334#include "PlatformWheelEvent.h"
    … …  
    3536#include "Scrollbar.h"
    3637#include "ScrollbarTheme.h"
     38#include "TextStream.h"
    3739#include <wtf/StdLibExtras.h>
    3840
    … …  
    872874}
    873875
     876IntPoint ScrollView::contentsToContainingViewContents(const IntPoint& point) const
     877{
     878    if (const ScrollView* parentScrollView = parent()) {
     879        IntPoint pointInContainingView = convertToContainingView(contentsToView(point));
     880        return parentScrollView->viewToContents(pointInContainingView);
     881    }
     882
     883    return contentsToView(point);
     884}
     885
     886IntRect ScrollView::contentsToContainingViewContents(IntRect rect) const
     887{
     888    if (const ScrollView* parentScrollView = parent()) {
     889        IntRect rectInContainingView = convertToContainingView(contentsToView(rect));
     890        return parentScrollView->viewToContents(rectInContainingView);
     891    }
     892
     893    return contentsToView(rect);
     894}
     895
    874896IntPoint ScrollView::rootViewToContents(const IntPoint& rootViewPoint) const
    875897{

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

    r189144 r194405  
    295295    IntRect contentsToView(IntRect) const;
    296296
     297    IntPoint contentsToContainingViewContents(const IntPoint&) const;
     298    IntRect contentsToContainingViewContents(IntRect) const;
     299
    297300    WEBCORE_EXPORT IntPoint rootViewToTotalContents(const IntPoint&) const;
    298301

  • trunk/Source/WebCore/platform/graphics/IntPoint.cpp

    r191216 r194405  
    3838}
    3939
     40IntPoint IntPoint::constrainedBetween(const IntPoint& min, const IntPoint& max) const
     41{
     42    return {
     43        std::max(min.x(), std::min(max.x(), m_x)),
     44        std::max(min.y(), std::min(max.y(), m_y))
     45    };
     46}
     47
     48
    4049TextStream& operator<<(TextStream& ts, const IntPoint& p)
    4150{

  • trunk/Source/WebCore/platform/graphics/IntPoint.h

    r191216 r194405  
    8585    IntPoint expandedTo(const IntPoint& other) const
    8686    {
    87         return IntPoint(m_x > other.m_x ? m_x : other.m_x,
    88             m_y > other.m_y ? m_y : other.m_y);
     87        return {
     88            m_x > other.m_x ? m_x : other.m_x,
     89            m_y > other.m_y ? m_y : other.m_y
     90        };
    8991    }
    9092
    9193    IntPoint shrunkTo(const IntPoint& other) const
    9294    {
    93         return IntPoint(m_x < other.m_x ? m_x : other.m_x,
    94             m_y < other.m_y ? m_y : other.m_y);
    95     }
     95        return {
     96            m_x < other.m_x ? m_x : other.m_x,
     97            m_y < other.m_y ? m_y : other.m_y
     98        };
     99    }
     100
     101    IntPoint constrainedBetween(const IntPoint& min, const IntPoint& max) const;
    96102
    97103    int distanceSquaredToPoint(const IntPoint&) const;

  • trunk/Source/WebCore/rendering/RenderBox.cpp

    r194404 r194405  
    876876}
    877877
     878// FIXME: This is badly named. overflow:hidden can be programmatically scrolled, yet this returns false in that case.
    878879bool RenderBox::canBeProgramaticallyScrolled() const
    879880{

  • trunk/Source/WebCore/rendering/RenderLayer.cpp

    r194184 r194405  
    7979#include "HitTestRequest.h"
    8080#include "HitTestResult.h"
     81#include "Logging.h"
    8182#include "OverflowEvent.h"
    8283#include "OverlapTestRequestClient.h"
    … …  
    14551456}
    14561457
    1457 static RenderLayer* parentLayerCrossFrame(const RenderLayer& layer, LayoutRect* rect = nullptr)
     1458static RenderLayer* parentLayerCrossFrame(const RenderLayer& layer)
    14581459{
    14591460    if (layer.parent())
    … …  
    14681469        return nullptr;
    14691470
    1470     // Convert the rect into the coordinate space of the parent frame's document.
    1471     if (rect) {
    1472         IntRect viewRect = layer.renderer().frame().view()->convertToContainingView(enclosingIntRect(*rect));
    1473         *rect = ownerRenderer->frame().view()->viewToContents(viewRect);
    1474     }
    1475 
    14761471    return ownerRenderer->enclosingLayer();
    14771472}
    14781473
    1479 RenderLayer* RenderLayer::enclosingScrollableLayer(LayoutRect* rect) const
    1480 {
    1481     for (RenderLayer* nextLayer = parentLayerCrossFrame(*this, rect); nextLayer; nextLayer = parentLayerCrossFrame(*nextLayer, rect)) {
     1474RenderLayer* RenderLayer::enclosingScrollableLayer() const
     1475{
     1476    for (RenderLayer* nextLayer = parentLayerCrossFrame(*this); nextLayer; nextLayer = parentLayerCrossFrame(*nextLayer)) {
    14821477        if (is<RenderBox>(nextLayer->renderer()) && downcast<RenderBox>(nextLayer->renderer()).canBeScrolledAndHasScrollableArea())
    14831478            return nextLayer;
    … …  
    22842279}
    22852280
     2281// FIXME: unify with the scrollRectToVisible() code below.
    22862282void RenderLayer::scrollByRecursively(const IntSize& delta, ScrollOffsetClamping clamp, ScrollableArea** scrolledArea)
    22872283{
    … …  
    24372433}
    24382434
    2439 static inline bool frameElementAndViewPermitScroll(HTMLFrameElementBase* frameElementBase, FrameView* frameView)
     2435static inline bool frameElementAndViewPermitScroll(HTMLFrameElementBase* frameElementBase, FrameView& frameView)
    24402436{
    24412437    // If scrollbars aren't explicitly forbidden, permit scrolling.
    … …  
    24442440
    24452441    // If scrollbars are forbidden, user initiated scrolls should obviously be ignored.
    2446     if (frameView->wasScrolledByUser())
     2442    if (frameView.wasScrolledByUser())
    24472443        return false;
    24482444
    24492445    // Forbid autoscrolls when scrollbars are off, but permits other programmatic scrolls,
    24502446    // like navigation to an anchor.
    2451     return !frameView->frame().eventHandler().autoscrollInProgress();
     2447    return !frameView.frame().eventHandler().autoscrollInProgress();
     2448}
     2449
     2450bool RenderLayer::allowsCurrentScroll() const
     2451{
     2452    if (!renderer().hasOverflowClip())
     2453        return false;
     2454
     2455    // Don't scroll to reveal an overflow layer that is restricted by the -webkit-line-clamp property.
     2456    // FIXME: Is this still needed? It used to be relevant for Safari RSS.
     2457    if (renderer().parent() && !renderer().parent()->style().lineClamp().isNone())
     2458        return false;
     2459
     2460    RenderBox* box = renderBox();
     2461    ASSERT(box); // Only boxes can have overflowClip set.
     2462
     2463    if (renderer().frame().eventHandler().autoscrollInProgress()) {
     2464        // The "programmatically" here is misleading; this asks whether the box has scrollable overflow,
     2465        // or is a special case like a form control.
     2466        return box->canBeProgramaticallyScrolled();
     2467    }
     2468
     2469    // Programmatic scrolls can scroll overflow:hidden.
     2470    return box->hasHorizontalOverflow() || box->hasVerticalOverflow();
    24522471}
    24532472
    24542473void RenderLayer::scrollRectToVisible(const LayoutRect& rect, const ScrollAlignment& alignX, const ScrollAlignment& alignY)
    24552474{
     2475    LOG_WITH_STREAM(Scrolling, stream << "Layer " << this << " scrollRectToVisible " << rect);
     2476
    24562477    RenderLayer* parentLayer = nullptr;
    24572478    LayoutRect newRect = rect;
    … …  
    24612482    FrameView& frameView = renderer().view().frameView();
    24622483
    2463     bool restrictedByLineClamp = false;
    2464     if (renderer().parent()) {
     2484    if (renderer().parent())
    24652485        parentLayer = renderer().parent()->enclosingLayer();
    2466         restrictedByLineClamp = !renderer().parent()->style().lineClamp().isNone();
    2467     }
    2468 
    2469     if (renderer().hasOverflowClip() && !restrictedByLineClamp) {
     2486
     2487    if (allowsCurrentScroll()) {
    24702488        // Don't scroll to reveal an overflow layer that is restricted by the -webkit-line-clamp property.
    24712489        // This will prevent us from revealing text hidden by the slider in Safari RSS.
    … …  
    24842502            newRect = LayoutRect(box->localToAbsoluteQuad(FloatQuad(FloatRect(localExposeRect)), UseTransforms).boundingBox());
    24852503        }
    2486     } else if (!parentLayer && renderer().isBox() && renderBox()->canBeProgramaticallyScrolled()) {
     2504    } else if (!parentLayer && renderer().isRenderView()) {
    24872505        HTMLFrameOwnerElement* ownerElement = renderer().document().ownerElement();
    24882506
    … …  
    24932511                frameElementBase = downcast<HTMLFrameElementBase>(ownerElement);
    24942512
    2495             if (frameElementAndViewPermitScroll(frameElementBase, &frameView)) {
     2513            if (frameElementAndViewPermitScroll(frameElementBase, frameView)) {
    24962514                LayoutRect viewRect = frameView.visibleContentRect(LegacyIOSDocumentVisibleRect);
    24972515                LayoutRect exposeRect = getRectToExpose(viewRect, viewRect, rect, alignX, alignY);
    24982516
    2499                 int xOffset = roundToInt(exposeRect.x());
    2500                 int yOffset = roundToInt(exposeRect.y());
     2517                IntPoint scrollOffset(roundedIntPoint(exposeRect.location()));
    25012518                // Adjust offsets if they're outside of the allowable range.
    2502                 xOffset = std::max(0, std::min(frameView.contentsWidth(), xOffset));
    2503                 yOffset = std::max(0, std::min(frameView.contentsHeight(), yOffset));
    2504 
    2505                 frameView.setScrollPosition(IntPoint(xOffset, yOffset));
     2519                scrollOffset = scrollOffset.constrainedBetween(IntPoint(), IntPoint(frameView.contentsSize()));
     2520                frameView.setScrollPosition(scrollOffset);
     2521
    25062522                if (frameView.safeToPropagateScrollToParent()) {
    25072523                    parentLayer = ownerElement->renderer()->enclosingLayer();
    2508                     // FIXME: This doesn't correctly convert the rect to
    2509                     // absolute coordinates in the parent.
    2510                     newRect.setX(rect.x() - frameView.scrollX() + frameView.x());
    2511                     newRect.setY(rect.y() - frameView.scrollY() + frameView.y());
     2524                    // Convert the rect into the coordinate space of the parent frame's document.
     2525                    newRect = frameView.contentsToContainingViewContents(enclosingIntRect(newRect));
    25122526                } else
    25132527                    parentLayer = nullptr;
    … …  
    25382552    }
    25392553   
    2540     if (renderer().frame().eventHandler().autoscrollInProgress())
    2541         parentLayer = enclosingScrollableLayer(&newRect);
    2542 
    25432554    if (parentLayer)
    25442555        parentLayer->scrollRectToVisible(newRect, alignX, alignY);

  • trunk/Source/WebCore/rendering/RenderLayer.h

    r192193 r194405  
    405405
    406406    // Returns the nearest enclosing layer that is scrollable.
    407     RenderLayer* enclosingScrollableLayer(LayoutRect* = nullptr) const;
     407    RenderLayer* enclosingScrollableLayer() const;
    408408
    409409    // The layer relative to which clipping rects for this layer are computed.
    … …  
    898898   
    899899    void updateScrollableAreaSet(bool hasOverflow);
     900   
     901    bool allowsCurrentScroll() const;
    900902
    901903    void dirtyAncestorChainVisibleDescendantStatus();

  • trunk/Source/WebCore/rendering/RenderTextControlSingleLine.h

    r180490 r194405  
    106106    virtual bool hasLineIfEmpty() const override { return true; }
    107107    virtual bool isTextControlInnerBlock() const override { return true; }
     108    virtual bool canBeProgramaticallyScrolled() const override { return true; }
    108109};
    109110
Note: See TracChangeset for help on using the changeset viewer.