Bug 172019

document.elementFromPoint() ignores elements outside the "viewport", but should we climb based on the layout viewport, or the visual viewport? I always thought this clipping behavior was arbitrary and interfere with genuine use cases; not sure if it's a security policy or not.

Need to fix this to fix the imported/w3c/web-platform-tests/cssom-view/elementFromPoint.html test.

<rdar://problem/32178867>

Created attachment 313860 [details] WIP Still need to add tests and sort out failures.

Created attachment 314585 [details] Patch

Comment on attachment 314585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314585&action=review > Source/WebCore/dom/TreeScope.cpp:307 > + documentScope().updateLayout(); Why is this documentScope().updateLayout(); only in the visual viewport code path? > Source/WebCore/dom/TreeScope.cpp:308 > + FloatPoint layoutViewportPoint = view->clientToLayoutViewportPoint(clientPoint); clientPoint should already be in layout viewport coordinates from the caller. > Source/WebCore/page/FrameView.h:480 > + FloatRect layoutViewportToAbsoluteRect(FloatRect) const; > + FloatPoint layoutViewportToAbsolutePoint(FloatPoint) const; > + > + FloatPoint clientToLayoutViewportPoint(FloatPoint) const; I don't think we should introduce these new conversion functions. "layoutViewport" coordinates should just be client coordinates when visual viewports are enabled, and callers can already convert to them via documentToClientRect etc. > Source/WebCore/rendering/RenderLayer.cpp:4934 > + hitTestArea.intersect(absoluteLayoutViewportRect); > + } else > + hitTestArea.intersect(renderer().view().frameView().visibleContentRect(LegacyIOSDocumentVisibleRect)); Neither of these make much sense for the case when visualOverflowRect() was called; that's not in absolute coordinates.

Comment on attachment 314585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314585&action=review >> Source/WebCore/dom/TreeScope.cpp:307 >> + documentScope().updateLayout(); > > Why is this documentScope().updateLayout(); only in the visual viewport code path? This is because of the call to view->layoutViewportRect() below. We need the layout viewport updated before that. In the existing code path, we don't update layout until RenderView::hitTest. >> Source/WebCore/dom/TreeScope.cpp:308 >> + FloatPoint layoutViewportPoint = view->clientToLayoutViewportPoint(clientPoint); > > clientPoint should already be in layout viewport coordinates from the caller. The layout viewport incorporates page zoom, so this conversion is needed in order to treat the input point as being in the same space as the rect output by getBoundingClientRect(). For example, with an 800x600 window and a page zoom of 2, the layout viewport will still be 800x600, but an element positioned at the bottom right of the window (and at the bottom right of the layout viewport) will have a bounding client rect whose bottom right corner is (400, 300). >> Source/WebCore/rendering/RenderLayer.cpp:4934 >> + hitTestArea.intersect(renderer().view().frameView().visibleContentRect(LegacyIOSDocumentVisibleRect)); > > Neither of these make much sense for the case when visualOverflowRect() was called; that's not in absolute coordinates. I'll look into this some more to figure out how hitTestArea is being used in the RenderFlowThread case.

