WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
195696
Web Automation: elements larger than the viewport have incorrect in-view center point
https://bugs.webkit.org/show_bug.cgi?id=195696
Summary
Web Automation: elements larger than the viewport have incorrect in-view cent...
Blaze Burg
Reported
2019-03-13 13:25:51 PDT
This seems to be an omission in the specification. While it does mention that the in-view center point (IVCP) must be within the viewport, the algorithm never intersects the element bounding box with the viewport rect. Spec issue filed here:
https://github.com/w3c/webdriver/issues/1402
Attachments
WIP patch
(12.85 KB, patch)
2019-03-14 13:54 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
WIP v2
(31.13 KB, patch)
2019-04-15 17:14 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
Patch
(35.35 KB, patch)
2019-05-06 17:01 PDT
,
Blaze Burg
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews211 for win-future
(13.67 MB, application/zip)
2019-05-06 23:31 PDT
,
EWS Watchlist
no flags
Details
Patch
(37.90 KB, patch)
2019-05-10 13:06 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(38.12 KB, patch)
2019-05-11 13:14 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from ews214 for win-future
(13.78 MB, application/zip)
2019-05-12 09:48 PDT
,
EWS Watchlist
no flags
Details
Patch
(38.16 KB, patch)
2019-05-13 19:31 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(36.98 KB, patch)
2019-05-14 17:36 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(38.78 KB, patch)
2019-05-15 00:50 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Blaze Burg
Comment 1
2019-03-13 13:26:11 PDT
<
rdar://problem/48737122
>
Blaze Burg
Comment 2
2019-03-14 13:54:04 PDT
Created
attachment 364682
[details]
WIP patch This is an attempt to fix some hit testing issues I ran into in more complicated content such as scrolling. I need to run full test suites to verify this doesn't regress anything. There are also other unexpected 'element not interactable' errors I've run into, but these may be unrelated to the bugs fixed in this patch.
Simon Fraser (smfr)
Comment 3
2019-03-14 14:06:17 PDT
Comment on
attachment 364682
[details]
WIP patch View in context:
https://bugs.webkit.org/attachment.cgi?id=364682&action=review
> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1550 > + WebCore::FloatRect visualViewportRect = page.customFixedPositionRect(); // Content coordinates.
This is wrong; customFixedPositionRect is layoutViewport, not visualViewport.
> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1551 > + visualViewportRect.setLocation(WebCore::FloatPoint::zero());
visualViewportRect.setLocation({ }) should work.
> Source/WebKit/UIProcess/Automation/ios/WebAutomationSessionIOS.mm:180 > +static TextStream& operator<<(TextStream& ts, const TouchInteraction& interaction)
TouchInteraction is an enum so you can pass by value.
> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:480 > +// Returns the in-view center point of an element in client coordinates.
At some future point "client" coordinates might be layoutViewport coords, which might allow the layout viewport center to not be visible.
> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:494 > + FloatRect elementRect = FloatRect(firstElementRect->x(), firstElementRect->y(), firstElementRect->width(), firstElementRect->height());
= { firstElementRect->x(), firstElementRect->y(), firstElementRect->width(), firstElementRect->height() } We should probably give DOMRectReadOnly a FloatRect operator.
> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:495 > + FloatRect visibleElementRect = intersection(viewportRect, elementRect);
You're assuming element.getClientRects() are in mainFrameView.visualViewportRect() coordinates, which is true now but may not always be.
Blaze Burg
Comment 4
2019-04-15 17:14:15 PDT
Created
attachment 367478
[details]
WIP v2
Blaze Burg
Comment 5
2019-04-15 17:16:52 PDT
(In reply to Simon Fraser (smfr) from
comment #3
)
> Comment on
attachment 364682
[details]
> WIP patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=364682&action=review
> > > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1550 > > + WebCore::FloatRect visualViewportRect = page.customFixedPositionRect(); // Content coordinates. > > This is wrong; customFixedPositionRect is layoutViewport, not visualViewport.
OK
> > > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1551 > > + visualViewportRect.setLocation(WebCore::FloatPoint::zero()); > > visualViewportRect.setLocation({ }) should work.
OK
> > > Source/WebKit/UIProcess/Automation/ios/WebAutomationSessionIOS.mm:180 > > +static TextStream& operator<<(TextStream& ts, const TouchInteraction& interaction) > > TouchInteraction is an enum so you can pass by value.
OK
> > > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:480 > > +// Returns the in-view center point of an element in client coordinates. > > At some future point "client" coordinates might be layoutViewport coords, > which might allow the layout viewport center to not be visible.
See question below.
> > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:494 > > + FloatRect elementRect = FloatRect(firstElementRect->x(), firstElementRect->y(), firstElementRect->width(), firstElementRect->height()); > > = { firstElementRect->x(), firstElementRect->y(), firstElementRect->width(), > firstElementRect->height() } > > We should probably give DOMRectReadOnly a FloatRect operator.
¯\_(ツ)_/¯
> > > Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:495 > > + FloatRect visibleElementRect = intersection(viewportRect, elementRect); > > You're assuming element.getClientRects() are in > mainFrameView.visualViewportRect() coordinates, which is true now but may > not always be.
Should code for this situation be guarded by this, which I found in TreeScope.cpp?
> if (settings.visualViewportEnabled() && settings.clientCoordinatesRelativeToLayoutViewport()) {
I'll add that in when I'm able to test it, but for the sake of getting this landed sometime, I might make a separate bug.
EWS Watchlist
Comment 6
2019-04-15 17:17:41 PDT
Comment hidden (obsolete)
Attachment 367478
[details]
did not pass style-queue: ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:586: elementBounds_frame_client is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:587: elementBounds_mainframe_root is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:588: elementBounds_mainframe_document is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:589: elementBounds_mainframe_viewport is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:629: inViewCenterPoint_mainframe_root is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:630: inViewCenterPoint_mainframe_document is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:631: inViewCenterPoint_mainframe_viewport is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 7 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Blaze Burg
Comment 7
2019-04-15 17:18:58 PDT
This new patch is not ready to land as I'm unable to test it on iOS at this time. On Mac, it only causes one WPT test regression on Mac (which is actually a separate, newly-revealed bug). For iOS, I need to substitute contentsToRootView with the manual frame client->root view conversion code. This is currently in a comment block so I don't forget the gist of it. Comments and feedback welcome. I'm hoping to put out a final patch tomorrow.
Blaze Burg
Comment 8
2019-05-06 17:01:36 PDT
Created
attachment 369207
[details]
Patch
EWS Watchlist
Comment 9
2019-05-06 23:31:43 PDT
Comment hidden (obsolete)
Comment on
attachment 369207
[details]
Patch
Attachment 369207
[details]
did not pass win-ews (win): Output:
https://webkit-queues.webkit.org/results/12120114
New failing tests: legacy-animation-engine/compositing/reflections/load-video-in-reflection.html
EWS Watchlist
Comment 10
2019-05-06 23:31:45 PDT
Comment hidden (obsolete)
Created
attachment 369241
[details]
Archive of layout-test-results from ews211 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Devin Rousso
Comment 11
2019-05-10 13:06:05 PDT
Created
attachment 369584
[details]
Patch Rebase, plus some minor style changes
Simon Fraser (smfr)
Comment 12
2019-05-10 18:04:03 PDT
Comment on
attachment 369584
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=369584&action=review
> Source/WebCore/platform/ScrollView.cpp:850 > + return viewToContents(IntPoint(point));
roundedIntPoint
> Source/WebCore/platform/ScrollView.cpp:858 > + return contentsToView(IntPoint(point));
roundedIntPoint
> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:554 > + auto& frame = frameView->frame(); > + clientRect.scale(frame.pageZoomFactor() * frame.frameScaleFactor()); > + clientRect.moveBy(frameView->contentsScrollPosition()); > + return clientRect;
Maybe add a comment to say why contentsToRootView doesn't work for you in this case.
> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:565 > + auto& frame = frameView->frame(); > + clientPoint.scale(frame.pageZoomFactor() * frame.frameScaleFactor()); > + clientPoint.moveBy(frameView->contentsScrollPosition()); > + return clientPoint;
Ditto.
Devin Rousso
Comment 13
2019-05-11 13:14:17 PDT
Created
attachment 369659
[details]
Patch
EWS Watchlist
Comment 14
2019-05-12 09:48:53 PDT
Comment hidden (obsolete)
Comment on
attachment 369659
[details]
Patch
Attachment 369659
[details]
did not pass win-ews (win): Output:
https://webkit-queues.webkit.org/results/12169065
New failing tests: http/tests/misc/repeat-open-cancel.html
EWS Watchlist
Comment 15
2019-05-12 09:48:56 PDT
Comment hidden (obsolete)
Created
attachment 369676
[details]
Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Devin Rousso
Comment 16
2019-05-13 17:02:09 PDT
Comment on
attachment 369584
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=369584&action=review
>> Source/WebCore/platform/ScrollView.cpp:850 >> + return viewToContents(IntPoint(point)); > > roundedIntPoint
This actually causes WebDriver WPT failures, specifically all of the odd numbered wpt/tests/element_click/center_point.py::test_css_pixel_rounding tests.
Devin Rousso
Comment 17
2019-05-13 19:31:13 PDT
Created
attachment 369812
[details]
Patch
Darin Adler
Comment 18
2019-05-13 19:58:51 PDT
Comment on
attachment 369812
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=369812&action=review
> Source/WebCore/dom/DOMRectReadOnly.h:55 > + operator FloatRect() const { return FloatRect(m_x, m_y, m_width, m_height); }
Not sure it’s a good idea to have a lossy operation that converts doubles to floats be implicit; typically we only want lossless operations to be implicit and lossy ones to have names
Devin Rousso
Comment 19
2019-05-14 17:36:50 PDT
Created
attachment 369913
[details]
Patch
EWS Watchlist
Comment 20
2019-05-14 17:39:09 PDT
Comment hidden (obsolete)
Attachment 369913
[details]
did not pass style-queue: ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:609: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Devin Rousso
Comment 21
2019-05-15 00:50:15 PDT
Created
attachment 369936
[details]
Patch
EWS Watchlist
Comment 22
2019-05-15 00:52:47 PDT
Comment hidden (obsolete)
Attachment 369936
[details]
did not pass style-queue: ERROR: Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:609: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 23
2019-05-15 01:37:08 PDT
Comment on
attachment 369936
[details]
Patch Clearing flags on attachment: 369936 Committed
r245320
: <
https://trac.webkit.org/changeset/245320
>
WebKit Commit Bot
Comment 24
2019-05-15 01:37:10 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug