1080764 - Add support for anonymous nodes in chrome

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

We need support for anonymous chrome nodes for conversion of mozmill tests to marionette.

Comment 1

[Andrew Halberstadt [:ahal]]

After some initial investigation, I think the best way to approach this is to:

1) Create a new find_element method where the user can pass in a dict of arbitrary attributes that returns elements for which all attributes match.

2) Create a DOMElement.find_elements(<method>, <search str>) function that is the same as the global marionette one, except only searches child nodes of the root element.

3) Create a library similar to gaiatest except for Firefox that makes it easier to find and manipulate commonly used elements (like the tab strip). I think such a library should live in its own repo outside of both marionette-core and the marionette-release-tests.

With 1) and 2) it should be possible to build 3). I like this approach because it avoids the need for some sort of lookup expression syntax.

Please let me know if this makes sense or if there's a better approach, I'm still pretty new to this marionette stuff!

Comment 2

[Andrew Halberstadt [:ahal]]

We should also keep an eye on bug 967201. Depending on the cause of that bug, there may be some code somewhere in devtools land that already returns a unique selector for anonymous nodes. Tapping into that could be very helpful.

Comment 3

[Andrew Halberstadt [:ahal]]

Another thing that would be helpful would be supporting "css selector" in chrome scope (especially if bug 967201 gets fixed). If we could get that working, 1) and 2) wouldn't actually be necessary (though I still think they would be helpful).

p.s The code that finds a unique css selector given an element lives here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/styleinspector/css-logic.js#917

Comment 4

[Andrew Halberstadt [:ahal]]

Heh, looks like 2) is already done \o/

Comment 5

[Jonathan Griffin (:jgriffin)]

(In reply to Andrew Halberstadt [:ahal] from comment #1)
> After some initial investigation, I think the best way to approach this is
> to:
> 
> 1) Create a new find_element method where the user can pass in a dict of
> arbitrary attributes that returns elements for which all attributes match.
> 
This all sounds good.  For this one, I think we can just add a new 'method' to the existing find_element call, to tell it that it's dealing with anonymous nodes.  This way we don't have to increase the API space.

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#61

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-elements.js#261

Comment 6

[Andrew Halberstadt [:ahal]]

Attachment: find_by_anon

This patch simply adds two new "find methods" that map directly to elem.getAnonymousNodes() and elem.getAnonymousElementByAttribute() directly.

A potentially controversial change is that this make the "target" attribute of find_element() optional. This is because "getAnonymousNodes()" just returns all anonymous children, there are no terms to search for. If this is not acceptable, we could alternatively either:
A) Not support getAnonymousNodes ("anon attribute" is probably good enough for most use cases)
B) Pass in None explicitly

"anon attribute" isn't supported by find_elements() because getAnonymousElementByAttribute only ever returns a single element.

Here's a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=272a46c8b997

Comment 7

[Martijn Wargers (dead)]

This won't work for the video controls in the video tag, will it?
Because those are added as a native anonymous element, which has a -moz-binding attached to it.
I worked out a way in bug 1047168, comment 32, on how to access those.

Comment 8

[Andrew Halberstadt [:ahal]]

The last try run didn't run the Gij tests under b2g desktop due to a trychooser bug. Here's a new run that will catch them:
https://tbpl.mozilla.org/?tree=Try&rev=8f32a45a9420

Also here's a re-run of the Win7 orange with the patch in bug 1084412 to try and see if this is a newly introduced intermittent or not:
https://tbpl.mozilla.org/?tree=Try&rev=8bc01631b03d

Comment 9