(In reply to Ali Juma from comment #7) > Comment on attachment 314585 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314585&action=review > > >> Source/WebCore/dom/TreeScope.cpp:307 > >> + documentScope().updateLayout(); > > > > Why is this documentScope().updateLayout(); only in the visual viewport code path? > > This is because of the call to view->layoutViewportRect() below. We need the > layout viewport updated before that. In the existing code path, we don't > update layout until RenderView::hitTest. OK. > >> Source/WebCore/dom/TreeScope.cpp:308 > >> + FloatPoint layoutViewportPoint = view->clientToLayoutViewportPoint(clientPoint); > > > > clientPoint should already be in layout viewport coordinates from the caller. > > The layout viewport incorporates page zoom, so this conversion is needed in > order to treat the input point as being in the same space as the rect output > by getBoundingClientRect(). Ah right. Long term, I'd like all "document" coordinates to be independent of page zoom (see recently-added FIXME in FrameView.h), so ideally visual/layoutViewportRects would not change with page zoom, but that's too invasive to do now. I was going to ask you if you'd tested with page zoom! > >> Source/WebCore/rendering/RenderLayer.cpp:4934 > >> + hitTestArea.intersect(renderer().view().frameView().visibleContentRect(LegacyIOSDocumentVisibleRect)); > > > > Neither of these make much sense for the case when visualOverflowRect() was called; that's not in absolute coordinates. > > I'll look into this some more to figure out how hitTestArea is being used in > the RenderFlowThread case.

Created attachment 314850 [details] Patch

Comment on attachment 314585 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314585&action=review >>>> Source/WebCore/rendering/RenderLayer.cpp:4934 >>>> + hitTestArea.intersect(renderer().view().frameView().visibleContentRect(LegacyIOSDocumentVisibleRect)); >>> >>> Neither of these make much sense for the case when visualOverflowRect() was called; that's not in absolute coordinates. >> >> I'll look into this some more to figure out how hitTestArea is being used in the RenderFlowThread case. > > It turns out that the visualOverflowRect() case never happens, since RenderLayer::hitTest is never called on a RenderFlowThread. RenderLayer::hitTest is the top-level entry point to hit-testing (RenderLayer::hitTestLayer is what gets called on each layer). It's mostly called on RenderViews, though from accessibility code it's possible to call it on the currently focused Element's RenderObject's layer. But RenderFlowThreads are never directly owned by Elements.

Comment on attachment 314850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314850&action=review See the "Coordinate systems:" comment in FrameView.h, and enhance it to add "layoutViewport" coordinates. > Source/WebCore/page/FrameView.cpp:4930 > + ASSERT(frame().settings().visualViewportEnabled()); > + rect.scale(frame().frameScaleFactor()); > + return rect; This seems wrong. layoutViewport -> absolute should be accounting for both the layout viewport origin, and the frame scale. > Source/WebCore/page/FrameView.cpp:4936 > + p.scale(frame().frameScaleFactor()); Ditto. > Source/WebCore/page/FrameView.cpp:4944 > + ASSERT(frame().settings().visualViewportEnabled()); > + p.scale(frame().pageZoomFactor()); > + p.moveBy(layoutViewportRect().location()); client to layout viewport should just be about pageZoom. > Source/WebCore/page/FrameView.h:480 > + FloatRect layoutViewportToAbsoluteRect(FloatRect) const; > + FloatPoint layoutViewportToAbsolutePoint(FloatPoint) const; > + > + FloatPoint clientToLayoutViewportPoint(FloatPoint) const; Please add a comment saying that "layoutViewport" coordinate differ from client coordinates because of page zoom.

Created attachment 314984 [details] Patch Addressed comments.

Comment on attachment 314984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314984&action=review > Source/WebCore/dom/TreeScope.cpp:309 > + FloatRect layoutViewportBounds(FloatPoint(), view->layoutViewportRect().size()); FloatPoint() can be { } > Source/WebCore/page/FrameView.cpp:4939 > + p.scale(frame().frameScaleFactor()); > + return p; This can use p.scaled() > Source/WebCore/page/FrameView.cpp:4946 > + p.scale(frame().pageZoomFactor()); > + return p; This can use p.scaled() > Source/WebCore/page/FrameView.h:455 > + // Relative to the layout viewport rect, affected by page zoom but not by page scale. Affected by scroll origin. I would say "Similar to client coordinates, but affected by page zoom (but not page scale)." I think the "Affected by scroll origin" adds confusion, so remove it. > Source/WebCore/rendering/RenderLayer.cpp:4931 > + const FrameView& frameView = renderer().view().frameView(); auto& frameView = > Source/WebCore/rendering/RenderLayer.cpp:4932 > + LayoutRect layoutViewportBounds(LayoutPoint(), frameView.layoutViewportRect().size()); LayoutPoint() -> { }

Created attachment 315019 [details] Patch for landing

Comment on attachment 315019 [details] Patch for landing Rejecting attachment 315019 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 315019, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/4094583

Created attachment 315022 [details] Patch for landing

Sorry about the previous patch, I was expecting the commit queue to fix up the OOPS lines. The newly-uploaded patch should be good to go.

Comment on attachment 315022 [details] Patch for landing Clearing flags on attachment: 315022 Committed r219342: <http://trac.webkit.org/changeset/219342>

All reviewed patches have been landed. Closing bug.