Bug 153137 – Implement document.elementsFromPoint

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Mike Taylor]

Spec: https://drafts.csswg.org/cssom-view/#dom-document-elementsfrompoint

Chrome, Opera and (soon) Firefox are shipping this. IE10+ and Edge have it prefixed.

[Frédéric Wang (:fredw)]

We have one W3C test for this at imported/w3c/web-platform-tests/cssom-view/elementsFromPoint.html

[Ali Juma]

Created attachment 311737 [details]
WIP

[Build Bot]

Comment on attachment 311737 [details]
WIP

Attachment 311737 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3855164

New failing tests:
imported/w3c/web-platform-tests/cssom-view/negativeMargins.html

[Build Bot]

Created attachment 311743 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311737 [details]
WIP

Attachment 311737 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3855172

New failing tests:
imported/w3c/web-platform-tests/cssom-view/negativeMargins.html

[Build Bot]

Created attachment 311744 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Simon Fraser (smfr)]

Comment on attachment 311737 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=311737&action=review

> Source/WebCore/dom/TreeScope.cpp:306
> +    point.scale(scaleFactor);
> +    point.moveBy(view->contentsScrollPosition());

This code will change with bug 172019. It should use a new function called clientToDocumentPoint() which is the mirror image of FrameView::documentToClientPoint().

> Source/WebCore/rendering/HitTestRequest.h:45
> +        ListBased = 1 << 13,

The "-based" doesn't fit the rest of the names. Maybe CollectsMultipleElements or MultipleResults or something.

> Source/WebCore/rendering/HitTestRequest.h:47
> +        PenetratingList = 1 << 14

This is a terrible name. Penetrating what? Maybe "inclusive"?

> Source/WebCore/rendering/HitTestRequest.h:71
> +    bool listBased() const { return m_requestType & ListBased; }
> +    bool penetratingList() const { return m_requestType & PenetratingList; }

These need renaming.

[Build Bot]

Comment on attachment 311737 [details]
WIP

Attachment 311737 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3855180

New failing tests:
imported/w3c/web-platform-tests/cssom-view/negativeMargins.html

[Build Bot]

Created attachment 311749 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Please make this a runtime-enabled feature.

[Build Bot]

Comment on attachment 311737 [details]
WIP

Attachment 311737 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3855432

New failing tests:
imported/w3c/web-platform-tests/cssom-view/negativeMargins.html
fast/dom/elementsFromPoint/elementsFromPoint-iframes.html
fast/dom/elementsFromPoint/elementsFromPoint-invalid-cases.html

[Build Bot]

Created attachment 311758 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Ali Juma]

Created attachment 313729 [details]
Patch

[Build Bot]

Comment on attachment 313729 [details]
Patch

Attachment 313729 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3985318

New failing tests:
fast/dom/elementFromPoint-scaled-scrolled.html

[Build Bot]

Created attachment 313734 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Simon Fraser (smfr)]

Comment on attachment 313729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313729&action=review

> Source/WebCore/ChangeLog:15
> +        This also makes elementFromPoint and elementsFromPoint treat their input
> +        point to be in client (layout viewport) coordinates.

Can you please do this change separately, via bug 172019?

[Ali Juma]

Created attachment 313866 [details]
Patch

[Ali Juma]

Comment on attachment 313866 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313866&action=review

> Source/WebCore/dom/DocumentOrShadowRoot.idl:34
> +    [EnabledAtRuntime=ElementsFromPoint] sequence<Element> elementsFromPoint(double x, double y);

This is using a runtime-enabled feature rather than a Setting because of bug 173737.

> LayoutTests/imported/w3c/web-platform-tests/cssom-view/elementsFromPoint-expected.txt:15
> +FAIL transformed element at x,y assert_array_equals: Should have returned a sequence with [pink, body, html] lengths differ, expected 3 got 0
> +FAIL no hit target at x,y assert_array_equals: Should have returned the sequence [body, html] lengths differ, expected 2 got 0

These two failures are because the test requires a larger window than what we use in layout tests; they pass when run interactively in a larger window. I'll get the test fixed.

> LayoutTests/imported/w3c/web-platform-tests/cssom-view/elementsFromPoint-shadowroot-expected.txt:3
> +FAIL elementsFromPoint on a shadow root should only return elements in that shadow tree assert_equals: shadowRoot.elementsFromPoint(50,15) expected "DIV#box12" but got "DIV#box12, DIV#host, BODY, HTML"

This is bug 170743.