[Andrew Halberstadt [:ahal]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #7)
> This won't work for the video controls in the video tag, will it?
> Because those are added as a native anonymous element, which has a
> -moz-binding attached to it.
> I worked out a way in bug 1047168, comment 32, on how to access those.

No, I don't think it will. It looks like getAnonymousNodes and getAnonymousElementByAttribute are only implemented by XULDocuments, not HTMLDocuments. I guess this raises a good question though of whether the "anon" and "anon attribute" search methods should be available in content. I guess content XUL is technically possible, but I don't know if we care about supporting it.

Comment 10

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

(In reply to Andrew Halberstadt [:ahal] from comment #9)
> "anon" and "anon attribute" search methods should be available in content. I
> guess content XUL is technically possible, but I don't know if we care about
> supporting it.

We have to support it given that all the about: pages are actually XUL documents living in content. What we do not have to support are remote XUL documents because those won't work anymore in Firefox.

Comment 11

[Martijn Wargers (dead)]

One can turn on XUL and XBL for remote documents. And there is the marquee tag, that's using an XBL binding, which works on all html documents.

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: Add 'anon' and 'anon attribute' strategies to marionette's findElement

This patch does some minor cleanup, but nothing major. All the context from comment 6 still applies.

Try run for Gij:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3c591d279e6e

General try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a53da7fab9eb

Comment 13

[David Burns :automatedtester]

Comment on attachment 8511106 [details] [diff] [review]
Add 'anon' and 'anon attribute' strategies to marionette's findElement

Review of attachment 8511106 [details] [diff] [review]:
-----------------------------------------------------------------

great patch, just a few things that I would like to be cleaned up

::: testing/marionette/client/marionette/marionette.py
@@ +48,5 @@
>  
>      def __eq__(self, other_element):
>          return self.id == other_element.id
>  
> +    def find_element(self, method, target=None):

please can we have this as a normal argument. I would like people to be explicit with what they are searching for even it is None in this case

@@ +57,5 @@
>          Marionette class.
>          '''
>          return self.marionette.find_element(method, target, self.id)
>  
> +    def find_elements(self, method, target=None):

please can we have this as a normal argument. I would like people to be explicit with what they are searching for even it is None in this case

@@ +1268,5 @@
>                                        line=int(frame[1]),
>                                        filename=os.path.basename(frame[0]))
>          return self.unwrapValue(response)
>  
> +    def find_element(self, method, target=None, id=None):

please can we have this as a normal argument. I would like people to be explicit with what they are searching for even it is None in this case

@@ +1297,5 @@
>          response = self._send_message('findElement', 'value', **kwargs)
>          element = HTMLElement(self, response['ELEMENT'])
>          return element
>  
> +    def find_elements(self, method, target=None, id=None):

please can we have this as a normal argument. I would like people to be explicit with what they are searching for even it is None in this case

::: testing/marionette/client/marionette/tests/unit/test_anonymous_content.py
@@ +37,5 @@
> +    def test_find_anonymous_element_by_attribute(self):
> +        el = self.marionette.find_element("id", "dia")
> +        self.assertEquals(HTMLElement, type(el.find_element("anon attribute", {"anonid": "buttons"})))
> +        with self.assertRaises(MarionetteException):
> +            el.find_elements("anon attribute", {"anonid": "buttons"})

findElements shouldn't throw an exception, it should return an empty array unless this is supposed to be findElement

@@ +38,5 @@
> +        el = self.marionette.find_element("id", "dia")
> +        self.assertEquals(HTMLElement, type(el.find_element("anon attribute", {"anonid": "buttons"})))
> +        with self.assertRaises(MarionetteException):
> +            el.find_elements("anon attribute", {"anonid": "buttons"})
> +

It would be awesome to have a negative test like the above for find_element

Comment 14

[Andrew Halberstadt [:ahal]]

(In reply to David Burns :automatedtester from comment #13)
> great patch, just a few things that I would like to be cleaned up

Thanks, makes sense I'll get a new patch up with the changes. Just one thing I need re-clarification on..

> ::: testing/marionette/client/marionette/tests/unit/test_anonymous_content.py
> @@ +37,5 @@
> > +    def test_find_anonymous_element_by_attribute(self):
> > +        el = self.marionette.find_element("id", "dia")
> > +        self.assertEquals(HTMLElement, type(el.find_element("anon attribute", {"anonid": "buttons"})))
> > +        with self.assertRaises(MarionetteException):
> > +            el.find_elements("anon attribute", {"anonid": "buttons"})
> 
> findElements shouldn't throw an exception, it should return an empty array
> unless this is supposed to be findElement

The exception here isn't raised because no elements were found. It's raised because "anon attribute" can never ever return more than one element and so isn't a valid strategy to use with find_elements().

This is because it maps directly to doc.getAnonymousElementByAttribute() that only returns a single element, even if multiple exist. In the future we could probably hack around this by using doc.getAnonymousNodes() instead and then checking for attributes manually, but I'd rather do that in a follow-up (if we care about doing it at all).

At the very least I'll add a comment to the test explaining this.

Comment 15

[Andrew Halberstadt [:ahal]]

Attachment: Add 'anon' and 'anon attribute' strategies to marionette's findElement

Address review comments minus the one about the exception. I added a comment to the test explaining it and also filed bug 1089661 to improve our documentation around the various search strategies.

Comment 16

[David Burns :automatedtester]

Comment on attachment 8512021 [details] [diff] [review]
Add 'anon' and 'anon attribute' strategies to marionette's findElement

Review of attachment 8512021 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/marionette/client/marionette/tests/unit/test_anonymous_content.py
@@ +44,5 @@
> +            el.find_element("anon attribute", {"anonid": "nonexistent"})
> +
> +        # anon attribute is not a valid strategy to use with find_elements, make sure it raises
> +        with self.assertRaises(MarionetteException):
> +            el.find_elements("anon attribute", {"anonid": "buttons"})

This doesn't follow the rest of the semantics of the findElement/findElements. I would prefer that we keep that semantic and only return an array with 1 item

Comment 17

[Andrew Halberstadt [:ahal]]

Attachment: Add 'anon' and 'anon attribute' strategies to marionette's findElement

Sounds good. This patch updates the behaviour, docstring and test and will return an array of length 0 or q1 if find_elements("anon attribute", ...) is used.

Comment 18

[David Burns :automatedtester]

Comment on attachment 8512728 [details] [diff] [review]
Add 'anon' and 'anon attribute' strategies to marionette's findElement

Review of attachment 8512728 [details] [diff] [review]:
-----------------------------------------------------------------

awesome work Andrew :)

Comment 19

[Andrew Halberstadt [:ahal]]

The try runs from comment 12 look good and the patch hasn't changed all that much since then..

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bde606f4d52

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8bde606f4d52