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
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.
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
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 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 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 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.
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.
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
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.
(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() :( )
2017-06-01 11:48 PDT, Ali Juma
2017-06-01 12:56 PDT, Build Bot
2017-06-01 13:03 PDT, Build Bot
2017-06-01 13:17 PDT, Build Bot
2017-06-01 14:05 PDT, Build Bot
2017-06-23 11:10 PDT, Ali Juma
2017-06-23 12:47 PDT, Build Bot
2017-06-26 13:42 PDT, Ali Juma
2017-07-17 13:42 PDT, Ali Juma
2017-07-17 15:28 PDT, Build Bot
2017-07-20 11:34 PDT, Ali Juma
2017-07-20 17:43 PDT, Build Bot
2017-07-21 10:31 PDT, Ali Juma
2017-07-24 11:54 PDT, Ali Juma