Changeset 218982 in webkit


Ignore:
Timestamp:
Jun 29, 2017, 7:09:09 PM (8 years ago)
Author:
Simon Fraser
Message:

getBoundingClientRect returns wrong value for combination of page zoom and scroll
​https://bugs.webkit.org/show_bug.cgi?id=173841
rdar://problem/32983841

Reviewed by Dean Jackson.

Source/WebCore:

The layout viewport returned by FrameView::layoutViewportRect() is affected by page (Command-+) zooming,
since it's computed using scroll positions, so when we use its origin to convert into client coordinates
(which are zoom-agnostic), we need to account for page zoom, so fix FrameView::documentToClientOffset()
to do this.

Callers of documentToClientOffset() were checked, revealing that event client coordinates were also
wrong with page zoom and are fixed in the same way. It was found that SimulatedClick was using an
entirely wrong rect to compute its location: Element::clientRect() is NOT in client coordinates,
so change this code to use getBoundingClientRect() instead.

Minor refactoring in MouseRelatedEvent to make getting to the FrameView cleaner.

Some geometry types enhanced to have non-mutating scale functions.

Tests: fast/events/simulated-click-zoomed.html

fast/visual-viewport/client-rects-relative-to-layout-viewport-zoomed.html

  • dom/MouseRelatedEvent.cpp:

(WebCore::MouseRelatedEvent::init):
(WebCore::MouseRelatedEvent::initCoordinates):
(WebCore::MouseRelatedEvent::frameView):
(WebCore::MouseRelatedEvent::documentToAbsoluteScaleFactor):
(WebCore::MouseRelatedEvent::computePageLocation):
(WebCore::MouseRelatedEvent::computeRelativePosition):
(WebCore::pageZoomFactor): Deleted.
(WebCore::frameScaleFactor): Deleted.

  • dom/MouseRelatedEvent.h:

(WebCore::MouseRelatedEvent::absoluteLocation):
(WebCore::MouseRelatedEvent::setAbsoluteLocation): Deleted.

  • dom/SimulatedClick.cpp:
  • page/FrameView.cpp:

(WebCore::FrameView::layoutViewportRect): baseLayoutViewportSize() is the same as the old code.
(WebCore::FrameView::documentToAbsoluteScaleFactor):
(WebCore::FrameView::absoluteToDocumentScaleFactor):
(WebCore::FrameView::absoluteToDocumentPoint):
(WebCore::FrameView::documentToClientOffset):

  • page/FrameView.h:
  • platform/graphics/FloatPoint.h:

(WebCore::FloatPoint::scale):
(WebCore::FloatPoint::scaled):

  • platform/graphics/FloatSize.h:

(WebCore::FloatSize::scaled):

  • platform/graphics/LayoutPoint.h:

(WebCore::LayoutPoint::scaled):

Tools:

Make "Zoom In" and "Zoom Out" work correctly in the WebKit1 window. Previously they
always did text zooming.

  • MiniBrowser/mac/WK1BrowserWindowController.m:

(-[WK1BrowserWindowController zoomIn:]):
(-[WK1BrowserWindowController zoomOut:]):
(-[WK1BrowserWindowController canResetZoom]):
(-[WK1BrowserWindowController resetZoom:]):

LayoutTests:

  • fast/events/clientXY-in-zoom-and-scroll.html: New baseline for progressed behavior.
  • fast/events/simulated-click-zoomed-expected.txt: Added.
  • fast/events/simulated-click-zoomed.html: Added.
  • fast/visual-viewport/client-rects-relative-to-layout-viewport-zoomed-expected.txt: Added.
  • fast/visual-viewport/client-rects-relative-to-layout-viewport-zoomed.html: Added.
  • platform/ios/TestExpectations:
  • platform/mac/fast/events/clientXY-in-zoom-and-scroll-expected.txt:
