Andreas Tolfsen ❲:ato❳ Reporter
	Description • 8 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 1 • 8 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.

	David Burns :automatedtester
	Comment 7 • 7 years ago

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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 12 • 7 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.).

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 13 • 7 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 14 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 20 • 7 years 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.

	David Burns :automatedtester
	Comment 21 • 7 years ago

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.

	David Burns :automatedtester
	Comment 22 • 7 years ago

Google have hit an issue and have gratiously recreated the issue for us.

https://gist.github.com/juangj/9d5ed2c12fbb789eaed7e9aa006d054f

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 23 • 7 years 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?

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 24 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 25 • 7 years 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?

	David Burns :automatedtester
	Comment 26 • 7 years ago

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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 27 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 28 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 29 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 66 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 87 • 7 years 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?

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 88 • 7 years 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.)

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 89 • 7 years 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?

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 90 • 7 years 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.)

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 94 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 95 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 96 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 97 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 98 • 7 years 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 ')'

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 99 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 100 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 101 • 7 years 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!

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 102 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 103 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 107 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 108 • 7 years 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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 109 • 7 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 110 • 7 years 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!

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 111 • 7 years ago

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}}}}

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 112 • 7 years ago

Nevermind, it was a path issue. This is passing a small test suite. I'll try it tomorrow with a couple of larger suites.

	Alexander
	Comment 113 • 7 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 114 • 7 years 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

	Alexander
	Comment 115 • 7 years 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 ?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 116 • 7 years 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.

	Alexander
	Comment 117 • 7 years ago

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!

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 118 • 7 years ago

I've tested this with three suites, and have seen no regressions when switching to the webdriverClick.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 119 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 120 • 7 years 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!

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 121 • 7 years 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);
>   }

	Alexander
	Comment 122 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 123 • 7 years 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.

	Alexander
	Comment 124 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 125 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 126 • 7 years ago

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

	Jason Juang (:juangj)
	Comment 127 • 7 years ago

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.

	Jason Juang (:juangj)
	Comment 128 • 7 years ago

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.

	Jason Juang (:juangj)
	Comment 129 • 7 years 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.

	Alexander
	Comment 130 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 131 • 7 years 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.

	Jason Juang (:juangj)
	Comment 132 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 133 • 7 years 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.

	Alexander
	Comment 134 • 7 years 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

	Jason Juang (:juangj)
	Comment 135 • 7 years 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},
]

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 136 • 7 years ago

Jason, is your issue related to bug 1374283? If not can you please file a new bug blocking this bug?

	Jason Juang (:juangj)
	Comment 137 • 7 years ago

Ah, it does seem to be the same as that bug.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 138 • 7 years 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.

	Jason Juang (:juangj)
	Comment 139 • 7 years 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?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 140 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 141 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 142 • 7 years 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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 144 • 7 years 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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 145 • 7 years 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.

	Pulsebot
	Comment 147 • 7 years ago

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

	Andreea Pavel [:apavel]
	Comment 148 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/a29ab4c986c7

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 149 • 7 years 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?