Closed Bug 1405474 Opened 7 years ago Closed 7 years ago

provide more information about overlapping element on click

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox57 fixed, firefox58 fixed)

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

People

(Reporter: akostadinov, Assigned: whimboo)

References

Details

Attachments

(1 file)

Bug 1405474 - Add more attributes for elements in pprint output.
59 bytes, text/x-review-board-request
ato
: review+
Details 
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170901071738

Steps to reproduce:

* use webdriver conforming click (as describer in bug 1321516)
* click button 5 on page https://bugzilla.mozilla.org/attachment.cgi?id=8909704


Actual results:

There is an error message (because the button ends up under a footer) which is not descriptive enough to identify the element intercepting the click:

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


Expected results:

An error message that *is* descriptive enough to identify the element intercepting the click (include all element attributes and partial element text):

> #<Selenium::WebDriver::Error::ElementClickInterceptedError: Element <button id=\"button5\"> is not clickable at point (635,869) because another element <a href="#link">Overlay L...</a> obscures it>\n"
Please keep in mind that this could be really long, also child nodes could exist, but which we might not want to include in the error message.
Blocks: 1321516
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 1.9.0 Branch → unspecified
Agreed, there should be some character limit. But I think at least all attributes and a number of characters from the element text would be very useful. I see some attributes are returned, but the `href` seems not.

Ideally the element handle would be also returned so one can try to get around it in code automatically. I'd have to file a webdriver spec issue for this AFAICT.
So I wonder what would actually be good candidates here. Something which comes into my mind to make it more clear for those nodes which do not have an id or class assigned:

* <a> nodes should contain the `href` property
* <img> nodes should contain the `src` property (would also apply for other node types)
* <input> nodes would benefit from `name` and `type`

I think the above should give enough information right now. Anything else I missed? 

