Closed Bug 1406505 Opened 7 years ago Closed 7 years ago

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

Categories

(Remote Protocol :: Marionette, defect, P2)

Product:
Remote Protocol
Component:
Marionette
Type:
defect

Tracking

(firefox57 fixed, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: juangj, Assigned: whimboo)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [17/11])

Attachments

(5 files, 1 obsolete file)

Selenium test that illustrates that the old click works
976 bytes, text/plain
Details
Bug 1406505 - Fix element.getContainer() to return button as container.
59 bytes, text/x-review-board-request
ato
: review+
Details
HTML test page
631 bytes, text/html
Details
Bug 1406505 - Fix element.isInView to use table cells instead of rows.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1406505 - Improve click tests for both legacy and wdspec compliant clicks.
59 bytes, text/x-review-board-request
ato
: review+
Details 
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",
  ...
}
Blocks: 1321516
Attachment #8916103 - Attachment mime type: text/x-python → text/plain
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.
Status: UNCONFIRMED → NEW
status-firefox57: --- → affected
status-firefox58: --- → affected
Ever confirmed: true
Priority: -- → P2
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
Flags: needinfo?(bugs)
Summary: WebDriver click regression → element.isInView() fails, because a span node inside a button is not returned by elementsFromPoint
Attached file HTML test page (obsolete) — 
This simple test page demonstrates the problem. Just move around and click at the wanted position.
Attachment #8920335 - Attachment mime type: text/plain → text/html
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.
Flags: needinfo?(ato)
In Firefox <button> is the hit testing target, not any elements inside it. We're considering changing that.
Flags: needinfo?(bugs)
(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.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Flags: needinfo?(ato) → needinfo?(bugs)
I guess it is bug 1089326
Flags: needinfo?(bugs)
That sounds plausible. I will add it to the dependency list so that we are getting notified when the change got implemented. Thanks.
Depends on: 1089326
Andreas, this patch is WIP but I would like to hear an early feedback from you.
Flags: needinfo?(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.
(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.
Flags: needinfo?(ato)
Attached file 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?
Attachment #8920335 - Attachment is obsolete: true
Flags: needinfo?(bugs)
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.
(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.
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(dbaron)
Summary: element.isInView() fails, because a span node inside a button is not returned by elementsFromPoint → element.isInView() fails, because child nodes inside a button, or tr nodes are not returned by elementsFromPoint
(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.
Flags: needinfo?(dbaron)
whimboo, could you file a bug about that <tr> issue?
Depends on: 1413493
(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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Attachment #8924208 - Flags: review?(ato)
Attachment #8924209 - Flags: review?(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.
Attachment #8924208 - Flags: review?(ato) → review+
Comment on attachment 8924209 [details]
Bug 1406505 - Improve click tests for both legacy and wdspec compliant clicks.

https://reviewboard.mozilla.org/r/195426/#review200598
Attachment #8924209 - Flags: review?(ato) → review+
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 on attachment 8920618 [details]
Bug 1406505 - Fix element.getContainer() to return button as container.

https://reviewboard.mozilla.org/r/191612/#review200644
Attachment #8920618 - Flags: review+
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.
Pushed by hskupin@mozilla.com:
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
[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?
Flags: needinfo?(hskupin)
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.
Depends on: 1413821
Flags: needinfo?(hskupin)
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.
Whiteboard: [checkin-needed-beta]
(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.
Whiteboard: [checkin-needed-beta] → [checkin-needed-beta][uplift bug 1413821 first]
Whiteboard: [17/11]
Attachment #8922225 - Attachment mime type: text/plain → text/html
Blocks: 1448825
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: