1530110 - element.isInView throws for non-HTML elements.

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Because it uses element.style without checking first, and element.style is only available in HTML and SVG elements:

https://drafts.csswg.org/cssom/#the-elementcssinlinestyle-interface

This is why the test in https://github.com/web-platform-tests/wpt/pull/15530 fails in Firefox.

Comment 1

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

Here the stack trace from that github PR:

UnknownException: TypeError: el.style is undefined
stacktrace:
element.isInView@chrome://marionette/content/element.js:1065:7
webdriverClickElement@chrome://marionette/content/interaction.js:148:8
interaction.clickElement@chrome://marionette/content/interaction.js:130:11
clickElement/<@chrome://marionette/content/listener.js:1208:14
navigate/<@chrome://marionette/content/listener.js:408:13
navigate@chrome://marionette/content/listener.js:407:13
clickElement@chrome://marionette/content/listener.js:1207:5

It's being used for getting/setting el.style.pointerEvents. Do MathML documents don't have pointer events, or would we only have to change the logic in how to get/set the pointer event type?

If we cannot use pointer events auto in MathML how could we get the pointer-interactable paint tree? Maybe it's not necessary for MathML?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

They do have style, and thus pointer events, just no inline style interface... It'd be fairly weird to specify pointer-events in MathML I guess...

Comment 3

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

I don't know all the details about MathML. So feedback about that would be welcome.

The affected code in Marionette looks like the following:
https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/testing/marionette/element.js#1060-1077

Any proposal from your side in what we would have to change?

Comment 4

[Frédéric Wang (:fredw)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #3)

I don't know all the details about MathML. So feedback about that would be welcome.

The affected code in Marionette looks like the following:
https://searchfox.org/mozilla-central/rev/dbddac86aadf1d4871fb350bbe66db43728a9f81/testing/marionette/element.js#1060-1077

Any proposal from your side in what we would have to change?

I expect this can be fixed by bug 1571487.

However, I wonder if what you want is actually to use window.getComputedStyle to get the exact style of the element rather than the inline style (i.e. set by the style attribute). See https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style and https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle

If so, I can try to upload a patch.

Comment 5

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

Interesting. So there is also this paragraph on MDN, which shows that we even would not get all the final values for CSS properties, and as such using Element.style could have wrong behavior:

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/style#Getting_style_information

To get the values of all CSS properties for an element you should use Window.getComputedStyle() instead.

Andreas, do you know why that code doesn't use Window.getComputedStyle() but Element.style instead? If it is just old and crufty code, and would appreciate a rewrite?

Comment 6

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #5)

[D]o you know why that code doesn't use
Window.getComputedStyle() but Element.style instead? If it is
just old and crufty code, and would appreciate a rewrite?

That looks like an oversight: it should definitely be using the
computed style!

I want to also point out that this try…catch logic with modifying
the document is not ideal. If anyone knows of a better way to get
elements with pointer-events: none set included in the paint
tree/elementsFromPoint(), then please speak up.

Comment 7

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

Andreas, mind filing the topic of the 2nd paragraph as a new bug? I don't think that we should shovel it into this bug.

Frédéric Wang, if you could create a patch for using getComputedStyle() that would be really appreciated. Maybe you could also add a small MathML testcase (as best as data URI) so we can verify it?

Comment 8

[Andreas Tolfsen ❲:ato❳]

bug 1575510

Comment 9

[Frédéric Wang (:fredw)]

Attachment: bug-1530110.patch

OK, I stand corrected: https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleDeclaration says that Window.getComputedStyle() returns a read-only interface so you can read the original value but won't be able to use it to set the inline style anyway. One would have to do something more complicate with setAttribute('style', '...') instead. I think it will be best to implement the style IDL attribute on MathML elements instead (bug 1571487).

In any case I'm attaching a patch with a tentative marionette test and the MathML href test that is expected to be fixed.

Comment 10

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

Thanks. Marking bug 1571487 as dependency then.

Comment 11

[Frédéric Wang (:fredw)]

Attachment: Bug 1530110 - element.isInView throws for non-HTML elements. r=whimboo

Marionette's element.isInView function relies on the style IDL attribute.
This patch verifies that the function works as expected for MathML elements
after bug 1571487.

Comment 12

[Frédéric Wang (:fredw)]

For MathML this is going to be fixed by 1571487. I uploaded a patch to add a marionette test for MathML. I think things will work for HTML, SVG, MathML and XUL elements since they support the style IDL attribute. Not sure we really care about this issue for other elements....

Comment 13

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

Great to see. Thanks a lot. We can already do the review while waiting for the other bug to be fixed.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21e3e7f5b1ff
element.isInView throws for non-HTML elements. r=whimboo

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/21e3e7f5b1ff