(In reply to Alexander from comment #2)
> Ideally the element handle would be also returned so one can try to get
> around it in code automatically. I'd have to file a webdriver spec issue for
> this AFAICT.

I wouldn't include that right now. A separate bug should be filed if agreed on.
The above sounds good but I think for links it may not be enough. In recent dynamic websites often links act by triggering events, not by having some unique href specified. If we want to whilelist tags depending on element types, perhaps at least `class` needs to always be included I think.
id and class is already included by default when using `pprint`, which my patch will add for the obscured element. Sorry for not mentioning that.
Comment on attachment 8919868 [details]
Bug 1405474 - Add more attributes for elements in pprint output.

https://reviewboard.mozilla.org/r/190804/#review196038

I think it’s a good idea to include more identification
properties.  Possibly it would be possible to adapt format.truncate
so that the max length could be limited in case we wanted to also
include innerText like the bug suggests.

However we should use identifiers that reflect the current state of
the DOM, which means we should look at the element’s properties
instead of its HTML attributes.

::: testing/marionette/format.js:48
(Diff revision 2)
> -      idents = " " + ident.join(" ");
> +      if (el.hasAttribute(attr)) {
> +        idents += ` ${attr}="${el.getAttribute(attr)}"`;
> +      }

This changes pprint from getting the id and className properties
to getting the attributes.  I think we should continue to use
properties.
Attachment #8919868 - Flags: review?(ato) → review-
Comment on attachment 8919868 [details]
Bug 1405474 - Add more attributes for elements in pprint output.

https://reviewboard.mozilla.org/r/190804/#review196038

> This changes pprint from getting the id and className properties
> to getting the attributes.  I think we should continue to use
> properties.

I read about `class` and `className` and found the following on MDN:

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

> className can also be an instance of SVGAnimatedString if the element is an SVGElement. It is better to get/set the className of an element using Element.getAttribute and Element.setAttribute if you are dealing with SVG elements 

So at least in some cases we would have to use the has/getAttribute methods. But why do you think we have to keep by using properties only? Doesn't the attribute also reflect the change of eg. the elements `id`? I don't understand the drawback by not using the property, given that attributes reflect the state of the property.

I can see that it will not work for `innerText`, and maybe we could use a fallback to use the property if the attribute is not present? At least this would solve my problem with `class` vs. `className`, where the former is most likely better known.
Comment on attachment 8919868 [details]
Bug 1405474 - Add more attributes for elements in pprint output.

https://reviewboard.mozilla.org/r/190804/#review196038

> I read about `class` and `className` and found the following on MDN:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Element/className
> 
> > className can also be an instance of SVGAnimatedString if the element is an SVGElement. It is better to get/set the className of an element using Element.getAttribute and Element.setAttribute if you are dealing with SVG elements 
> 
> So at least in some cases we would have to use the has/getAttribute methods. But why do you think we have to keep by using properties only? Doesn't the attribute also reflect the change of eg. the elements `id`? I don't understand the drawback by not using the property, given that attributes reflect the state of the property.
> 
> I can see that it will not work for `innerText`, and maybe we could use a fallback to use the property if the attribute is not present? At least this would solve my problem with `class` vs. `className`, where the former is most likely better known.

> So at least in some cases we would have to use the
> has/getAttribute methods. But why do you think we have to keep
> by using properties only? Doesn't the attribute also reflect the
> change of eg. the elements id? I don't understand the drawback by
> not using the property, given that attributes reflect the state of
> the property.

Right, so for a large subset of attributes it is true that the
content attribute (property) bear no reflection of the IDL
attribute.  This is true for the selection of attributes you have
chosen, so I guess it is fine to use getAttribute for these.

Resolving this issue.

> I can see that it will not work for innerText, and maybe we
> could use a fallback to use the property if the attribute is not
> present? At least this would solve my problem with class vs.
> className, where the former is most likely better known.

I don’t think a fallback approach makes any sense because the
attributes/properties we need to look at needs to be specific to
the element prototype we are looking at.  See the new issues I’ve
raised.

It would also be fine to include innerText at a later stage, as it
was merely a suggestion for how to approach the problem from my side.
Comment on attachment 8919868 [details]
Bug 1405474 - Add more attributes for elements in pprint output.

https://reviewboard.mozilla.org/r/190804/#review196324

::: testing/marionette/format.js:24
(Diff revision 2)
>   *     pprint`Expected boolean, got ${bool}`;
>   *     => 'Expected boolean, got [object Object] {"value": true}'
>   *
>   *     let htmlElement = document.querySelector("input#foo");
>   *     pprint`Expected element ${htmlElement}`;
> - *     => 'Expected element <input id="foo" class="bar baz">'
> + *     => 'Expected element <input id="foo" class="bar baz" type="input">'

No such type of <input>.

::: testing/marionette/format.js:44
(Diff revision 2)
>      }
>      return proto + " " + s;
>    }
>  
>    function prettyElement(el) {
> -    let ident = [];
> +    let attrs = ["id", "class", "href", "name", "src", "type"];

id and class are, I think, generic to all HTML elements, but href/name/src/type applies only to a subset.

Because you’ve chosen to use getAttribute to get them there is additional need to make sure the element prototype is the correct one, since in theory, any element may have attributes with these names.

::: testing/marionette/test_format.js:33
(Diff revision 2)
> +    hasAttribute: attr => attr in el,
> +    getAttribute: attr => attr in el ? el[attr] : null,
>      nodeType: 1,
>      localName: "input",
>      id: "foo",
> -    classList: {length: 1},
> +    class: "a b",

The property storing the class is called className, so ideally you would maintain a separate binding between IDL- and content attributes.  Or maybe it doesn’t matter specifically in this context?

::: testing/marionette/test_format.js:39
(Diff revision 2)
> -    className: "bar baz",
> +    href: "#",
> +    name: "bar",
> +    src: "s",
> +    type: "t",
>    };
> -  equal('<input id="foo" class="bar baz">', pprint`${el}`);
> +  equal('<input id="foo" class="a b" href="#" name="bar" src="s" type="t">',

We shouldn’t display an <input>’s href or src.
Comment on attachment 8919868 [details]
Bug 1405474 - Add more attributes for elements in pprint output.

https://reviewboard.mozilla.org/r/190804/#review196324

> id and class are, I think, generic to all HTML elements, but href/name/src/type applies only to a subset.
> 
> Because you’ve chosen to use getAttribute to get them there is additional need to make sure the element prototype is the correct one, since in theory, any element may have attributes with these names.

No, that's why I use `hasAttribute` here to check if that attribute really exists. In difference to `getAttribute` which returns null for non-existent attributes, `hasAttribute` only returns true if the attribute really exists.

Also why should we limit href/name/src/type to only a subset? If those attributes are present for an element we should clearly make use of them. As more as we can add as easier it will become to identify the referenced element.

> The property storing the class is called className, so ideally you would maintain a separate binding between IDL- and content attributes.  Or maybe it doesn’t matter specifically in this context?

This is just a test, so I did not want to make it that complicated. Maybe adding a comment here would make it easier to understand?

> We shouldn’t display an <input>’s href or src.

Why not? If nothing else is available and only such attributes have been set for a couple of input fields, how would you otherwise find out which element it actually is? As said above, we should make the inclusion of attributes dependent on their existence but not the type of element.
Comment on attachment 8919868 [details]
Bug 1405474 - Add more attributes for elements in pprint output.

https://reviewboard.mozilla.org/r/190804/#review196324

> No, that's why I use `hasAttribute` here to check if that attribute really exists. In difference to `getAttribute` which returns null for non-existent attributes, `hasAttribute` only returns true if the attribute really exists.
> 
> Also why should we limit href/name/src/type to only a subset? If those attributes are present for an element we should clearly make use of them. As more as we can add as easier it will become to identify the referenced element.

You misunderstand my point.  A hyperlink can have a "src" attribute
without that attribute being relevant to the function of the
element.  For the following HTML document, it makes no sense to
display the attributes that are irrelevant:

> <a href="…" src="https://sny.no/t/2017.gif" type="radio">foo</a>

> This is just a test, so I did not want to make it that complicated. Maybe adding a comment here would make it easier to understand?

Leave it as it is is fine, I guess.

> Why not? If nothing else is available and only such attributes have been set for a couple of input fields, how would you otherwise find out which element it actually is? As said above, we should make the inclusion of attributes dependent on their existence but not the type of element.

href or src on <input> doesn’t make any sense.  It’s just noise.
Comment on attachment 8919868 [details]
Bug 1405474 - Add more attributes for elements in pprint output.

https://reviewboard.mozilla.org/r/190804/#review196324

> You misunderstand my point.  A hyperlink can have a "src" attribute
> without that attribute being relevant to the function of the
> element.  For the following HTML document, it makes no sense to
> display the attributes that are irrelevant:
> 
> > <a href="…" src="https://sny.no/t/2017.gif" type="radio">foo</a>

Are you really proposing that we should keep maintaining a list of all the available element types, and which identifiers are allowed, and are unique to them? I see that as a very huge task with basically no real value for the output we want to do here.

Please keep in mind that the example you gave above will most likely not be present on the web. No-one would put a `src` attribute on a <a> element, because it's also not defined in the HTML spec. But if someone really does it will certainly be of help eg. for this example: `<a onclick="…" src="…">`. In such a case the src attribute could be used to better identify the link. Beside that please note that `type` is a valid attribute for <a> too, so if specified it will be helpful as well.

> href or src on <input> doesn’t make any sense.  It’s just noise.

If you are going to argue that this example is wrong, I can change it to not use an <input> element. The test here simply ensures that all the wanted attributes are getting added. I could even add another test which ensures that if an attribute has not been set it will not be added.
We talked about this topic in our chitchat and agreed to just land the patches. Then we want to file a follow-up to make the list of attributes dynamic.

Andreas, can you please set the r+ so I can land? Thanks.
Flags: needinfo?(ato)
Comment on attachment 8919868 [details]
Bug 1405474 - Add more attributes for elements in pprint output.

https://reviewboard.mozilla.org/r/190804/#review198196

whimboo and I discussed this change and we agreed it was the right
thing to do to accept it as-is.  It is better than the current
situation and helps users diagnose the problem to a better degree.

However, for the future, we want to enumerate all attributes on DOM
nodes when printing them, and I filed https://bugzil.la/1411634
about this.
Attachment #8919868 - Flags: review- → review+
Flags: needinfo?(ato)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3071d20a77e7
Add more attributes for elements in pprint output. r=ato
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/3071d20a77e7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Please uplift this test-only patch to mozilla-beta. Thanks.
Whiteboard: [checkin-needed-beta]
Needs a rebased patch.
Whiteboard: [checkin-needed-beta]
The patch didn't apply because on mozilla-central the pprint template got moved from error.js to format.js. I updated the patch and pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f542b4919bd81372a632289dc64044fb16cf42f6
The try job passed. So please uplift the following changeset to mozilla-beta:

https://hg.mozilla.org/try/rev/2101db5e488217a23817fdfe43531f46ac3e41ab
Whiteboard: [checkin-needed-beta]
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: