1200197 - element.tap() doesn't work when hidden inside overflow:scroll element

Status: RESOLVED INCOMPLETE

(Remote Protocol :: Marionette, defect)

Description

[Martijn Wargers (dead)]

Marionette's singleTap function scrolls an element into view when it's outside the viewport.
However, it doesn't do that when elements are out of view inside a overflow:scroll block, for example.
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/listener.js#867

We should use document.elementFromPoint and change the elementInViewport function so that it checks if the element is in view:
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/listener.js#917

Comment 1

[Martijn Wargers (dead)]

Attachment: 1200197.diff

Something like this makes it work. I also adjusted the testcase in the patch to make it work on Firefox desktop, because I don't know how to run it for b2g, really.

Comment 2

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8654896 [details] [diff] [review]
1200197.diff

I'm going to pass this review to AutomatedTester; I think he has a better idea of how this method is supposed to work.

Comment 3

[David Burns :automatedtester]

Comment on attachment 8654896 [details] [diff] [review]
1200197.diff

Martin, please can you push this to Mozreview and I will review there.

Comment 4

[Martijn Wargers (dead)]

Attachment: https://reviewboard.mozilla.org/r/18225/

Comment 5

[David Burns :automatedtester]

Martijn,

I am not sure how you pushed to mozreview but details on how to use Mozreview can be found at 
http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html

I can't see the diff on Mozreview which makes it difficult to review.

Comment 6

[Martijn Wargers (dead)]

