1406505 - element.isInView() fails, because child nodes inside a button, or tr nodes are not returned by elementsFromPoint

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P2)

Description

[Jason Juang (:juangj)]

Attachment: Selenium test that illustrates that the old click works

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36

Steps to reproduce:

This is split off from https://bugzilla.mozilla.org/show_bug.cgi?id=1321516#c127.

The WebDriver-conformant click fails to click certain elements, illustrated below. The attached file is a Python Selenium test case that shows that the click succeeds with the old click, and fails with the new click.

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. It shows that Document.elementsFromPoint doesn't contain the target elements, even though they seem to be visible.

```
<div>
  <button>
    <span>isAtPoint($('button span')) == false</span>
  </button>
</div>

<table>
  <tr>
    <td>isAtPoint($('tr')) == false</td>
  </tr>
</table>

<script>
function isAtPoint(el) {
  const r = el.getClientRects()[0];
  const [x, y] = [r.x + r.width/2, r.y + r.height/2];
  const tree = document.elementsFromPoint(x, y);
  console.log([x, y]);
  console.log(tree);
  return tree.includes(el);
}
</script>
```


Actual results:

{
  "error":"element not interactable",
  "message":"Element <tr> could not be scrolled into view",
  ...
}

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The HTML source as given in comment 0 can also be found as attachment 8913900 [details], which makes it easier to use for a test.

I can reproduce the problem now, and would have to check why it's failing.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hm, I don't actually think that this is a bug in Marionette's implementation. Take the following data URL as example:

data:text/html;charset=utf-8,%3Cbutton%3E%3Cspan%3Efoo%3C/span%3E%3C/button%3E

When I load this page, and use the developer tools to run `document.elementsFromPoint(x, y)` for whatever values of x and y, I never get the <span> included in the result. It always only lists at maximum the <button>. 

This also only happens when we use a <button> element as wrapper. When a <div> is used it all works fine.

Olli, would you mind having a look at this example? Maybe there is a known bug in Firefox already? Thanks

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: HTML test page

This simple test page demonstrates the problem. Just move around and click at the wanted position.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

A workaround for the time being could be that we assume <button> as container, so that for any child node a click would actually scroll the button into view.

As discussed on IRC adding ni? for Andreas, so that we can figure out a good solution for now.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

In Firefox <button> is the hit testing target, not any elements inside it. We're considering changing that.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #5)
> In Firefox <button> is the hit testing target, not any elements inside it.
> We're considering changing that.

Good to know about that. Thanks. Is there already a bug about this possible change? I would like to know otherwise we would miss to remove our "workaround" which I would implement now.

So it means we will make the button the container for scrollIntoView and clicking.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I guess it is bug 1089326

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That sounds plausible. I will add it to the dependency list so that we are getting notified when the change got implemented. Thanks.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1406505 - Fix element.getContainer() to return button as container.

Review commit: https://reviewboard.mozilla.org/r/191612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191612/

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Andreas, this patch is WIP but I would like to hear an early feedback from you.

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8920618 [details]
Bug 1406505 - Fix element.getContainer() to return button as container.

https://reviewboard.mozilla.org/r/191612/#review197252

::: testing/marionette/element.js:850
(Diff revision 1)
>   *     Container element of |el|.
>   */
>  element.getContainer = function(el) {
> -  if (el.localName != "option") {
> -    return el;
> +  let container = el;
> +
> +  function _getContainer(ctx, container_types) {

I think this deserves a more descriptive name.  It walks the
ancestral elements until it can find one of container_types, so
maybe findAncestralElement?

::: testing/marionette/element.js:850
(Diff revision 1)
>   *     Container element of |el|.
>   */
>  element.getContainer = function(el) {
> -  if (el.localName != "option") {
> -    return el;
> +  let container = el;
> +
> +  function _getContainer(ctx, container_types) {

s/ctx/startNode/ maybe?

::: testing/marionette/element.js:850
(Diff revision 1)
>   *     Container element of |el|.
>   */
>  element.getContainer = function(el) {
> -  if (el.localName != "option") {
> -    return el;
> +  let container = el;
> +
> +  function _getContainer(ctx, container_types) {

s/container_types/validAncestors/ or something?

::: testing/marionette/element.js:867
(Diff revision 1)
> -  if (!validContext(parent)) {
> -    return el;
> +  // Child nodes of button will not be part of the element tree for
> +  // elementsFromPoint until bug 1321516 is fixed.

Does that mean the if-condition should be guarded on whether
moz:webdriverClick is enabled?

::: testing/marionette/element.js:869
(Diff revision 1)
> +  } else {
> +    container = _getContainer(el, ["button"]);
>    }
> -  return parent;
> +
> +  return container;

Change this so we can get rid of the else-clause.  That means the
"container" variable can go and that we can return directly from
_getContainer.

Comment 12

[Andreas Tolfsen ❲:ato❳]

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

> Andreas, this patch is WIP but I would like to hear an early
> feedback from you.

I think your patch makes sense, given the context that <button> will
always be the hit target.

I made some review comments, because that was the most convenient
way of referencing particular code lines, but I only had minor name
suggestions and nits.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: HTML test page

Updated HTML test case which also includes a table now.

As it can be seen when hovering over `Doe` the table cell is listed in the element tree, but the <tr> element doesn't exist.

Olli, can I assume that this is left out on purpose? Or is there another reason (bug) why table rows are not getting included in the element tree?

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8920618 [details]
Bug 1406505 - Fix element.getContainer() to return button as container.

https://reviewboard.mozilla.org/r/191612/#review197252

> Does that mean the if-condition should be guarded on whether
> moz:webdriverClick is enabled?

Ups! I put in the wrong bug id as reference. What I actually meant to add here is bug 1089326. Does that solve the question? It's really a Firefox behavior and affects both methods.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8920618 [details]
Bug 1406505 - Fix element.getContainer() to return button as container.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191612/diff/1-2/

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Created attachment 8922225 [details]
> HTML test page
> 
> Updated HTML test case which also includes a table now.
> 
> As it can be seen when hovering over `Doe` the table cell is listed in the
> element tree, but the <tr> element doesn't exist.
> 
> Olli, can I assume that this is left out on purpose? Or is there another
> reason (bug) why table rows are not getting included in the element tree?

tr elements are in the tree, but looks like elementsFromPoint doesn't return them.
Why that behavior, I don't know. That is a question to the layout folks.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Olli Pettay [:smaug] from comment #16)
> (In reply to Henrik Skupin (:whimboo) from comment #13)
> > Created attachment 8922225 [details]
> > HTML test page
> > 
> > Updated HTML test case which also includes a table now.
> > 
> > As it can be seen when hovering over `Doe` the table cell is listed in the
> > element tree, but the <tr> element doesn't exist.
> > 
> > Olli, can I assume that this is left out on purpose? Or is there another
> > reason (bug) why table rows are not getting included in the element tree?
> 
> tr elements are in the tree, but looks like elementsFromPoint doesn't return
> them.
> Why that behavior, I don't know. That is a question to the layout folks.

Interesting. I'm not sure whom from the layout team to ask directly. David, can you help us on that question please? Thanks.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> (In reply to Olli Pettay [:smaug] from comment #16)
> > tr elements are in the tree, but looks like elementsFromPoint doesn't return
> > them.
> > Why that behavior, I don't know. That is a question to the layout folks.
> 
> Interesting. I'm not sure whom from the layout team to ask directly. David,
> can you help us on that question please? Thanks.

I suspect this would be fixed by adding code that does roughly:

  if (aBuilder->IsForEventDelivery()) {
    nsDisplayBackgroundImage::AppendBackgroundItemsToTop(aBuilder, this, GetRectRelativeToSelf(), aLists.BorderBackground());
  }

or similar to nsTableRowFrame::BuildDisplayList (and the same for nsTableRowGroupFrame... the interesting question is whether to do the same for nsTableColFrame and nsTableColGroupFrame).  Though a little bit more might be needed to make it actually work.  (You can see similar code at at least some if not many of the uses of https://searchfox.org/mozilla-central/search?q=IsForEventDelivery&path= .)

This code isn't there already because table rows aren't responsible for drawing their own background; it's drawn via the cells.  But nsLayoutUtils::GetFramesForArea() uses a display list builder configured for event delivery.

The other question is then whether there are other uses of display list builders for event delivery that rely on not having table rows (etc.).  I hope not, but it seems like something to consider.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

whimboo, could you file a bug about that <tr> issue?

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Olli Pettay [:smaug] from comment #19)
> whimboo, could you file a bug about that <tr> issue?

Sure, I was just away a couple of days. Did that now and filed bug 1413493.

For now I wonder how to workaround this behavior. Should we may try to use the first <td> node instead if it exists to determine if the table row is in view? Olli, what would you say?

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

I don't know if having td in view guarantees that the tr is in view too in all the corner cases, but
it is probably good enough approximation.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1406505 - Fix element.isInView to use table cells instead of rows.

As long as bug 1413493 has not been fixed the <tr> nodes will not be part
of the element tree. To workaround this problem we could use the first
found cell of the given row to check for visibility.

Review commit: https://reviewboard.mozilla.org/r/195424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195424/

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1406505 - Improve click tests for both legacy and wdspec compliant clicks.

Some of the tests do not check if the click() was successful. To prove that
the click methods work, such a check always has to happen.

Also some tests should be executed in both the legacy and webdriver compliant
mode to ensure there are no regressions. Affected tests have been moved around.

Review commit: https://reviewboard.mozilla.org/r/195426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195426/

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8920618 [details]
Bug 1406505 - Fix element.getContainer() to return button as container.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191612/diff/2-3/

Comment 25

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8924208 [details]
Bug 1406505 - Fix element.isInView to use table cells instead of rows.

https://reviewboard.mozilla.org/r/195424/#review200594

::: testing/marionette/element.js:1047
(Diff revision 1)
> -  return doc.elementsFromPoint(centre.x, centre.y);
> +  let tree = doc.elementsFromPoint(centre.x, centre.y);
> +
> +  return tree;

Pointless assignment.

Comment 26

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8924209 [details]
Bug 1406505 - Improve click tests for both legacy and wdspec compliant clicks.

https://reviewboard.mozilla.org/r/195426/#review200598

Comment 27

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8920618 [details]
Bug 1406505 - Fix element.getContainer() to return button as container.

https://reviewboard.mozilla.org/r/191612/#review197252

> Ups! I put in the wrong bug id as reference. What I actually meant to add here is bug 1089326. Does that solve the question? It's really a Firefox behavior and affects both methods.

OK.

Comment 28

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8920618 [details]
Bug 1406505 - Fix element.getContainer() to return button as container.

https://reviewboard.mozilla.org/r/191612/#review200644

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8924208 [details]
Bug 1406505 - Fix element.isInView to use table cells instead of rows.

https://reviewboard.mozilla.org/r/195424/#review200594

> Pointless assignment.

Sorry, left-over code from debugging. Thanks for catching that.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8924208 [details]
Bug 1406505 - Fix element.isInView to use table cells instead of rows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195424/diff/1-2/

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8924209 [details]
Bug 1406505 - Improve click tests for both legacy and wdspec compliant clicks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195426/diff/1-2/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23c624afffb0
Fix element.getContainer() to return button as container. r=ato
https://hg.mozilla.org/integration/autoland/rev/a7d85e5e585f
Fix element.isInView to use table cells instead of rows. r=ato
https://hg.mozilla.org/integration/autoland/rev/b475379e4bff
Improve click tests for both legacy and wdspec compliant clicks. r=ato

Comment 33

[Cristina Coroiu [:ccoroiu]]

[task 2017-11-01T21:01:16.870Z] 21:01:16  WARNING -  TEST-UNEXPECTED-FAIL | test_click.py TestClick.test_inclusive_descendant | AssertionError: -1 == -1 

Bug is only failing on Android 4.3 API16+ debug

This is the log: https://treeherder.mozilla.org/logviewer.html#?job_id=141450131&repo=autoland&lineNumber=1999

:whimboo, could you please take a look?

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Yes, I filed bug 1413821 to take care of it. Maybe we just have to remove this assertion for now, or skip the test on Android.

Comment 35

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/23c624afffb0
https://hg.mozilla.org/mozilla-central/rev/a7d85e5e585f
https://hg.mozilla.org/mozilla-central/rev/b475379e4bff

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please uplift this Marionette related patch to mozilla-beta. It adds workarounds for Firefox bugs, which are needed to get the webdriver conformant click to work.

When uplifting please land the patch on bug 413821 first.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Henrik Skupin (:whimboo) from comment #36)
> When uplifting please land the patch on bug 413821 first.

This should actually have been bug 1413821.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7ce2c3117176 (FIREFOX_57b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-beta/rev/d4ede1341fbf (FIREFOX_57b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-beta/rev/4cab93886f10 (FIREFOX_57b_RELBRANCH)

https://hg.mozilla.org/releases/mozilla-release/rev/4ea91858ca5f
https://hg.mozilla.org/releases/mozilla-release/rev/e2c27f238172
https://hg.mozilla.org/releases/mozilla-release/rev/b5602fdcc2bc