|
Description
Mike Taylor
2016-01-15 10:44:55 PST
We have one W3C test for this at imported/w3c/web-platform-tests/cssom-view/elementsFromPoint.html Created attachment 311737 [details]
WIP
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 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
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 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
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. 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 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
Please make this a runtime-enabled feature. 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 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
Created attachment 313729 [details]
Patch
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 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
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? Created attachment 313866 [details]
Patch
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. (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. Created attachment 315705 [details]
Patch
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. 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 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
(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... 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. Created attachment 316006 [details]
Patch
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. 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. 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 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
Created attachment 316099 [details]
Patch for landing
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.
Created attachment 316304 [details]
Patch for landing
Comment on attachment 316304 [details]
Patch for landing
This is ready to land now (the web platform tests have landed upstream).
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). (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). 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() :( ) Comment on attachment 316304 [details] Patch for landing Clearing flags on attachment: 316304 Committed r219961: <http://trac.webkit.org/changeset/219961> All reviewed patches have been landed. Closing bug. |