(In reply to David Burns :automatedtester from comment #5)
> I can't see the diff on Mozreview which makes it difficult to review.

The diff is right there, no? https://reviewboard.mozilla.org/r/18225/diff/1#index_header

Comment 7

[David Burns :automatedtester]

This patch breaks a number of this that would have been caught with a try run. Please only ask for review after a try run and post the link in here so I can review the results

Comment 8

[Martijn Wargers (dead)]

But you can see the patch now, right?

Comment 9

[Martijn Wargers (dead)]

What is "breaks a number of this", btw?

Comment 10

[David Burns :automatedtester]

s/this/tests/

Comment 11

[David Burns :automatedtester]

What the issue i think you are wanting to solve here is that you can scroll the element into view, not allow clicks to happen on invisible tests

Comment 12

[David Burns :automatedtester]

s/tests/elements

Comment 14

[Martijn Wargers (dead)]

I see a failure with the patch when running: http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_accessibility.py

However, I also get a failure without the patch when just running this part:
    def test_element_is_not_enabled_to_accessbility(self):

        self.setup_accessibility(False, False)

        self.run_element_test(self.disabled_accessibility_elementIDs,
                              lambda element: element.click())

e.g.:
NoSuchElementException: NoSuchElementException: Unable to locate element: button12

When I add this test after it:
    def test_element_is_enabled_to_accessibility(self):
        self.setup_accessibility()
        # No exception should be raised
        self.run_element_test(self.disabled_elementIDs, lambda element: element.is_enabled())

Then the previous test and this test is passing again.

Yura, are subtests like these supposed to depend on each other like that? I don't see a relation between the two.

Comment 15

[Martijn Wargers (dead)]

The accessibility api uses the checkVisible method in this way:
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/listener.js#1453
1453 function clickElement(id) {
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/listener.js#889
889 function checkVisible(el, x, y) {
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/listener.js#985
985 function checkVisibleAccessibility(accesible, visible) {

By changing elementInViewport in the patch, checkVisible returns false for css pointer:none elements. It seems like Marionette tap/click method tries to tap on those elemens, currently, which is not really possible.
But perhaps accessibility can still do something with those css pointer:none elements?

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

I suspect this happens with individual test because accessibility is not initialized in time for when the test runs and thus the test fails (I get this failure in test_element_is_not_enabled_to_accessbility: AssertionError: ElementNotAccessibleException not raised)

So in order to make sure that accessibility is initialized in time, I needed to add this to setup_accessibility function (to wait for a11y ready):

self.marionette.set_context('chrome')
self.marionette.execute_async_script("""
     var accRetrieval = Components.classes["@mozilla.org/accessibleRetrieval;1"].
       getService(Components.interfaces.nsIAccessibleRetrieval);
     var waitForDocLoad = function() {
       window.setTimeout(function() {
         var accDoc = accRetrieval.getAccessibleFor(document);
         var state = {};
         accDoc.getState(state, {});
         if (state.value & Components.interfaces.nsIAccessibleStates.STATE_BUSY) {
           return waitForDocLoad();
         }
         window.setTimeout(marionetteScriptFinished, 0);
       }, 0);
     };
     waitForDocLoad();""")
self.marionette.set_context('content')

Ideally I would love to be able to wait for this in a general case to avoid false negatives. Perhaps you or David would know?

Comment 17

[Martijn Wargers (dead)]

When running test_element_is_not_enabled_to_accessbility, this error occurs with the patch applied:
ElementNotVisibleException: ElementNotVisibleException: Element is not visible
http://mxr.mozilla.org/mozilla-central/source/testing/marionette/listener.js#1460
This is because the element is not clickable (although it is visible), due to css pointer: none.

I really think Marionette should not try to click on elements that are not clickable, so I think it's actually good that this exception is occuring. Although the name of the exception is a bit weird, because the element is fully visible. But I don't see a better exception name for this case.

Comment 18

[Martijn Wargers (dead)]

Attachment: test_accessibility.diff

With the patch applied, I tried something like this included and then I get the ElementNotAccessibleException exception, but when I use ElementNotAccessibleException in the run_element_test, then I get the ElementNotVisibleException again.

I don't understand this. I thought that if the exception is raised, "ElementNotVisibleException", that the second one, "ElementNotAccessibleException" would not occur anymore, because js is stopped after the first exception.

Comment 19

[Martijn Wargers (dead)]

(In reply to Yura Zenevich [:yzen] from comment #16)
> Ideally I would love to be able to wait for this in a general case to avoid
> false negatives. Perhaps you or David would know?

If you want to add it to every marionette-test, I guess you could add it here: http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette_test.py#686

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

Note the issue with test_accessibility looks to be fixed with bug 1201595 (some improvements with timing for a11y checks throwing), also some improvements with tap in particular.

Comment 21

[Martijn Wargers (dead)]

Irc discussion with Yura:
14:34 yzen: mwargers so platform a11y will make an accessible object for it but it will not be actionable
14:35 mwargers: what do you mean with not actionable?
14:36 yzen: mwargers clicked, tapped, etc via assistive technology (screen reader for example)
14:36 mwargers: ah, ok
14:36 yzen: mwargers essentially almost like if it's disabled from a11y standpoint, this is actually one of the problematic patterns we saw in gaia
14:38 mwargers: so we need an isActionable check or something for https://bugzilla.mozilla.org/show_bug.cgi?id=1200197 ?
14:39 mwargers: because element.tap() should not work for elements with pointer: none
14:41 ahal has joined ([e-mail])
14:42 yzen: mwargers so we already check if something is actionable on tap (https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/listener.js#L926) but unlike with click we do not check if it's enabled for accessibility (https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/listener.js#L1456) which checks pointer events none. This was done to resemble the fact that on tap, unlike click, marionette does not check for isElementEn
14:42 yzen: abled. At the time I asked AutomatedTester|AFK about it and he mentioned there was a reason for that
14:43 yzen: mwargers i dont know the reasons for that but imo I would check isElementEnabled and checkEnabledAccessibility on tap
14:44 mwargers: yzen: do you have perhaps links of the source of those methods?
14:44 yzen: sure
14:44 yzen: https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/listener.js#L951-L970
14:44 yzen: https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/listener.js#L998-L1014

Comment 22

[Martijn Wargers (dead)]

What I think singleTap is doing/trying to do:
- It tries to check if the element can be tapped upon.
- If it doesn't, it tries to scroll the element into view.
- If the element then still can't be tapped upon, then it throws an error.
- If the element then can be tapped upon, then it tries to tap on it

Yura, I'm not sure what the accessibility code is doing in all of this? Is it trying to improve the "detect if element is tappable/clickable" thing or is it part of accessibility testing?
The best way to check if an elmenent is tappable/clickable is the document.elementFromPoint API, I think. It would also detect cases where elements are completely overlapped by other elements for instance (something which I think does not work with this code, either).

Comment 23

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #22)
> What I think singleTap is doing/trying to do:
> - It tries to check if the element can be tapped upon.
> - If it doesn't, it tries to scroll the element into view.
> - If the element then still can't be tapped upon, then it throws an error.
> - If the element then can be tapped upon, then it tries to tap on it
> 
> Yura, I'm not sure what the accessibility code is doing in all of this? Is
> it trying to improve the "detect if element is tappable/clickable" thing or
> is it part of accessibility testing?

So the accessibility checks do not improve anything from the typical standpoint. They merely test that if the element looks tappable normally that it also can be activated with the platform accessibility.

> The best way to check if an elmenent is tappable/clickable is the
> document.elementFromPoint API, I think. It would also detect cases where
> elements are completely overlapped by other elements for instance (something
> which I think does not work with this code, either). 

For accessibility it is not relevant if the element is overlapped, off screen. In many cases screen reader, for example, can access those elements when typical user can't.

Comment 24

[David Burns :automatedtester]

Closing as B2G related but there might be more to it. If I find more will reopen.