Switch to WebDriver conformant interactability checks for clickElement

RESOLVED FIXED in Firefox 58

Status

P2
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: ato, Assigned: whimboo)

Tracking

(Depends on: 1 bug, Blocks: 4 bugs)

Version 3
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 27 obsolete attachments)

1.65 KB, text/html
Details
59 bytes, text/x-review-board-request
ato
: review+
Details | Review
(Reporter)

Description

2 years ago
We currently use the isDisplayed atom from Selenium when determining if we can interact with the element or not.  We should switch to the WebDriver conformant interactability algorithm that is currently guarded behind the `specificationLevel` capability.
(Reporter)

Updated

2 years ago
Blocks: 721859
(Reporter)

Comment 1

2 years ago
To enable the interactability check you currently have to set the `specificationLevel` capability to 1.  When we make the switch, we should remove the isDisplayed atom from Marionette and delete this capability.
(Reporter)

Updated

2 years ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
are we sure that we are not breaking any Selenium tests? There is an agreement within the working group we would use the atom until we can guarantee the same level of support.
Flags: needinfo?(ato)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 12

2 years ago
(In reply to David Burns :automatedtester from comment #7)
> are we sure that we are not breaking any Selenium tests? There is an
> agreement within the working group we would use the atom until we can
> guarantee the same level of support.

We don’t run the Selenium test suite, so we’re not sure about that until we try it out.

I found that the Mn tests I had to change were mostly about incorrect test assumptions, and not about whether the test assumed an element was visible when this patch thought it was not.  In other words, the poitner-interactable technique runs all existing atom-visibility tests plus a superset of new edge cases the atom does not handle.

If you look at the tests in test_click.py, the pointer-interactability approach generally catches more edge cases of elements not being visible/interactable, but there are no regressions amongst our tests that show it is in any way regressing existing behaviour.

I would expect failures in Selenium tests to be cases the atom does not currently support (such as pointer-events: none, transforms, partially visible elements, opacity, &c.).
Flags: needinfo?(ato)
(Reporter)

Comment 13

2 years ago
I should add that there are some test failures in the latest try run I’m working on resolving.  These failures are related to XUL and chrome scope handling, as well as sending keys to elements, which uses the same checks.
(Reporter)

Updated

a year ago
Attachment #8817846 - Flags: review?(dburns)
(Reporter)

Updated

a year ago
Attachment #8817847 - Flags: review?(dburns)
Attachment #8817848 - Flags: review?(dburns)
(Reporter)

Comment 14

a year ago
(In reply to David Burns :automatedtester from comment #7)

> There is an agreement within the working group we would use the atom
> until we can guarantee the same level of support.

As far as I’m aware there is no contention ni the WG around Element
Click.  It was only Element Get Text (and Element Is Displayed) that an
agreement was struck to keep using the Selenium atoms for the forseeable
future, until cross-browser web platform APIs become available.

The click command uses the paint tree/hit testing (through
elementsFromPoint) to determine whether elements can be interacted with,
which appears to me as a far more reliable approach than using the
displayedness atom.  I guess that this also cannot be changed now that
the specification is in CR.

The specification-conforming Element Click command, introduced in
https://bugzilla.mozilla.org/show_bug.cgi?id=1333014, implements
https://w3c.github.io/webdriver/webdriver-spec.html#dfn-element-click
exactly.
(Reporter)

Updated

a year ago
Priority: -- → P1
(Reporter)

Updated

a year ago
Blocks: 1354203
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 20

a year ago
Re-did the patches removing the Selenium element click path from Marionette, favouring the WebDriver-conforming element interaction algorithm added in https://bugzilla.mozilla.org/show_bug.cgi?id=1333014.
I have asked Google Web Testing to do a "try" run on their infra with specificationLevel: 1 in capabilities and to let me know what breaks.
Google have hit an issue and have gratiously recreated the issue for us.

https://gist.github.com/juangj/9d5ed2c12fbb789eaed7e9aa006d054f
(Assignee)

Comment 23

a year ago
(In reply to David Burns :automatedtester from comment #22)
> Google have hit an issue and have gratiously recreated the issue for us.
> 
> https://gist.github.com/juangj/9d5ed2c12fbb789eaed7e9aa006d054f

They tested with Firefox 52 and most likely hit https://github.com/mozilla/geckodriver/issues/285 or bug 1337743. The latter is part of Aurora now, but hasn't been uplifted to 53. Andreas, will you try to uplift it?
Flags: needinfo?(ato)
(Reporter)

Comment 24

a year ago
(In reply to Henrik Skupin (:whimboo) from comment #23)
> They tested with Firefox 52 and most likely hit
> https://github.com/mozilla/geckodriver/issues/285 or bug 1337743. The latter
> is part of Aurora now, but hasn't been uplifted to 53. Andreas, will you try
> to uplift it?

There are some pretty substantive conflicts trying to cherrypick those patches on to beta, so I guess something must be missing, but not sure what.
Flags: needinfo?(ato)
(Reporter)

Comment 25

a year ago
(In reply to David Burns :automatedtester from comment #22)
> Google have hit an issue and have gratiously recreated the issue for us.
> 
> https://gist.github.com/juangj/9d5ed2c12fbb789eaed7e9aa006d054f

They appear to have been using 52.0, which doesn’t have https://bugzilla.mozilla.org/show_bug.cgi?id=1333014, so whatever issue they ran into must be a bug in the Selenium atom or our usage of it.

Could you ask them to retry with Firefox Nightly?
I am a bit confused, I thought you said having specificationLevel: 1 would test this new code path.

Could we get proper steps to replicate so to minimize wasting time retesting it to find it was wrong again.
Flags: needinfo?(ato)
(Reporter)

Comment 27

a year ago
So looking at the test attached in the gist above:

> elem = driver.find_element_by_id('parent')
> # This normally clicks the child element. With specificationLevel: 1, it throws.
> elem.click()

According to the specification this is supposed to return an ‘element click intercepted’ error because #child obscures #parent.  Because #child has a higher paint order than #parent, it is impossible for the click to reach the requested element; the click would hit #child instead.

As I mentioned in comment #12, this is one of the expected changes from implementing WebDriver.
(Reporter)

Comment 28

a year ago
(In reply to David Burns :automatedtester from comment #26)
> I am a bit confused, I thought you said having specificationLevel: 1 would
> test this new code path.

The element click implementation in 52.0 final isn’t spec conforming.  If you consider the tracking information on https://bugzilla.mozilla.org/show_bug.cgi?id=1333014, the patch is only available in Firefox 52 ESR (not 52.0) onwards:

> status-firefox53: 	fixed
> status-firefox54: 	fixed
> status-firefox55: 	fixed
> status-firefox-esr52: 	fixed 

> Could we get proper steps to replicate so to minimize wasting time retesting
> it to find it was wrong again.

As I said in comment #27, I don’t think the test from https://gist.github.com/juangj/9d5ed2c12fbb789eaed7e9aa006d054f is testing expected WebDriver behaviour.  The ‘element click intercepted’ error was added so that users would be notified when the click would not hit the desired element.
Flags: needinfo?(ato)
Priority: P1 → P2
(Reporter)

Comment 29

a year ago
https://github.com/mozilla/geckodriver/issues/615, which was filed on geckodriver a week ago, is blocked by this.  Switching over to the WebDriver spec-conforming Element Click command implementation would skip the element enabled check.
(Reporter)

Updated

a year ago
Duplicate of this bug: 1355546
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8855401 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 66

a year ago
mozreview-review
Comment on attachment 8859187 [details]
Bug 1321516 - Reject interaction.flushEventLoop if window is discarded

https://reviewboard.mozilla.org/r/131234/#review135188

::: testing/marionette/interaction.js:260
(Diff revision 3)
> +
>    return new Promise((resolve, reject) => {
> +    if (win.closed) {
> +      reject();
> +      return;
> +    }

Please note that I think that every handling of page load status and window existence is wrong in this module. It should be all done in listener.js, or for the window case in proxy.js. For the latter please see bug 1223277. If its priority has to be bumped please do so.

Also as mentioned on bug 1335778 I might completely remove the `flushEventLoop` method from this file.
(Reporter)

Updated

a year ago
Depends on: 1359004
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8817847 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8817848 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8859183 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8859184 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8859185 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8859187 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8859188 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8855400 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8859186 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8859189 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8817846 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Depends on: 321516
(Reporter)

Updated

a year ago
Depends on: 1359043
No longer depends on: 321516
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8860108 - Flags: review?(hskupin)
(Reporter)

Updated

a year ago
Depends on: 1359050
(Reporter)

Updated

a year ago
Attachment #8860108 - Flags: review?(hskupin)
(Reporter)

Updated

a year ago
Depends on: 1359053
(Reporter)

Updated

a year ago
Depends on: 1359054
(Reporter)

Updated

a year ago
Depends on: 1359059
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Depends on: 1359079
(Reporter)

Updated

a year ago
Attachment #8860108 - Flags: review?(hskupin)
(Assignee)

Comment 87

a year ago
mozreview-review
Comment on attachment 8860108 [details]
Bug 1321516 - Remove old Marionette click implementation

https://reviewboard.mozilla.org/r/132120/#review136072

What about the TestLegacyClick class in test_click()? Shouldn't all those tests be moved to the TestClick class?
(Reporter)

Comment 88

a year ago
(In reply to Henrik Skupin (:whimboo) from comment #87)
> What about the TestLegacyClick class in test_click()? Shouldn't all those
> tests be moved to the TestClick class?

TestClick extends TestLegacyClick, and so all the test cases that are in the file are being kept.  The difference is that we don’t gate running either class on whether specificationLevel is set.

(Sorry for accidentally setting r? on this through mozreview.  I want to land all the dependents of this patch before reworking this patch series and requesting review on it.)
(Reporter)

Updated

a year ago
Blocks: 1275273
(Assignee)

Comment 89

a year ago
So I talked with Andreas today in regards of anything we can do to make some progress on this move. The following steps came out of the meeting:

1) Run all Marionette tests with the webdriver conformant clickElement method and check what's broken across platforms
2) Fix broken behavior / tests to be able to run in both modes
2.1) Add a separate Marionette-e10s job which runs all tests with webdriver conformant clicks
3) Add a capability for geckodriver under moz:firefoxOptions to specify the type of clickElement to use
4) Ask Selenium testers to run their tests with webdriver conformant clicks

When checking this bug now I'm a bit worried that this might not be that easy given the long list of commits on this bug. Andreas, mind having a quick look what's already landed and if there is something we still need? Or should I simply try step 1) so we know where we are at the moment?
Flags: needinfo?(ato)
(Reporter)

Comment 90

a year ago
(In reply to Henrik Skupin (:whimboo) from comment #89)

> When checking this bug now I'm a bit worried that this might not
> be that easy given the long list of commits on this bug. Andreas,
> mind having a quick look what's already landed and if there is
> something we still need? Or should I simply try step 1) so we know
> where we are at the moment?

I see what you mean.

A quick glance through the patches associated with this bug tells me
that they are fixes for ensuring the WebDriver conforming element
clicks to work against the body of tests we have for Marionette in
m-c.  I think some of these can probably be rebased and be landed
separately with the existing element click implementation, but I
think some of them are correcting test expectations that are now
incorrect from having a better click.

To reiterate a point I made earlier in this bug as to make
it absolutely clear: the WebDriver conforming Element Click
implementation in Marionette _is not_ 100% backwards compatible with
the way we currently do clicks, and this is OK.

Tomorrow I will spend some time going through these patches to
figure out which can be landed separately, and which of them make
changes that we need to fix when we make the switch to the new click
implementation.

Also thanks to Henrik for picking up this work where I left off.  We
are not doing ourselves a favour by maintaining to Element Click
implementations.

(Will clear the needinfo when I’m done.)
(Reporter)

Updated

a year ago
Depends on: 1387447
(Reporter)

Updated

a year ago
Depends on: 1387452
(Reporter)

Updated

a year ago
Depends on: 1387457
Flags: needinfo?(ato)
(Reporter)

Updated

a year ago
Depends on: 1387461
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8860978 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8860979 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8860980 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8860981 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8860982 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8860983 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8860984 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8860985 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8860986 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8860108 - Flags: review?(hskupin)
(Reporter)

Updated

a year ago
Depends on: 1387470
(Reporter)

Comment 94

a year ago
I have now gone through all the patches that were associated with
this patch and either verified that they are already on central or
submitted new patches for them.

I have also uploaded a three-part changeset to this bug which can
be rebased when all the dependencies of this bug has landed.  This
changeset will effectively remove the old WebDriver:ElementClick
implementation from Marionette and replace it with the WebDriver
conforming implementation.

For the reasons that whimboo gave, we should obviously not
land that until we have gathered some more data.  I filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1387470 to add a new
extension capability to geckodriver so that external consumers
can run their tests against the new implementation, because it
is currently not possible to pass {specificationLevel: 1} as a
capability to geckodriver.
(Assignee)

Comment 95

a year ago
mozreview-review
Comment on attachment 8860108 [details]
Bug 1321516 - Remove old Marionette click implementation

https://reviewboard.mozilla.org/r/132120/#review170734

::: testing/marionette/interaction.js:138
(Diff revision 11)
>   * @throws {ElementNotAccessibleError}
>   *     If <var>strict</var> is true and element is not accessible.
>   * @throws {InvalidElementStateError}
>   *     If <var>el</var> is not enabled.
>   */
> -interaction.clickElement = function* (
> +interaction.clickElement = function* (el, {strict = false} = {}) {

I find it awkward to do the dict expansion in the function's parameter list. Are we doing that in our code base somewhere else? I have never seen that, and wouldn't propose to do it at all. Because once we put more options in here, it's going to become a mess.
Attachment #8860108 - Flags: review+
(Assignee)

Comment 96

a year ago
Andreas, would you mind triggering a new try job across platforms so we can see how Marionette unit tests behave? If we are close to be green I would not spend my time in setting up a separate job for conforming clicks, but just get started to add the capability to geckodriver for wider exposure.
Flags: needinfo?(ato)
(Assignee)

Comment 97

a year ago
Given that Andreas is out the next days I took the opportunity to just trigger a try build myself via mozreview. Lets see how it works.
Flags: needinfo?(ato)
(Assignee)

Comment 98

a year ago
mozreview-review
Comment on attachment 8860108 [details]
Bug 1321516 - Remove old Marionette click implementation

https://reviewboard.mozilla.org/r/132120/#review172132

::: testing/marionette/listener.js:1371
(Diff revision 11)
>      loadListener.navigate(() => {
>        return interaction.clickElement(
>            seenEls.get(id, curContainer),
> -          capabilities.get("moz:accessibilityChecks"),
> +          {strict: capabilities.get("moz:accessibilityChecks")});
> -          capabilities.get("specificationLevel") >= 1
>        );

This causes a syntax failure:

[task 2017-08-10T07:29:20.042701Z] 07:29:20     INFO -  JavaScript error: chrome://marionette/content/listener.js, line 1371: SyntaxError: expected expression, got ')'
Attachment #8860108 - Flags: review+ → review-
(Reporter)

Comment 99

a year ago
I didn’t ask for review on these patches: they are are a prototype
that will only work if you apply all the patches from the dependent
bugs of this patch, so I didn’t check if they actually work.  I
put it up here for reference as to what needs to be done.
(Assignee)

Comment 100

a year ago
Andreas, all of the dependent bugs are closed. So what else is blocking us from testing it? If you can fix the JS error we could push another try build for checks. Thanks.
(Reporter)

Comment 101

a year ago
(In reply to Henrik Skupin (:whimboo) from comment #100)
> Andreas, all of the dependent bugs are closed. So what else is blocking us
> from testing it? If you can fix the JS error we could push another try build
> for checks. Thanks.

I will get around to that, I’ve only just come back from PTO.  Please give me
the time!
(Reporter)

Updated

a year ago
Depends on: 1374283
(Assignee)

Comment 102

a year ago
(In reply to Andreas Tolfsen ‹:ato› from comment #101)
> I will get around to that, I’ve only just come back from PTO.  Please give me
> the time!

Andreas, this patch really needs a very tiny change only, see comment 98. With that I could trigger a full try build to check how stable we are with specificationLevel 1. If it looks good I would go ahead and work on bug 1387470. Thanks.
Flags: needinfo?(ato)
(Reporter)

Comment 103

a year ago
(In reply to Henrik Skupin (:whimboo) from comment #102)

> Andreas, this patch really needs a very tiny change only, see
> comment 98.  With that I could trigger a full try build to check
> how stable we are with specificationLevel 1. If it looks good I
> would go ahead and work on bug 1387470. Thanks.

I’m sorry but I don’t see why
https://bugzilla.mozilla.org/show_bug.cgi?id=1387470 is dependent
on this.  The patches in this bug _removes_ the Selenium click
implementation entirely from Marionette, whereas investigation into
whether the WebDriver conforming click implementation is sound must
happen _before_ this lands.
No longer depends on: 1387470
Flags: needinfo?(ato)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8860108 - Flags: review?(hskupin)
(Assignee)

Comment 107

a year ago
Sorry, but it was a misunderstanding on my side. I just read it wrong, and we clarified it in our meeting now. So I will have a look at bug 1387470.
(Assignee)

Comment 108

a year ago
So basically I wasn't that wrong... I wanted to use Andreas patch to just submit all the changes to try to see how our Marionette tests actually perform on all platforms. So this little update would have been nice.

Given that it hasn't been done in the last 22 days, I pushed a patch myself which simply changes the default value of specificationLevel, and leaves everything else in-tact. The try build can be found here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bef44cbf497ae253e6f291982faea3e0f34489be

Updated

a year ago
Depends on: 1396880
(Assignee)

Comment 109

a year ago
Good news, all of our in-tree Marionette tests except a single test is passing. I filed bug 1396880 to get this fixed.
(Assignee)

Updated

a year ago
No longer depends on: 1374283
(Assignee)

Updated

11 months ago
Blocks: 1394354
(Assignee)

Comment 110

11 months ago
Dave, with the new geckodriver 0.19 release we support a new capability with the name "moz:webdriverClick". By default it is False and means using the legacy clickElement from the old days. But turning it to True, tests would use the WebDriver spec conforming clickElement.

Would you mind to check how test suites of the most important Mozilla web properties react on that change? So far we haven't gotten any feedback yet from outside of Marionette unit tests, so we are eager to hear how stable our new implementation is. If everything runs smoothly and we also don't get other negative feedback we would like to turn the new method on as soon as possible. Thanks!
Flags: needinfo?(dave.hunt)
I have tried enabling this via a capability but I'm uncertain if it's actually being enabled. Unless I can confirm that the behaviour has been changed, there's not much point running the tests. Here's an excerpt from the trace geckodriver log:

1505762544430	Marionette	DEBUG	Accepted connection 0 from 127.0.0.1:56309
1505762544430	geckodriver::marionette	TRACE	<- {"applicationType":"gecko","marionetteProtocol":3}
1505762544430	geckodriver::marionette	TRACE	-> 113:[0,1,"newSession",{"capabilities":{"desiredCapabilities":{"moz:webdriverClick":true}},"moz:webdriverClick":true}]
1505762544432	Marionette	TRACE	0 -> [0,1,"newSession",{"capabilities":{"desiredCapabilities":{"moz:webdriverClick":true}},"moz:webdriverClick":true}]
1505762544434	Marionette	CONFIG	Matched capabilities: {"browserName":"firefox","browserVersion":"55.0.1","platformName":"darwin","platformVersion":"16.7.0","pageLoadStrategy":"normal","acceptInsecureCerts":false,"timeouts":{"implicit":0,"pageLoad":300000,"script":30000},"rotatable":false,"specificationLevel":0,"moz:processID":65103,"moz:profile":"/var/folders/wz/3v_j7g2n2zx_q6qs8g7vmyg00000gn/T/rust_mozprofile.VFwXd6bcHXwJ","moz:accessibilityChecks":false}
1505762544457	Marionette	DEBUG	loaded listener.js
1505762544464	Marionette	TRACE	0 <- [1,1,null,{"sessionId":"e231dd23-8d74-f444-a8d8-787b27da91d7","capabilities":{"browserName":"firefox","browserVersion":"55.0.1","platformName":"darwin","platformVersion":"16.7.0","pageLoadStrategy":"normal","acceptInsecureCerts":false,"timeouts":{"implicit":0,"pageLoad":300000,"script":30000},"rotatable":false,"specificationLevel":0,"moz:processID":65103,"moz:profile":"/var/folders/wz/3v_j7g2n2zx_q6qs8g7vmyg00000gn/T/rust_mozprofile.VFwXd6bcHXwJ","moz:accessibilityChecks":false}}]
1505762544465	geckodriver::marionette	TRACE	<- [1,1,null,{"sessionId":"e231dd23-8d74-f444-a8d8-787b27da91d7","capabilities":{"browserName":"firefox","browserVersion":"55.0.1","platformName":"darwin","platformVersion":"16.7.0","pageLoadStrategy":"normal","acceptInsecureCerts":false,"timeouts":{"implicit":0,"pageLoad":300000,"script":30000},"rotatable":false,"specificationLevel":0,"moz:processID":65103,"moz:profile":"/var/folders/wz/3v_j7g2n2zx_q6qs8g7vmyg00000gn/T/rust_mozprofile.VFwXd6bcHXwJ","moz:accessibilityChecks":false}}]
1505762544466	webdriver::server	DEBUG	<- 200 OK {"value": {"sessionId":"e231dd23-8d74-f444-a8d8-787b27da91d7","capabilities":{"acceptInsecureCerts":false,"browserName":"firefox","browserVersion":"55.0.1","moz:accessibilityChecks":false,"moz:processID":65103,"moz:profile":"/var/folders/wz/3v_j7g2n2zx_q6qs8g7vmyg00000gn/T/rust_mozprofile.VFwXd6bcHXwJ","pageLoadStrategy":"normal","platformName":"darwin","platformVersion":"16.7.0","rotatable":false,"specificationLevel":0,"timeouts":{"implicit":0,"pageLoad":300000,"script":30000}}}}
Flags: needinfo?(dave.hunt) → needinfo?(hskupin)
Nevermind, it was a path issue. This is passing a small test suite. I'll try it tomorrow with a couple of larger suites.
Flags: needinfo?(hskupin) → needinfo?(dave.hunt)

Comment 113

11 months ago
Created attachment 8909500 [details]
webdriverClick vs https://github.com/SeleniumHQ/selenium/issues/1740

Attaching a trace log from trying to click on a hidden button under a page header (Selenium debug log intermixed). Even though button I'm clicking is under the header, the click succeeds. Please see reproducer page on https://github.com/SeleniumHQ/selenium/issues/1740

Same behaviour I observe without `moz:webdriverClick`.

The same operation returns an error with the legacy driver. I'm not in fact sure I prefer to see an error but it seems like it shouldn't be possible to click a button under another element.

Another thing that I was looking after is
https://github.com/mozilla/geckodriver/issues/935 and I still reproduce it with 0.19.0
(Assignee)

Comment 114

11 months ago
That behavior is correct as I can see. The overlay doesn't overlay the full area and as such the button0 can be brought into view with scrolling the page. This is step 4 of "Element Click" in the WebDriver spec. As such clicking this button will always work. Here a jsbin page for verification: https://output.jsbin.com/batuwoyuhu

Comment 115

11 months ago
It *can* be scrolled but if button is under the header, then it is *not* actually scrolled. So button is clicked even if it is presently under the header.

I agree original test case is geared towards older scroll behaviour where element is scrolled to the top. I see GeckoDriver is scrolling to the bottom by default. So a test case using a footer instead of a header will make issue more visible. But you can manually scroll the button under the header and call click on it. That will perform no scrolling and click will be successful instead of returning an error.

I will try to explain with other words. If button is inside the screen but under the header, it is considered inside the view, so no scrolling is performed. Then click is successful even though another element will receive the click have it been performed by the user.

btw should your changes have any effect on https://github.com/mozilla/geckodriver/issues/935 ?
(Assignee)

Comment 116

11 months ago
When I was running it through Marionette with webdriverClick enabled it was perfectly scrolling downwards until button0 was visible. I strongly believe that scroll into view means do whatever is necessary with scrolling to show the element. Maybe Andreas can give his feedback on that case too.
Flags: needinfo?(ato)

Comment 117

11 months ago
Created attachment 8909704 [details]
test html for overlay footer

Hi again. Probably my explanation was not clear because it also required manual scrolling to make it obvious. Now I'm attaching the same HTML but with a footer instead of a header. If you open the page and then request say 5th button to be clicked, then you can see the button is scrolled under the footer. Click is also successful.

It is fine from my end if scroll is performed to a place where element is visible automatically. This would be much easier for testing because one wouldn't care about overlay header/footers. I don't know whether that would be possible or not.
ATM though existence of an overlay header/footer seems to be ignored by GeckoDriver for both - scrolling and clicking.

What I can say is that presently scroll is hardcoded to the bottom and even though button is covered by the footer, it is clicked successfully. And this is against spec as far as I can tell.

Also would appreciate if you comment whether your changes are expected to affect https://github.com/mozilla/geckodriver/issues/935 or not.

Thanks again!
I've tested this with three suites, and have seen no regressions when switching to the webdriverClick.
Flags: needinfo?(dave.hunt)
(Assignee)

Updated

11 months ago
Depends on: 1401197
(Assignee)

Comment 119

11 months ago
Ok, I was able to reproduce and I filed bug 1401197.

> Also would appreciate if you comment whether your changes are expected to affect https://github.com/mozilla/geckodriver/issues/935 or not.

As Andreas said and as we duped it might. Test it out. And if it is not working we might have to reopen your github issue, but lets not further discuss this here. Thanks.
(Assignee)

Comment 120

11 months ago
(In reply to Dave Hunt (:davehunt) from comment #118)
> I've tested this with three suites, and have seen no regressions when
> switching to the webdriverClick.

That's great. Thanks a lot!
(Assignee)

Updated

11 months ago
No longer depends on: 1401197
(Reporter)

Comment 121

11 months ago
(In reply to Alexander from comment #115)

> It *can* be scrolled but if button is under the header, then it
> is *not* actually scrolled. So button is clicked even if it is
> presently under the header.

We scroll an element into view using
Element.scrollIntoView({block:"end", inline: "nearest", behavior: "instant"}).
If you don’t specify the {block: "end"} argument, #button0 will be
hidden underneath the overlay as you describe.  With this argument,
it will become visible.

The implementation in Marionette for this can be found in
https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/testing/marionette/element.js#936-946.

> I agree original test case is geared towards older scroll
> behaviour where element is scrolled to the top. I see GeckoDriver
> is scrolling to the bottom by default. So a test case using a
> footer instead of a header will make issue more visible. But you
> can manually scroll the button under the header and call click on
> it. That will perform no scrolling and click will be successful
> instead of returning an error.

Yes, we optimise towards fixed elements at the top of the viewport.
You are entirely correct it will not handle fixed elements at the
bottom.  A test case using a footer will still fail, and I don’t
know about a workaround for this.

> I will try to explain with other words. If button is inside the
> screen but under the header, it is considered inside the view, so
> no scrolling is performed. Then click is successful even though
> another element will receive the click have it been performed by
> the user.

This is incorrect.  It is true that an element is inside the viewport
if it is hidden under the overlay, but the ‘in view’ definition
in the WebDriver specification uses the pointer-interactable paint
tree to see if the element in question is at the top of the stack,
i.e. that #button0 is the first element on the array returned by
document.elementsFromPoint, using #button0’s in-view centre point.

If then the overlay is painted on top of #button0, it will not be
considered in view and the scroll into view action is taken:

This is made obvious from the implementation in
https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/testing/marionette/interaction.js#167-170:

>   // step 4
>   if (!element.isInView(containerEl)) {
>     element.scrollIntoView(containerEl);
>   }
Flags: needinfo?(ato)

Comment 122

11 months ago
(In reply to Andreas Tolfsen ‹:ato› from comment #121)

Andreas, thank you for chiming in. I read your reply as contradicting comment 116. Your position is that marionette should just scroll to the bottom, instead of "do whatever is necessary with scrolling to show the element". Is this correct? Or is this just current behaviour that needs to be fixed?

The other interesting point in your comment is 

> This is incorrect.  It is true that an element is inside the viewport
> if it is hidden under the overlay, but the ‘in view’ definition
> in the WebDriver specification uses the pointer-interactable paint
> tree to see if the element in question is at the top of the stack,
> i.e. that #button0 is the first element on the array returned by
> document.elementsFromPoint, using #button0’s in-view centre point.
> 
> If then the overlay is painted on top of #button0, it will not be
> considered in view and the scroll into view action is taken:
> 
> This is made obvious from the implementation in
> https://searchfox.org/mozilla-central/rev/
> f6dc0e40b51a37c34e1683865395e72e7fca592c/testing/marionette/interaction.
> js#167-170:
> 
> >   // step 4
> >   if (!element.isInView(containerEl)) {
> >     element.scrollIntoView(containerEl);
> >   }

I don't observe this behaviour. When I call `element.click` in watir/selenium, the page is scrolled only if element is outside screen. If it is under some overlay header/footer/menu, then no scrolling is being performed. Same behaviour as the legacy driver btw.
(Reporter)

Comment 123

11 months ago
(In reply to Alexander from comment #122)

> Andreas, thank you for chiming in. I read your reply as
> contradicting comment 116.

Not contradicting that, because the scroll into view behaviour
based on hit testing I described is only available when using
element.webdriverClick, whereas the default is element.seleniumClick

With geckodriver 0.19.0 and Firefox Nightly (58) it should be
possible to configure the moz:webdriverClick capability to enable
the WebDriver conforming click implementation.  I would expect the
problem you’re describing to reproduce if this isn’t enabled
properly.

> Your position is that marionette should just scroll to the bottom,
> instead of "do whatever is necessary with scrolling to show the
> element". Is this correct? Or is this just current behaviour that
> needs to be fixed?

Please describe a technique that will allow us to “do whatever is
necessary” to bring an element into view.  As far as I’m aware
you can only configure ScrollIntoViewOptions so that it scrolls it
in either near the top or the bottom.  The current implementation
optimises towards elements overlaid at the top of the viewport
because these is more common.

> (In reply to Andreas Tolfsen ‹:ato› from comment #121)
> 
> > This is incorrect.  It is true that an element is inside
> > the viewport if it is hidden under the overlay, but the
> > ‘in view’ definition in the WebDriver specification
> > uses the pointer-interactable paint tree to see if the
> > element in question is at the top of the stack, i.e. that
> > #button0 is the first element on the array returned by
> > document.elementsFromPoint, using #button0’s in-view centre
> > point.
> > 
> > If then the overlay is painted on top of #button0, it will not
> > be considered in view and the scroll into view action is taken:
> > 
> > This is made obvious from the implementation in
> > https://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/testing/marionette/interaction.js#167-170:
> > 
> > >   // step 4
> > >   if (!element.isInView(containerEl)) {
> > >     element.scrollIntoView(containerEl);
> > >   }
> 
> I don't observe this behaviour. When I call `element.click` in
> watir/selenium, the page is scrolled only if element is outside
> screen. If it is under some overlay header/footer/menu, then no
> scrolling is being performed. Same behaviour as the legacy driver
> btw.

See my above comment about enabling the element.webdriverClick
function.

Comment 124

11 months ago
My original test was described in Comment 113. I believe webdriver click has been enabled. I attached a log file with comment 113 so hopefully somebody can confirm whether webdriver click was enabled or not. Are you able to tell whether I'm turning on this feature properly based on that log?

> Please describe a technique that will allow us to “do whatever is
necessary” to bring an element into view.

I can't describe such a technique. It was not my idea. It is from comment 116 based on author's reading of the spec. I'm just saying it will be very useful if possible :) Also I don't know how exactly we should read the spec in that regard.
(Reporter)

Comment 125

11 months ago
(In reply to Alexander from comment #124)

> My original test was described in Comment 113. I believe webdriver
> click has been enabled. I attached a log file with comment 113 so
> hopefully somebody can confirm whether webdriver click was enabled
> or not. Are you able to tell whether I'm turning on this feature
> properly based on that log?

Thanks for attaching those logs.  I took a look, and you are
using Firefox 55, which doesn’t have moz:webdriverClick.  You
need to test this with Firefox Nightly (58).  You can verify
that moz:webdriverClick is enabled by looking at the returned
capabilities.  There should be a {"moz:webdriverClick": true} field
in the JSON Object.
(Assignee)

Comment 126

11 months ago
Or Firefox 57 which will have its first beta 1 today.

Comment 127

11 months ago
Created attachment 8913900 [details]
HTML for moz:webdriverClick test case (ff_new_click.html)

Here's a test case that illustrates a couple of failures I found in our codebase.

The isAtPoint() function I stuck in the HTML page is a simplified version of my understanding of what Marionette is doing to check if an element is in view. I called it from devtools to observe that Document.elementsFromPoint doesn't contain the target elements, even though they seem to be visible.

Comment 128

11 months ago
Created attachment 8913901 [details]
Selenium test that illustrates that the old click works

And here's a Python test that shows that the old click works with the HTML I attached above, while the new click doesn't.

Comment 129

11 months ago
I also saw 4 failures in the Java Selenium test suite:

CorrectEventFiringTest.testNativelyClickOverlappingElements: I think this test is actually incorrect, and it should just be suppressed for Marionette; testClickOverlappingElements should be unsuppressed because it tests the expected behavior where an error is thrown if the target element is obscured.

FormHandlingTest.testCanClickOnASubmitButtonNestedSpan: This looks to be the same as the button>span test case I posted above.

SvgElementTest.testShouldClickOnGraphVisualElements and SvgElementTest.testShouldClickOnGraphTextElements: Probably along the same lines as the elementsFromPoint issue above, but I didn't look into these.

Comment 130

11 months ago
Report of moz:webdriverClick with FF 57 beta4/geckodriver 0.19.0:
* elements out of screen scrolled to the bottom of the page (unlike legacy driver which scrolled to the top)
* when overlayed element is clicked error is raised: Selenium::WebDriver::Error::ElementClickInterceptedError

This appears to be the expected behaviour according to comments above. A few questions:

A) the error message is:

> #<Selenium::WebDriver::Error::ElementClickInterceptedError: Element <button id=\"button5\"> is not clickable at point (635,869) because another element <a> obscures it>\n"

I think giving more details about the overlapped element will help debugging failures by reading the error message. Just "a" speaks very little.

B) Would it be in any way possible to add more scrolling strategies like scroll to the middle of the page (horizontally and vertically)? Or some other coordinates such that different web sites may choose easily values that work? Or driver try a few configurable scrolling locations so that it works automatically as much as possible?

Who can authoritatively say what is the idea behind `scroll into view` in webdriver spec? Is it do whatever possible or is it scroll somewhere and forget?

Thank you.
(Reporter)

Comment 131

11 months ago
(In reply to Alexander from comment #130)

> I think giving more details about the overlapped element will help
> debugging failures by reading the error message. Just "a" speaks
> very little.

What information do you suggest adding?  I would file a separate bug
about this.

> B) Would it be in any way possible to add more scrolling
> strategies like scroll to the middle of the page (horizontally
> and vertically)? Or some other coordinates such that different
> web sites may choose easily values that work? Or driver try a few
> configurable scrolling locations so that it works automatically as
> much as possible?

ScrollIntoViewOptions only allows for bottom and top, but it is
known that always scrolling in to the bottom will under certain
circumstances make it impossible to interact with the element.  I
would file a bug against the WebDriver specification on this.

> Who can authoritatively say what is the idea behind `scroll into
> view` in webdriver spec? Is it do whatever possible or is it
> scroll somewhere and forget?

I don’t understand the question.

Comment 132

11 months ago
(In reply to Andreas Tolfsen ‹:ato› from comment #131)
> (In reply to Alexander from comment #130)
> > Who can authoritatively say what is the idea behind `scroll into
> > view` in webdriver spec? Is it do whatever possible or is it
> > scroll somewhere and forget?
> 
> I don’t understand the question.

I interpret this question as asking: "The spec says to scroll the element into view. How exactly is 'scroll into view' defined in the spec?"

The answer is that "scroll into view" is defined within the spec: <https://w3c.github.io/webdriver/webdriver-spec.html#dfn-scroll-into-view>. That definition prescribes a specific course of action, and does not promise to "do whatever possible" to put the element in an interactable state. (In general, "do whatever possible" is not great spec language.)

(In answer to the *literal* question of "who can authoritatively state this" -- you can! If a definition for any term is not written in the text of the spec, then it is not authoritative. If you cannot find it there, then file a bug so that it can be added.)

Generally, the goal is to make a specification that is unambiguous and implementable. That doesn't mean specifying a system that is capable of solving all known problems with a single command. In those cases where a WebDriver user knows that the prescribed scrolling behavior isn't going to work, they can work around that on a case-by-case basis. Generalizing the behavior to "do whatever possible to achieve my desired state" seems prohibitively difficult for now.
(Reporter)

Comment 133

11 months ago
Thanks for the clarifications, Jason.  I do mean to reply to your
earlier comments as well, but they require a bit more thoughtwork on
my end, so please excuse the delay in response on that front.

(In reply to Jason Juang (:juangj) from comment #132)

> Generally, the goal is to make a specification that is unambiguous
> and implementable. That doesn't mean specifying a system that is
> capable of solving all known problems with a single command. In
> those cases where a WebDriver user knows that the prescribed
> scrolling behavior isn't going to work, they can work around
> that on a case-by-case basis. Generalizing the behavior to "do
> whatever possible to achieve my desired state" seems prohibitively
> difficult for now.

Yes, precisely this.  I suspect we are going to have to lobby the
CSS WG for a new variation on ScrollIntoViewOptions enum that
scrolls an element into view by its in-view centre point.  That
implies there needs to be a CSS definition of in-view centre point,
which is a much tougher sell, but they may be able to repurpose our
definition from WebDriver.

Comment 134

11 months ago
Hi again, thank you for the pointers.

I filed bug 1405474 about better error message.

wrt scrolling option, I tried to follow spec but I'm a little bit lost. Looking at the scroll into view description [1] I read it as spec mandates logical scroll position option to be "end".

Possible options for scroll positions can be { "start", "center", "end", "nearest" } [2]. The scroll into view method [3] should scroll element according to provided options in the method argument (or `end` if argument is false). Also point 8 of scroll into view method [3] description allows "Optionally perform some other action that brings the element to the user’s attention."

What I understand is that we can file a bug for webdriver to allow user to set any supported options to scroll into view method [3] instead of the hardcoded values specified in webdriver scroll into view description [1].

Did I understand correctly? 

I didn't understand at all comment 133 "new variation on ScrollIntoViewOptions enum that
scrolls an element into view by its in-view centre point". What change would be needed exactly and why? I'd like to file necessary bugs but CSS/webUI are not my primary area of expertise so I might be missing things that seem obvious to others.

[1] https://w3c.github.io/webdriver/webdriver-spec.html#dfn-scroll-into-view
[2] https://drafts.csswg.org/cssom-view/#dom-scrollintoviewoptions-block
[3] https://drafts.csswg.org/cssom-view/#dom-element-scrollintoview
(Assignee)

Updated

11 months ago
Depends on: 1405474

Comment 135

11 months ago
One more example of an element that can't be clicked with moz:webdriverClick. It's actually the same example as in https://github.com/mozilla/geckodriver/issues/653, which is a bug I filed for the old click behavior:

<!DOCTYPE html>
<a href="http://www.example.com/">
  <div>Click me</div>
</a>

Marionette can click the inner <div>, but not the <a>.

Interestingly, calling getClientRects() on the <a> reveals 3 rects, the first and last of which have width 0:
[
  { left: 8, top: -7, width: 0, height: 19 },
  { left: 8, top: 8, width: 1486, height: 19 },
  { left: 8, top: 12, width: 0, height: 19 },
]

The center point of the first client rect is (8, 2.5), which does actually seem to fall outside of the element's bounding rect. I'm not sure what this means, but it doesn't seem right -- is this a bug in `getClientRects`, or a bug in the WebDriver specification of `pointer-interactable paint tree` (which requires using the first client rect)?

FWIW, Chrome returns these client rects for the <a>:
[
  { left: 8, top: 8, width: 0, height: 0},
  { left: 8, top: 8, width: 1527, height: 18},
]
(Assignee)

Updated

11 months ago
Depends on: 1405967
(Assignee)

Updated

11 months ago
Blocks: 1405967
No longer depends on: 1405967
(Assignee)

Comment 136

11 months ago
Jason, is your issue related to bug 1374283? If not can you please file a new bug blocking this bug?
Flags: needinfo?(juangj)

Comment 137

11 months ago
Ah, it does seem to be the same as that bug.
Flags: needinfo?(juangj)
(Assignee)

Comment 138

11 months ago
(In reply to Jason Juang (:juangj) from comment #135)
> One more example of an element that can't be clicked with
> moz:webdriverClick. It's actually the same example as in
> https://github.com/mozilla/geckodriver/issues/653, which is a bug I filed
> for the old click behavior:

Jason, so you are saying that this click also didn't work before? If that is the case we should not block on that for turning on webdriver conforming clicks. Only those failures which are not visible with the legacy clickElement method but with webdriver conforming clicks should block us at the moment. Everything else we have to fix later in our implementation or by requesting updates to the spec. Anyway, can you please file a new bug so we do not miss your case? Thanks.
Flags: needinfo?(juangj)

Comment 139

11 months ago
That's right, the click also didn't work with the Selenium click. I do think comment 135 is the exact same issue as bug 1374283, which I wasn't aware of before. I do agree with your point that this particular issue shouldn't block the new click.

My test case from comment 127 and comment 128 is not the exact same bug, though, and it does represent a failure that occurs only with the new WebDriver click. Do you want that in a separate bug, too?
Flags: needinfo?(juangj)
(Assignee)

Comment 140

11 months ago
(In reply to Jason Juang (:juangj) from comment #139)
> My test case from comment 127 and comment 128 is not the exact same bug,
> though, and it does represent a failure that occurs only with the new
> WebDriver click. Do you want that in a separate bug, too?

Yes, please file a new bug for that which should block this one. I can then have a look at it. For the other issue you can also file a new bug, so it's not forgotten. Thanks.

Updated

11 months ago
Depends on: 1406505
(Assignee)

Updated

10 months ago
Attachment #8913900 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

10 months ago
Attachment #8913900 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8913901 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8909500 - Attachment is obsolete: true
(Assignee)

Comment 141

10 months ago
Andreas, can you have a look at comment 134? I would like to see if we can close out the discussion / issue Alexander is having. I would like to know if that blocks the switch or not. Thanks.
Flags: needinfo?(ato)
(Assignee)

Updated

10 months ago
Attachment #8860108 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8860977 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8860976 - Attachment is obsolete: true
(Assignee)

Comment 142

10 months ago
Btw. I removed the old patches which mostly are bitrotted and are also more related to bug 1405967 now. This bug is just for the switch now, and I will provide a patch early tomorrow.
Assignee: ato → hskupin
status-firefox57: --- → affected
status-firefox58: --- → affected
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8924309 - Flags: review?(ato)
(Reporter)

Comment 144

10 months ago
mozreview-review
Comment on attachment 8924309 [details]
Bug 1321516 - Switch to WebDriver conformant interactability checks.

https://reviewboard.mozilla.org/r/195558/#review200762

This looks good!  I’m excited by this change, in particular all
the work that has lead up to it.

::: commit-message-aa958:1
(Diff revision 1)
> +Bug 1321516 - Switch to WebDriver conformant interactability checks.

I don’t normally do this, but I want you to expand the commit
message to say concretely what means.  Also it would be good to say
that the capability can be reversed to retain the old behaviour.
Attachment #8924309 - Flags: review?(ato) → review+
(Assignee)

Comment 145

10 months ago
FYI: If we want to have this feature in Firefox 57, the uplift has to be done before Nov 6th, Monday. Otherwise we will miss it.

I will finish the patch up shortly, and will let it bake for 2 days at maximum before asking for uplift.
Comment hidden (mozreview-request)

Comment 147

10 months ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a29ab4c986c7
Switch to WebDriver conformant interactability checks. r=ato

Comment 148

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a29ab4c986c7
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Assignee)

Comment 149

10 months ago
Andreas and David, I made a couple of thoughts regarding the uplift to 57. More and more I think we should not doing that. Here are my reasons:

* Firefox Quantum needs to become an awesome release. As such making this change that late in the cycle - indeed a couple of days before the release candidate - doesn't seem to be the right solution. 

* Enabling wdspec clicks by default now, and remove the legacy code in 58 would give people only a timeframe of 6 weeks to update their tests in case of failures, and us to fix ALL the reported issues. Please note that we only received feedback from a couple of people yet. I would like to see way more feedback.

My proposal is to not uplift this patch for Firefox 57. What do you both think?
Flags: needinfo?(dburns)
(Assignee)

Updated

10 months ago
Blocks: 1414221
I agree with your reasoning and agree that we should not uplift.
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
(Assignee)

Comment 151

10 months ago
Keeping Andreas on ni? to answer comment 141.
status-firefox57: affected → wontfix
Flags: needinfo?(ato)
(Reporter)

Comment 152

10 months ago
(In reply to Alexander from comment #134)

> wrt scrolling option, I tried to follow spec but I'm a little bit
> lost.  Looking at the scroll into view description [1] I read it
> as spec mandates logical scroll position option to be "end".
> 
> Possible options for scroll positions can be { "start", "center",
> "end", "nearest" } [2]. The scroll into view method [3] should
> scroll element according to provided options in the method
> argument (or `end` if argument is false). Also point 8 of scroll
> into view method [3] description allows "Optionally perform some
> other action that brings the element to the user’s attention."

We can only feasibly support "start" and "end" because "center",
"inline", and "nearest" are not available in all browsers.  See the
notes under Browser compatibility for Element.scrollIntoView():

    https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView

> What I understand is that we can file a bug for webdriver to allow
> user to set any supported options to scroll into view method [3]
> instead of the hardcoded values specified in webdriver scroll into
> view description [1].

You can file a bug, but we are not going to change the behaviour
for level 1.  Any change we make to the specification in the future
must be backwards compatible, so the default must always remain the
current options.  I’m personally not convinced that making it
configurable is a good course of action versus choosing sensible
defaults.

> I didn't understand at all comment 133 "new variation on
> ScrollIntoViewOptions enum that scrolls an element into view by
> its in-view centre point". What change would be needed exactly
> and why? I'd like to file necessary bugs but CSS/webUI are not my
> primary area of expertise so I might be missing things that seem
> obvious to others.

The ‘in-view center point’ definition from WebDriver is
different to scrollIntoView’s approximation for the "center"
option of the "block" option.  This might not matter if it makes a
best-effort at bringing the element into view.  Quite possibly this
is not an issue, but only a test will tell.
Flags: needinfo?(ato)
(Reporter)

Comment 153

10 months ago
(In reply to Henrik Skupin (:whimboo) from comment #149)

> My proposal is to not uplift this patch for Firefox 57. What do
> you both think?

I’m fine with delaying it one release.
(Assignee)

Comment 154

10 months ago
As we just figured out the sendKeysToElement method is also not using the WebDriver conformant interactability checks. As such we need a follow-up, which I will file in a moment.
Summary: Switch to WebDriver conformant interactability checks → Switch to WebDriver conformant interactability checks for clickElement
(Assignee)

Updated

10 months ago
Blocks: 1414322
(Assignee)

Updated

10 months ago
Depends on: 1415068
(Assignee)

Updated

10 months ago
Depends on: 1415067
(Assignee)

Updated

9 months ago
Depends on: 1423325
(Assignee)

Updated

8 months ago
Blocks: 1401197
(Reporter)

Updated

6 months ago
Duplicate of this bug: 1368453
You need to log in before you can comment on or make changes to this bug.