> LayoutTests/imported/w3c/web-platform-tests/cssom-view/negativeMargins-expected.txt:4
> +FAIL cssom-view - elementFromPoint and elementsFromPoint dealing with negative margins 1 assert_array_equals: elementsFromPoint should get sequence [inner, outer, body, html] lengths differ, expected 4 got 5

The expected result here seems wrong to me -- it requires elementsFromPoint()[0] (which is 'inner') to be different from elementFromPoint() (which is 'outer'). The test was added in https://bugzilla.mozilla.org/show_bug.cgi?id=1164427, and reading through the comments (e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1164427#c27, https://bugzilla.mozilla.org/show_bug.cgi?id=1164427#c31) it sounds like they matched buggy behavior from Chrome under the mistaken impression that the spec required that behavior.

[Ali Juma]

(In reply to Simon Fraser (smfr) from comment #16)
> Comment on attachment 313729 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313729&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        This also makes elementFromPoint and elementsFromPoint treat their input
> > +        point to be in client (layout viewport) coordinates.
> 
> Can you please do this change separately, via bug 172019?

I've removed this part of the change, and will do it in bug 172019.

[Ali Juma]

Created attachment 315705 [details]
Patch

[Ali Juma]

Comment on attachment 315705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315705&action=review

> Source/WebCore/ChangeLog:14
> +        This is a runtime-enabled feature, enabled by default.

Just wanted to call out explicitly that this patch enables the feature by default. Let me know if there are other things that block enabling by default.

> LayoutTests/imported/w3c/ChangeLog:30
> +        * web-platform-tests/cssom-view/resources/iframe2.html: Added.

As recommended on webkit-dev (https://lists.webkit.org/pipermail/webkit-dev/2017-July/029265.html), I've created an upstream PR (https://github.com/w3c/web-platform-tests/pull/6568) for these tests in parallel with this review.

[Build Bot]

Comment on attachment 315705 [details]
Patch

Attachment 315705 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4137629

New failing tests:
security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html

[Build Bot]

Created attachment 315722 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Blaze Burg]

(In reply to Ryosuke Niwa from comment #10)
> Please make this a runtime-enabled feature.

This doesn't seem necessary. Is there a reason right now? What's the timeline/criteria for not making it optional? It's in a spec...

[Simon Fraser (smfr)]

Comment on attachment 315705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315705&action=review

Generally looks good but I'd like to see the suggested cleanup happen.

> Source/WebCore/dom/TreeScope.cpp:299
> +static bool absolutePointIfNotClipped(Document& document, const LayoutPoint& clientPoint, LayoutPoint& absolutePoint)

Cool kids would return an optional<LayoutPoint>

> Source/WebCore/dom/TreeScope.cpp:367
> +Vector<Element*> TreeScope::elementsFromPoint(double x, double y)

Can we name these clientX, clientY (and rename args to elementFromPoint too)?

> Source/WebCore/dom/TreeScope.h:92
>      WEBCORE_EXPORT Element* elementFromPoint(double x, double y);
> +    WEBCORE_EXPORT Vector<Element*> elementsFromPoint(double x, double y);

I think elementFromPoint() should return a RefPtr<Element>, and elementsFromPoint a Vector<RefPtr<Element>>.

> Source/WebCore/page/RuntimeEnabledFeatures.h:70
> +    void setElementsFromPointEnabled(bool isEnabled) { m_elementsFromPointEnabled = isEnabled; }
> +    bool elementsFromPointEnabled() const { return m_elementsFromPointEnabled; }

I don't think you need to add a runtime-enabled feature for this now.

> Source/WebCore/rendering/HitTestRequest.h:47
> +        // Collect a list of nodes instead of just one. Used for elementsFromPoint and rect-based tests.
> +        CollectMultipleElements = 1 << 13,
> +        // When using list-based testing, continue hit testing even after a hit has been found.
> +        InclusiveList = 1 << 14

I don't like InclusiveList. How about "IncludeAllElementsUnderPoint".

> Source/WebCore/rendering/HitTestRequest.h:70
> +    bool collectsMultipleElements() const { return m_requestType & CollectMultipleElements; }

Maybe "resultIsElementList"? You call this a "list-based test" elsewhere.

> Source/WebCore/rendering/HitTestRequest.h:71
> +    bool inclusiveList() const { return m_requestType & InclusiveList; }

This also needs renaming.

> Source/WebCore/rendering/HitTestResult.cpp:678
> +        return true;

Really hard to know what this boolean result means. Would be clearer with an enum. Does it mean "continue testing"?

> Source/WebCore/rendering/HitTestResult.cpp:690
>          return false;

Same with this one.

> Source/WebCore/rendering/HitTestResult.cpp:725
> +        for (NodeSet::const_iterator it = other.m_listBasedTestResult->begin(), last = other.m_listBasedTestResult->end(); it != last; ++it)

auto. Can't we use a modern C++ loop with m_listBasedTestResult?

> Source/WebCore/rendering/HitTestResult.h:134
> +    // Returns true if the test is a list-based test and we should continue testing.

Please make an enum for this.

> Source/WebCore/rendering/RenderView.cpp:186
> +    ASSERT(!location.isRectBasedTest() || request.collectsMultipleElements());

Is it really RenderView's job to make this assertion?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:356
> +            LayoutPoint localLayoutPoint(localPoint);

Why is this local variable necessary?

> Source/WebKit/Shared/WebPreferencesDefinitions.h:294
> +    macro(ElementsFromPointEnabled, elementsFromPointEnabled, Bool, bool, true, "ElementsFromPoint", "Enable document.elementsFromPoint support") \

No need for this.

> Source/WebKit/UIProcess/API/C/WKPreferences.cpp:1534
> +void WKPreferencesSetElementsFromPointEnabled(WKPreferencesRef preferencesRef, bool enabled)
> +{
> +    toImpl(preferencesRef)->setElementsFromPointEnabled(enabled);
> +}
> +
> +bool WKPreferencesGetElementsFromPointEnabled(WKPreferencesRef preferencesRef)
> +{
> +    return toImpl(preferencesRef)->elementsFromPointEnabled();
> +}

No need for this.

> Source/WebKit/UIProcess/API/C/WKPreferencesRefPrivate.h:510
> +// Defaults to true.
> +WK_EXPORT void WKPreferencesSetElementsFromPointEnabled(WKPreferencesRef, bool enabled);
> +WK_EXPORT bool WKPreferencesGetElementsFromPointEnabled(WKPreferencesRef);

No need for this.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3355
> +    RuntimeEnabledFeatures::sharedFeatures().setElementsFromPointEnabled(store.getBoolValueForKey(WebPreferencesKey::elementsFromPointEnabledKey()));

No need for this.

> Source/WebKitLegacy/mac/WebView/WebPreferenceKeysPrivate.h:192
> +#define WebKitElementsFromPointEnabledPreferenceKey @"WebKitElementsFromPointEnabled"

No need for this.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:680
> +        @YES, WebKitElementsFromPointEnabledPreferenceKey,

No need for this.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:3197
> +- (BOOL)elementsFromPointEnabled
> +{
> +    return [self _boolValueForKey:WebKitElementsFromPointEnabledPreferenceKey];
> +}
> +
> +- (void)setElementsFromPointEnabled:(BOOL)flag
> +{
> +    [self _setBoolValue:flag forKey:WebKitElementsFromPointEnabledPreferenceKey];
> +}

No need for this.

> Source/WebKitLegacy/mac/WebView/WebPreferencesPrivate.h:566
> +- (void)setElementsFromPointEnabled:(BOOL)flag;
> +- (BOOL)elementsFromPointEnabled;

No need for this.

> Source/WebKitLegacy/mac/WebView/WebView.mm:2998
> +    RuntimeEnabledFeatures::sharedFeatures().setElementsFromPointEnabled([preferences elementsFromPointEnabled]);

No need for this.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:968
> +    [preferences setElementsFromPointEnabled:YES];

No need for this.

> Tools/WebKitTestRunner/TestController.cpp:669
> +    WKPreferencesSetElementsFromPointEnabled(preferences, true);

No need for this.

[Radar WebKit Bug Importer]

<rdar://problem/33407048>

[Ali Juma]

Created attachment 316006 [details]
Patch

[Ali Juma]

Comment on attachment 315705 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315705&action=review

>> Source/WebCore/dom/TreeScope.cpp:299
>> +static bool absolutePointIfNotClipped(Document& document, const LayoutPoint& clientPoint, LayoutPoint& absolutePoint)
> 
> Cool kids would return an optional<LayoutPoint>

Done.

>> Source/WebCore/dom/TreeScope.cpp:367
>> +Vector<Element*> TreeScope::elementsFromPoint(double x, double y)
> 
> Can we name these clientX, clientY (and rename args to elementFromPoint too)?

Done.

>> Source/WebCore/dom/TreeScope.h:92
>> +    WEBCORE_EXPORT Vector<Element*> elementsFromPoint(double x, double y);
> 
> I think elementFromPoint() should return a RefPtr<Element>, and elementsFromPoint a Vector<RefPtr<Element>>.

Done.

>> Source/WebCore/page/RuntimeEnabledFeatures.h:70
>> +    bool elementsFromPointEnabled() const { return m_elementsFromPointEnabled; }
> 
> I don't think you need to add a runtime-enabled feature for this now.

Removed.

>> Source/WebCore/rendering/HitTestRequest.h:47
>> +        InclusiveList = 1 << 14
> 
> I don't like InclusiveList. How about "IncludeAllElementsUnderPoint".

Done.

>> Source/WebCore/rendering/HitTestRequest.h:70
>> +    bool collectsMultipleElements() const { return m_requestType & CollectMultipleElements; }
> 
> Maybe "resultIsElementList"? You call this a "list-based test" elsewhere.

Done.

>> Source/WebCore/rendering/HitTestRequest.h:71
>> +    bool inclusiveList() const { return m_requestType & InclusiveList; }
> 
> This also needs renaming.

Done.

>> Source/WebCore/rendering/HitTestResult.cpp:678
>> +        return true;
> 
> Really hard to know what this boolean result means. Would be clearer with an enum. Does it mean "continue testing"?

Changed to an enum.

>> Source/WebCore/rendering/HitTestResult.cpp:725
>> +        for (NodeSet::const_iterator it = other.m_listBasedTestResult->begin(), last = other.m_listBasedTestResult->end(); it != last; ++it)
> 
> auto. Can't we use a modern C++ loop with m_listBasedTestResult?

Done.

>> Source/WebCore/rendering/RenderView.cpp:186
>> +    ASSERT(!location.isRectBasedTest() || request.collectsMultipleElements());
> 
> Is it really RenderView's job to make this assertion?

Removed. The equivalent assertion in HitTestResult::addNodeToListBasedTestResult provides the same coverage.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:356
>> +            LayoutPoint localLayoutPoint(localPoint);
> 
> Why is this local variable necessary?

Removed.

[Simon Fraser (smfr)]

Comment on attachment 316006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316006&action=review

> Source/WebCore/dom/TreeScope.cpp:334
> +    std::optional<LayoutPoint> absolutePoint = absolutePointIfNotClipped(documentScope(), clientPoint);

auto absolutePoint

> Source/WebCore/dom/TreeScope.cpp:338
> +    HitTestResult result(*absolutePoint);

absolutePoint.value()

> Source/WebCore/dom/TreeScope.cpp:385
> +    HitTestResult result(*absolutePoint);

absolutePoint.value()

> Source/WebCore/rendering/HitTestResult.h:48
> +    enum HitTestProgress {
> +        StopHitTesting,
> +        ContinueHitTesting
> +    };

enum class HitTestProgress { Stop, Continue };

OK to move this outside the class.

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:186
> +            if (result.addNodeToListBasedTestResult(child->node(), request, localPoint) == HitTestResult::StopHitTesting)
> +                return true;

At some point we should convert nodeAtFloatPoint to return an enum as well.

[Build Bot]

Comment on attachment 316006 [details]
Patch

Attachment 316006 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4158171

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 316049 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Ali Juma]

Created attachment 316099 [details]
Patch for landing

[Ali Juma]

Comment on attachment 316099 [details]
Patch for landing

Marking cq- since the new web platform tests in this patch need to land upstream before this patch lands.

[Ali Juma]

Created attachment 316304 [details]
Patch for landing

[Ali Juma]

Comment on attachment 316304 [details]
Patch for landing

This is ready to land now (the web platform tests have landed upstream).

[Simon Fraser (smfr)]

The test will probably fail after https://trac.webkit.org/changeset/219829/webkit. You should land failing results, since I'll probably revert https://trac.webkit.org/changeset/219829/webkit again soon (after it's been cherry-picked for a branch).

[Ali Juma]

(In reply to Simon Fraser (smfr) from comment #36)
> The test will probably fail after
> https://trac.webkit.org/changeset/219829/webkit. You should land failing
> results, since I'll probably revert
> https://trac.webkit.org/changeset/219829/webkit again soon (after it's been
> cherry-picked for a branch).

I re-ran the tests after that change, and it turns out they still pass (the implementation of elementsFromPoint doesn't use documentToClientOffset; instead, it uses clientToLayoutViewportPoint and layoutViewportToAbsolutePoint).

[Simon Fraser (smfr)]

That means that I failed to change those methods (I knew there was a reason I didn't want functions that didn't go through documentToClientOffset() :( )

[WebKit Commit Bot]

Comment on attachment 316304 [details]
Patch for landing

Clearing flags on attachment: 316304

Committed r219961: <http://trac.webkit.org/changeset/219961>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.