Location:
trunk
Files:
4 added
17 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r218965 r218982  
     12017-06-28  Simon Fraser  <simon.fraser@apple.com>
     2
     3        getBoundingClientRect returns wrong value for combination of page zoom and scroll
     4        https://bugs.webkit.org/show_bug.cgi?id=173841
     5        rdar://problem/32983841
     6
     7        Reviewed by Dean Jackson.
     8
     9        * fast/events/clientXY-in-zoom-and-scroll.html: New baseline for progressed behavior.
     10        * fast/events/simulated-click-zoomed-expected.txt: Added.
     11        * fast/events/simulated-click-zoomed.html: Added.
     12        * fast/visual-viewport/client-rects-relative-to-layout-viewport-zoomed-expected.txt: Added.
     13        * fast/visual-viewport/client-rects-relative-to-layout-viewport-zoomed.html: Added.
     14        * platform/ios/TestExpectations:
     15        * platform/mac/fast/events/clientXY-in-zoom-and-scroll-expected.txt:
     16
    1172017-06-29  John Wilander  <wilander@apple.com>
    218

  • trunk/LayoutTests/fast/events/clientXY-in-zoom-and-scroll.html

    r216824 r218982  
    105105        event = e;
    106106        debug("\nZoomed and scrolled");
    107         shouldBe("event.clientX", hasSubpixelSupport ? "73" : "74");
    108         shouldBe("event.clientY", hasSubpixelSupport ? "73" : "74");
     107        shouldBe("event.clientX", hasSubpixelSupport ? "83" : "84");
     108        shouldBe("event.clientY", hasSubpixelSupport ? "83" : "84");
    109109        shouldBe("event.pageX", "133");
    110110        shouldBe("event.pageY", "133");

  • trunk/LayoutTests/platform/ios/TestExpectations

    r218722 r218982  
    328328fast/scrolling/scroll-animator-select-list-events.html [ Skip ]
    329329fast/events/prevent-default-prevents-interaction-with-scrollbars.html [ Skip ]
     330fast/events/simulated-click-zoomed.html [ Skip ]
    330331fast/text/text-disappear-on-deselect.html [ ImageOnlyFailure ]
    331332fast/css/pseudo-active-on-labeled-control-without-renderer.html [ Skip ]
    … …  
    27272728fast/scrolling/scroll-to-anchor-zoomed-header.html [ Skip ]
    27282729fast/visual-viewport/client-coordinates-relative-to-layout-viewport.html [ Skip ]
     2730fast/visual-viewport/client-rects-relative-to-layout-viewport-zoomed.html [ Skip ]
    27292731
    27302732# On iOS we do not visually highlight a programmatic selection

  • trunk/LayoutTests/platform/mac/fast/events/clientXY-in-zoom-and-scroll-expected.txt

    r216824 r218982  
    1818
    1919Zoomed and scrolled
    20 PASS event.clientX is 73
    21 PASS event.clientY is 73
     20PASS event.clientX is 83
     21PASS event.clientY is 83
    2222PASS event.pageX is 133
    2323PASS event.pageY is 133

  • trunk/Source/WebCore/ChangeLog

    r218979 r218982  
     12017-06-28  Simon Fraser  <simon.fraser@apple.com>
     2
     3        getBoundingClientRect returns wrong value for combination of page zoom and scroll
     4        https://bugs.webkit.org/show_bug.cgi?id=173841
     5        rdar://problem/32983841
     6
     7        Reviewed by Dean Jackson.
     8
     9        The layout viewport returned by FrameView::layoutViewportRect() is affected by page (Command-+) zooming,
     10        since it's computed using scroll positions, so when we use its origin to convert into client coordinates
     11        (which are zoom-agnostic), we need to account for page zoom, so fix FrameView::documentToClientOffset()
     12        to do this.
     13
     14        Callers of documentToClientOffset() were checked, revealing that event client coordinates were also
     15        wrong with page zoom and are fixed in the same way. It was found that SimulatedClick was using an
     16        entirely wrong rect to compute its location: Element::clientRect() is NOT in client coordinates,
     17        so change this code to use getBoundingClientRect() instead.
     18
     19        Minor refactoring in MouseRelatedEvent to make getting to the FrameView cleaner.
     20
     21        Some geometry types enhanced to have non-mutating scale functions.
     22
     23        Tests: fast/events/simulated-click-zoomed.html
     24               fast/visual-viewport/client-rects-relative-to-layout-viewport-zoomed.html
     25
     26        * dom/MouseRelatedEvent.cpp:
     27        (WebCore::MouseRelatedEvent::init):
     28        (WebCore::MouseRelatedEvent::initCoordinates):
     29        (WebCore::MouseRelatedEvent::frameView):
     30        (WebCore::MouseRelatedEvent::documentToAbsoluteScaleFactor):
     31        (WebCore::MouseRelatedEvent::computePageLocation):
     32        (WebCore::MouseRelatedEvent::computeRelativePosition):
     33        (WebCore::pageZoomFactor): Deleted.
     34        (WebCore::frameScaleFactor): Deleted.
     35        * dom/MouseRelatedEvent.h:
     36        (WebCore::MouseRelatedEvent::absoluteLocation):
     37        (WebCore::MouseRelatedEvent::setAbsoluteLocation): Deleted.
     38        * dom/SimulatedClick.cpp:
     39        * page/FrameView.cpp:
     40        (WebCore::FrameView::layoutViewportRect): baseLayoutViewportSize() is the same as the old code.
     41        (WebCore::FrameView::documentToAbsoluteScaleFactor):
     42        (WebCore::FrameView::absoluteToDocumentScaleFactor):
     43        (WebCore::FrameView::absoluteToDocumentPoint):
     44        (WebCore::FrameView::documentToClientOffset):
     45        * page/FrameView.h:
     46        * platform/graphics/FloatPoint.h:
     47        (WebCore::FloatPoint::scale):
     48        (WebCore::FloatPoint::scaled):
     49        * platform/graphics/FloatSize.h:
     50        (WebCore::FloatSize::scaled):
     51        * platform/graphics/LayoutPoint.h:
     52        (WebCore::LayoutPoint::scaled):
     53
    1542017-06-29  Megan Gardner  <megan_gardner@apple.com>
    255

  • trunk/Source/WebCore/dom/Element.cpp

    r218501 r218982  
    11631163}
    11641164
    1165 Ref<DOMRect> Element::getBoundingClientRect()
     1165FloatRect Element::boundingClientRect()
    11661166{
    11671167    document().updateLayoutIgnorePendingStylesheets();
    … …  
    11811181
    11821182    if (quads.isEmpty())
    1183         return DOMRect::create();
     1183        return { };
    11841184
    11851185    FloatRect result = quads[0].boundingBox();
    … …  
    11881188
    11891189    document().convertAbsoluteToClientRect(result, renderer()->style());
    1190     return DOMRect::create(result);
     1190    return result;
     1191}
     1192
     1193Ref<DOMRect> Element::getBoundingClientRect()
     1194{
     1195    return DOMRect::create(boundingClientRect());
    11911196}
    11921197

  • trunk/Source/WebCore/dom/Element.h

    r218345 r218982  
    174174    WEBCORE_EXPORT IntRect boundsInRootViewSpace();
    175175
     176    FloatRect boundingClientRect();
     177
    176178    Ref<DOMRectList> getClientRects();
    177179    Ref<DOMRect> getBoundingClientRect();

  • trunk/Source/WebCore/dom/MouseRelatedEvent.cpp

    r216824 r218982  
    6060void MouseRelatedEvent::init(bool isSimulated, const IntPoint& windowLocation)
    6161{
    62     auto* frame = view() ? view()->frame() : nullptr;
    63     if (frame && !isSimulated) {
    64         if (FrameView* frameView = frame->view()) {
     62    if (!isSimulated) {
     63        if (auto* frameView = this->frameView()) {
    6564            FloatPoint absolutePoint = frameView->windowToContents(windowLocation);
    6665            FloatPoint documentPoint = frameView->absoluteToDocumentPoint(absolutePoint);
    … …  
    8988    // Correct values are computed lazily, see computeRelativePosition.
    9089    FloatSize documentToClientOffset;
    91     auto* frame = view() ? view()->frame() : nullptr;
    92     if (frame) {
    93         if (FrameView* frameView = frame->view())
    94             documentToClientOffset = frameView->documentToClientOffset();
    95     }
     90    if (auto* frameView = this->frameView())
     91        documentToClientOffset = frameView->documentToClientOffset();
    9692
    9793    m_clientLocation = clientLocation;
    … …  
    105101}
    106102
    107 static float pageZoomFactor(const UIEvent* event)
    108 {
    109     DOMWindow* window = event->view();
    110     if (!window)
    111         return 1;
    112     Frame* frame = window->frame();
     103FrameView* MouseRelatedEvent::frameView() const
     104{
     105    auto* frame = view() ? view()->frame() : nullptr;
    113106    if (!frame)
    114         return 1;
    115     return frame->pageZoomFactor();
    116 }
    117 
    118 static float frameScaleFactor(const UIEvent* event)
    119 {
    120     DOMWindow* window = event->view();
    121     if (!window)
    122         return 1;
    123     Frame* frame = window->frame();
    124     if (!frame)
    125         return 1;
    126     return frame->frameScaleFactor();
     107        return nullptr;
     108
     109    return frame->view();
     110}
     111
     112float MouseRelatedEvent::documentToAbsoluteScaleFactor() const
     113{
     114    if (auto* frameView = this->frameView())
     115        return frameView->documentToAbsoluteScaleFactor();
     116
     117    return 1;
    127118}
    128119
    129120void MouseRelatedEvent::computePageLocation()
    130121{
    131     float scaleFactor = pageZoomFactor(this) * frameScaleFactor(this);
    132     setAbsoluteLocation(LayoutPoint(pageX() * scaleFactor, pageY() * scaleFactor));
     122    m_absoluteLocation = m_pageLocation.scaled(documentToAbsoluteScaleFactor());
    133123}
    134124
    … …  
    154144    if (RenderObject* r = targetNode->renderer()) {
    155145        m_offsetLocation = LayoutPoint(r->absoluteToLocal(absoluteLocation(), UseTransforms));
    156         float scaleFactor = 1 / (pageZoomFactor(this) * frameScaleFactor(this));
     146        float scaleFactor = 1 / documentToAbsoluteScaleFactor();
    157147        if (scaleFactor != 1.0f)
    158148            m_offsetLocation.scale(scaleFactor);

  • trunk/Source/WebCore/dom/MouseRelatedEvent.h

    r216824 r218982  
    2828
    2929namespace WebCore {
     30
     31class FrameView;
    3032
    3133struct MouseRelatedEventInit : public EventModifierInit {
    … …  
    6365    // usable with RenderObject::absoluteToLocal).
    6466    const LayoutPoint& absoluteLocation() const { return m_absoluteLocation; }
    65     void setAbsoluteLocation(const LayoutPoint& p) { m_absoluteLocation = p; }
    6667
    6768protected:
    … …  
    8182    void computePageLocation();
    8283    void computeRelativePosition();
     84
     85    float documentToAbsoluteScaleFactor() const;
     86    FrameView* frameView() const;
    8387
    8488    // Expose these so MouseEvent::initMouseEvent can set them.

  • trunk/Source/WebCore/dom/SimulatedClick.cpp

    r210216 r218982  
    2727#include "SimulatedClick.h"
    2828
     29#include "DOMRect.h"
    2930#include "DataTransfer.h"
    3031#include "Element.h"
    … …  
    7374            // Note that the call to screenRect() causes a synchronous IPC with the UI process.
    7475            m_screenLocation = target.screenRect().center();
    75             initCoordinates(LayoutPoint(target.clientRect().center()));
     76            initCoordinates(LayoutPoint(target.boundingClientRect().center()));
    7677        }
    7778    }

  • trunk/Source/WebCore/page/FrameView.cpp

    r218928 r218982  
    19791979
    19801980    // Size of initial containing block, anchored at scroll position, in document coordinates (unchanged by scale factor).
    1981     return LayoutRect(m_layoutViewportOrigin, renderView() ? renderView()->size() : size());
     1981    return LayoutRect(m_layoutViewportOrigin, baseLayoutViewportSize());
    19821982}
    19831983
    … …  
    48834883}
    48844884
     4885float FrameView::documentToAbsoluteScaleFactor(std::optional<float> effectiveZoom) const
     4886{
     4887    // If effectiveZoom is passed, it already factors in pageZoomFactor().
     4888    return effectiveZoom.value_or(frame().pageZoomFactor()) * frame().frameScaleFactor();
     4889}
     4890
    48854891float FrameView::absoluteToDocumentScaleFactor(std::optional<float> effectiveZoom) const
    48864892{
    48874893    // If effectiveZoom is passed, it already factors in pageZoomFactor().
    4888     float cssZoom = effectiveZoom.value_or(frame().pageZoomFactor());
    4889     return 1 / (cssZoom * frame().frameScaleFactor());
     4894    return 1 / documentToAbsoluteScaleFactor(effectiveZoom);
    48904895}
    48914896
    … …  
    48984903FloatPoint FrameView::absoluteToDocumentPoint(FloatPoint p, std::optional<float> effectiveZoom) const
    48994904{
    4900     p.scale(absoluteToDocumentScaleFactor(effectiveZoom));
    4901     return p;
     4905    return p.scaled(absoluteToDocumentScaleFactor(effectiveZoom));
    49024906}
    49034907
    49044908FloatSize FrameView::documentToClientOffset() const
    49054909{
    4906     return frame().settings().visualViewportEnabled() ? -toFloatSize(layoutViewportRect().location()) : -toFloatSize(visibleContentRect().location());
     4910    FloatSize clientOrigin = frame().settings().visualViewportEnabled() ? -toFloatSize(layoutViewportRect().location()) : -toFloatSize(visibleContentRect().location());
     4911
     4912    // Layout and visual viewports are affected by page zoom, so we need to factor that out.
     4913    return clientOrigin.scaled(1 / frame().pageZoomFactor());
    49074914}
    49084915

  • trunk/Source/WebCore/page/FrameView.h

    r218501 r218982  
    260260    WEBCORE_EXPORT void setLayoutViewportOverrideRect(std::optional<LayoutRect>);
    261261
    262     // These are in document coordinates, unaffected by zooming.
     262    // These are in document coordinates, unaffected by page scale (but affected by zooming).
    263263    WEBCORE_EXPORT LayoutRect layoutViewportRect() const;
    264264    WEBCORE_EXPORT LayoutRect visualViewportRect() const;
    … …  
    446446    // "Document"
    447447    //    Relative to the document's scroll origin, but not affected by page zoom or page scale. Size is equivalent to CSS pixel dimensions.
     448    //    FIXME: some uses are affected by page zoom (e.g. layout and visual viewports).
    448449    //
    449450    // "Client"
    … …  
    464465    IntPoint convertFromContainingView(const IntPoint&) const final;
    465466
     467    float documentToAbsoluteScaleFactor(std::optional<float> effectiveZoom = std::nullopt) const;
    466468    float absoluteToDocumentScaleFactor(std::optional<float> effectiveZoom = std::nullopt) const;
     469
    467470    FloatRect absoluteToDocumentRect(FloatRect, std::optional<float> effectiveZoom = std::nullopt) const;
    468471    FloatPoint absoluteToDocumentPoint(FloatPoint, std::optional<float> effectiveZoom = std::nullopt) const;

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

    r218368 r218982  
    119119    }
    120120
    121     void scale(float sx, float sy)
    122     {
    123         m_x *= sx;
    124         m_y *= sy;
     121    void scale(float scaleX, float scaleY)
     122    {
     123        m_x *= scaleX;
     124        m_y *= scaleY;
     125    }
     126
     127    FloatPoint scaled(float scale)
     128    {
     129        return { m_x * scale, m_y * scale };
     130    }
     131
     132    FloatPoint scaled(float scaleX, float scaleY)
     133    {
     134        return { m_x * scaleX, m_y * scaleY };
    125135    }
    126136

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

    r218368 r218982  
    9595    }
    9696
     97    FloatSize scaled(float s) const
     98    {
     99        return { m_width * s, m_height * s };
     100    }
     101
     102    FloatSize scaled(float scaleX, float scaleY) const
     103    {
     104        return { m_width * scaleX, m_height * scaleY };
     105    }
     106
    97107    WEBCORE_EXPORT FloatSize constrainedBetween(const FloatSize& min, const FloatSize& max) const;
    98108

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

    r218368 r218982  
    6868    }
    6969
     70    LayoutPoint scaled(float s) const
     71    {
     72        return { m_x * s, m_y * s };
     73    }
     74
     75    LayoutPoint scaled(float sx, float sy) const
     76    {
     77        return { m_x * sx, m_y * sy };
     78    }
     79
    7080    LayoutPoint constrainedBetween(const LayoutPoint& min, const LayoutPoint& max) const;
    7181

  • trunk/Tools/ChangeLog

    r218965 r218982  
     12017-06-28  Simon Fraser  <simon.fraser@apple.com>
     2
     3        getBoundingClientRect returns wrong value for combination of page zoom and scroll
     4        https://bugs.webkit.org/show_bug.cgi?id=173841
     5        rdar://problem/32983841
     6
     7        Reviewed by Dean Jackson.
     8
     9        Make "Zoom In" and "Zoom Out" work correctly in the WebKit1 window. Previously they
     10        always did text zooming.
     11
     12        * MiniBrowser/mac/WK1BrowserWindowController.m:
     13        (-[WK1BrowserWindowController zoomIn:]):
     14        (-[WK1BrowserWindowController zoomOut:]):
     15        (-[WK1BrowserWindowController canResetZoom]):
     16        (-[WK1BrowserWindowController resetZoom:]):
     17
    1182017-06-29  John Wilander  <wilander@apple.com>
    219

  • trunk/Tools/MiniBrowser/mac/WK1BrowserWindowController.m

    r217117 r218982  
    200200        return;
    201201
    202     [_webView makeTextLarger:sender];
     202    if (_zoomTextOnly)
     203        [_webView makeTextLarger:sender];
     204    else
     205        [_webView zoomPageIn:sender];
     206
    203207}
    204208
    … …  
    213217        return;
    214218
    215     [_webView makeTextSmaller:sender];
     219    if (_zoomTextOnly)
     220        [_webView makeTextSmaller:sender];
     221    else
     222        [_webView zoomPageOut:sender];
    216223}
    217224
    218225- (BOOL)canResetZoom
    219226{
    220     return [_webView canMakeTextStandardSize];
     227    return _zoomTextOnly ? [_webView canMakeTextStandardSize] : [_webView canResetPageZoom];
    221228}
    222229
    … …  
    226233        return;
    227234
    228     [_webView makeTextStandardSize:sender];
     235    if (_zoomTextOnly)
     236        [_webView makeTextStandardSize:sender];
     237    else
     238        [_webView resetPageZoom:sender];
    229239}
    230240
Note: See TracChangeset for help on using the changeset viewer.