I have seen this today when working on the base dialog implementation necessary for the old software update dialog in Firefox. Here we need references for the default buttons in a dialog, e.g. accept button. Stacktrace: File "/mozilla/code/firefox/nightly/testing/marionette/client/marionette/marionette_test.py", line 296, in run testMethod() File "/mozilla/code/firefox-ui-tests/firefox_puppeteer/tests/test_dialogs.py", line 29, in test_basic print dialog.accept File "/mozilla/code/firefox-ui-tests/firefox_puppeteer/ui/dialogs.py", line 27, in accept return self.marionette.find_element(By.ANON_ATTRIBUTE, {'dlgtype': 'accept'}) File "/mozilla/code/firefox/nightly/testing/marionette/driver/marionette_driver/marionette.py", line 1518, in find_element response = self._send_message('findElement', 'value', **kwargs) File "/mozilla/code/firefox/nightly/testing/marionette/driver/marionette_driver/decorators.py", line 36, in _ return func(*args, **kwargs) File "/mozilla/code/firefox/nightly/testing/marionette/driver/marionette_driver/marionette.py", line 717, in _send_message self._handle_error(response) File "/mozilla/code/firefox/nightly/testing/marionette/driver/marionette_driver/marionette.py", line 802, in _handle_error raise errors.MarionetteException(message=message, status=status, stacktrace=stacktrace) MarionetteException: MarionetteException: Argument 1 of Document.getAnonymousElementByAttribute does not implement interface Element. stacktrace: EM_findElement@chrome://marionette/content/marionette-elements.js:595:19 EM_find@chrome://marionette/content/marionette-elements.js:449:23 MDA_findElement@chrome://marionette/content/marionette-server.js:1928:14 MSC_onPacket@chrome://marionette/content/marionette-server.js:208:9 DebuggerTransport.prototype._onJSONObjectReady/<@resource://gre/modules/devtools/dbg-client.jsm -> resource://gre/modules/devtools/transport/transport.js:471:9 makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14 makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14 You can test this by doing the following steps: git pull https://github.com/whimboo/firefox-ui-tests.git dialog firefox-ui-tests --binary /mozilla/bin/nightly/firefox firefox_puppeteer/tests/test_dialogs.py This bug blocks us from getting the fallback software update test implemented. Chris, would you have time to check this today?

Sure I'll check it out.

Chris and I talked on IRC and the problem is that getAnonymousElementByAttribute() needs a parent element specified. By using self.marionette.find_element() there is none, so this method fails. We can get this fixed when Marionette would use the root element of a window as parent: root = self.marionette.find_element(By.CSS_SELECTOR, ':root') This is no longer blocking us, but would be a nice enhancement. This is in use in our Mozmill tests for years and makes it way easier to handle.

We can add code in https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1648 to do something like if By.ANON_ATTRIBUTE root = self.marionette.find_element(By.CSS_SELECTOR, ':root') id = root.id

Hi! I would like to work on this bug.

(In reply to Shruti Jasoria [:ShrutiJ] from comment #4) > Hi! I would like to work on this bug. great! Could you update as mentioned in comment 3 and when there is a patch attached I will assign the bug to you.

I have updated marionette.py as it was asked in comment 3. Please take a look and let me know if any more changes have to be made.

Comment on attachment 8769060 [details] [diff] [review] Require additions Review of attachment 8769060 [details] [diff] [review]: ----------------------------------------------------------------- Could you also add a test to test_findelement_chrome.py to make sure you code is working as intended. To run the tests you would do ./mach marionette-test ::: testing/marionette/client/marionette_driver/marionette.py @@ +1683,5 @@ > body = {"value": target, "using": method} > if id: > body["element"] = id > + > + if body["using"] == "anon attribute": This should be above like 1684. You have populated `id` later on but never makes it into what is sent.

I have made the changes which you had asked for in comment 7. I hope its good now.

Comment on attachment 8771264 [details] [diff] [review] Required Changes and test Review of attachment 8771264 [details] [diff] [review]: ----------------------------------------------------------------- Please can you also push the next version of this patch to mozreview http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html ::: testing/marionette/harness/marionette/tests/unit/test_findelement_chrome.py @@ +81,5 @@ > self.marionette.execute_script("window.document.getElementById('things').removeChild(window.document.getElementById('myid'));") > + > + def test_anon_attribute(self): > + parent = self.marionette.find_element(By.ID, "textInput") > + found_els = parent.find_elements(By.ANON_ATTRIBUTE, {'asdf': 'qwerty'}) The point of the bug was that the finding of the parent would be done implicitly. This should be done from `self.marionette` @@ +83,5 @@ > + def test_anon_attribute(self): > + parent = self.marionette.find_element(By.ID, "textInput") > + found_els = parent.find_elements(By.ANON_ATTRIBUTE, {'asdf': 'qwerty'}) > + for el in found_els: > + assertEqual(el.id, 'root') I would be surprised if this is always root. Did you run the tests with ./mach marionette-test

Shruti, are you still working on this bug? If yes, can you please submit an updated patch? If not please also let us know so that we can find someone else. Thanks.

Sorry, I will not be able to work on this bug anymore.

I will work on this bug now so we can have it as part of the next ESR release.

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review97546 ::: testing/marionette/element.js:279 (Diff revision 1) > let rootNode = container.shadowRoot || container.frame.document; > let startNode = opts.startNode || rootNode; > > + // For anonymous nodes the start node needs to be of type DOMElement, which > + // will refer to :root in case of a DOMDocument. > + switch (strategy) { > + case element.Strategy.Anon: > + case element.Strategy.AnonAttribute: > + if (startNode instanceof Ci.nsIDOMDocument) { > + startNode = startNode.documentElement; > + } > + break; > + } If I read this code correctly, if `opts.startNode` is defined and it is not an instance of `Ci.nsIDOMElement` it will be replaced by its own property `documentElement`. This property only exists on the document element. Maybe this should be addressed in the assignment to `rootNode` instead of overriding the user’s choice of start node? I would suggest: ```js let rootNode; switch (strategy) { case element.Strategy.Anon: case element.Strategy.AnonAttribute: rootNode = container.frame.document.documentElement; break; default: rootNode = container.shadowRoot || container.frame.document; break; } let startNode = opts.startNode || rootNode; ``` Possibly this can be broken out in a separate function taking `container` and `strategy` as input: ```js let rootNode = rootNodeForStrategy_(strategy, container); ``` ::: testing/marionette/harness/marionette/chrome/test_anonymous_content.xul:6 (Diff revision 1) > -<!DOCTYPE window [ > +<!DOCTYPE dialog [ > ]> > -<window id="winTest" title="Title Test" windowtype="Test Type" > - xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > - <dialog id="dia" > + > +<dialog id="testDialogAnonymousNode" > + buttons="accept, cancel" > - xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > + xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> I’m not so well-versed in the particulars of XUL, but I’ll trust you on these ones. ::: testing/marionette/harness/marionette/tests/unit/test_anonymous_content.py:75 (Diff revision 1) > + # By using the default start node > + self.assertListEqual(buttons, > + self.marionette.find_elements(By.ANON_ATTRIBUTE, > + {"anonid": "buttons"})) > + > + self.assertListEqual([], > + self.marionette.find_elements(By.ANON_ATTRIBUTE, > + {"anonid": "nonexistent"})) This indentation seems extremely wonky even if it’s correct PEP8. Can you consider putting `buttons` on a new line?

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review97546 > If I read this code correctly, if `opts.startNode` is defined and it is not an instance of `Ci.nsIDOMElement` it will be replaced by its own property `documentElement`. This property only exists on the document element. > > Maybe this should be addressed in the assignment to `rootNode` instead of overriding the user’s choice of start node? > > I would suggest: > > ```js > let rootNode; > switch (strategy) { > case element.Strategy.Anon: > case element.Strategy.AnonAttribute: > rootNode = container.frame.document.documentElement; > break; > > default: > rootNode = container.shadowRoot || container.frame.document; > break; > } > let startNode = opts.startNode || rootNode; > ``` > > Possibly this can be broken out in a separate function taking `container` and `strategy` as input: > > ```js > let rootNode = rootNodeForStrategy_(strategy, container); > ``` Sounds like a fine and better idea. I will move it out to its own method and only act on the root node.

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review97546 > Sounds like a fine and better idea. I will move it out to its own method and only act on the root node. Well, I have to correct myself. `rootNode` should remain unchanged given that this node actually is used to call the search function. What needs to be changed is indeed the `startNode`.

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review97546 > Well, I have to correct myself. `rootNode` should remain unchanged given that this node actually is used to call the search function. What needs to be changed is indeed the `startNode`. To extend that I do not see a value in moving this code out into its own method given that this is the only place it is used. So please check your review again. Thanks.

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review97546 > To extend that I do not see a value in moving this code out into its own method given that this is the only place it is used. So please check your review again. Thanks. It’s not correct behaviour to modify the user’s input value. In this case, the user gives you an optional start node and you proceed to replace it with something else irregardless. I understand we need to pass the root node on unmodified for the search to be successful, but we shouldn’t do something different wiht the data the user sends us. This is very confusing.

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review97546 > It’s not correct behaviour to modify the user’s input value. In this case, the user gives you an optional start node and you proceed to replace it with something else irregardless. I understand we need to pass the root node on unmodified for the search to be successful, but we shouldn’t do something different wiht the data the user sends us. This is very confusing. So you are saying that we should raise an exception in case the calling code specified a nsIDOMDocument as startNode? I think that this is a valid request. I can put this check very early in this method, so we do not have to overcomplicate the switch block.

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review97546 > So you are saying that we should raise an exception in case the calling code specified a nsIDOMDocument as startNode? I think that this is a valid request. I can put this check very early in this method, so we do not have to overcomplicate the switch block. To wrap up... driver.findElement() excepts an HTMLElement as optional parameter for `cmd.parameters.element`. That one we pass through to `element.find_()`. So can the user ever pass a nsIDOMDocument in here? I don't think so. As such the above code would only be hit in the case we assign a new value via the rootNode. I can reorganize the code if you really want it.

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review98878 ::: testing/marionette/element.js:279 (Diff revision 5) > let rootNode = container.shadowRoot || container.frame.document; > let startNode = opts.startNode || rootNode; > > + switch (strategy) { > + // For anonymous nodes the start node needs to be of type DOMElement, which > + // will refer to :root in case of a DOMDocument. > + case element.Strategy.Anon: > + case element.Strategy.AnonAttribute: > + if (!opts.startNode && startNode instanceof Ci.nsIDOMDocument) { > + startNode = startNode.documentElement; > + } > + break; > + } Just to clarify: If the user _does not_ provide a start node but the root node is a `nsIDOMDocument`, then reassign `startNode` to its document element. I think the `startNode` assignment is a bit hard to reason about in this case because it is assigned once and then reassigned if two if-conditions are met. Can we consider assigning it only once? I think this should be functionally equivalent: ```js let rootNode = container.shadowRoot || container.frame.document; let startNode; switch (strategy) { case element.Strategy.Anon: case element.Strategy.AnonAttribute: if (!opts.startNode && rootNode instanceof Ci.nsIDOMDocument) { startNode = rootNode.documentElement; break; } default: startNode = opts.startNode || rootNode; break; } ``` ::: testing/marionette/harness/marionette_harness/tests/unit/test_anonymous_content.py:56 (Diff revision 5) > - el = Wait(self.marionette).until(element_present(By.ID, "dia")) > - self.assertEquals(HTMLElement, type(el.find_element(By.ANON_ATTRIBUTE, {"anonid": "buttons"}))) > + _accept_button = (By.ANON_ATTRIBUTE, {"dlgtype": "accept"},) > + _not_existent = (By.ANON_ATTRIBUTE, {"anonid": "notexistent"},) Why the underscore prefix? ::: testing/marionette/harness/marionette_harness/tests/unit/test_anonymous_content.py:59 (Diff revision 5) > + # By using the window root element > + start_node = self.marionette.find_element(By.CSS_SELECTOR, ":root") `:root` is the current browsing context’s _document element_. It’s equivalent to `document.documentElement`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_anonymous_content.py:62 (Diff revision 5) > - self.assertEquals(HTMLElement, type(el.find_element(By.ANON_ATTRIBUTE, {"anonid": "buttons"}))) > - self.assertEquals(1, len(el.find_elements(By.ANON_ATTRIBUTE, {"anonid": "buttons"}))) > + _not_existent = (By.ANON_ATTRIBUTE, {"anonid": "notexistent"},) > + > + # By using the window root element > + start_node = self.marionette.find_element(By.CSS_SELECTOR, ":root") > + button = start_node.find_element(*_accept_button) > + self.assertEquals(HTMLElement, type(button)) Use `self.assertIsInstance` if you can. ::: testing/marionette/harness/marionette_harness/tests/unit/test_anonymous_content.py:67 (Diff revision 5) > + # Not existent anon node > with self.assertRaises(NoSuchElementException): > - el.find_element(By.ANON_ATTRIBUTE, {"anonid": "nonexistent"}) > + self.marionette.find_element(*_not_existent) Perhaps also run this check given `start_node`? ::: testing/marionette/harness/marionette_harness/tests/unit/test_anonymous_content.py:72 (Diff revision 5) > + _dialog_buttons = (By.ANON_ATTRIBUTE, {"anonid": "buttons"},) > + _not_existent = (By.ANON_ATTRIBUTE, {"anonid": "notexistent"},) Again, why the underscores? ::: testing/marionette/harness/marionette_harness/tests/unit/test_anonymous_content.py:75 (Diff revision 5) > + # By using the window root element > + start_node = self.marionette.find_element(By.CSS_SELECTOR, ":root") s/window root element/document element/ ::: testing/marionette/harness/marionette_harness/tests/unit/test_anonymous_content.py:79 (Diff revision 5) > + > + # By using the window root element > + start_node = self.marionette.find_element(By.CSS_SELECTOR, ":root") > + buttons = start_node.find_elements(*_dialog_buttons) > + self.assertEquals(1, len(buttons)) > + self.assertEquals(HTMLElement, type(buttons[0])) Use `self.assertIsInstance`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_anonymous_content.py:86 (Diff revision 5) > + # By using the default start node > + self.assertListEqual(buttons, self.marionette.find_elements(*_dialog_buttons)) > + self.assertListEqual([], self.marionette.find_elements(*_not_existent)) > > def test_find_anonymous_children(self): > - el = Wait(self.marionette).until(element_present(By.ID, "dia")) > + self.assertEquals(HTMLElement, type(self.marionette.find_element(By.ANON, None))) Use `self.assertIsInstance`. ::: testing/marionette/harness/marionette_harness/tests/unit/test_anonymous_content.py:92 (Diff revision 5) > - self.assertEquals(2, len(el.find_elements(By.ANON, None))) > > - el = self.marionette.find_element(By.ID, "framebox") > + frame = self.marionette.find_element(By.ID, "framebox") > with self.assertRaises(NoSuchElementException): > - el.find_element(By.ANON, None) > - self.assertEquals([], el.find_elements(By.ANON, None)) > + frame.find_element(By.ANON, None) > + self.assertEquals([], frame.find_elements(By.ANON, None)) Does `assertEquals` do comparisons of lists properly? I suspect you may want the `assertListEqual` you used above here. (I realise it used `assertEquals` from earlier, but I think that was probably wrong.)

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review97546 > To wrap up... driver.findElement() excepts an HTMLElement as optional parameter for `cmd.parameters.element`. That one we pass through to `element.find_()`. So can the user ever pass a nsIDOMDocument in here? I don't think so. As such the above code would only be hit in the case we assign a new value via the rootNode. I can reorganize the code if you really want it. Your latest revision of this patch is better, where you look at whether `opts.startNode` is defined, and if it isn’t assign it the value of `rootNode.documentElement`. My problem was with the fact that the provided `opts.startNode` was always being discarded in favour of something different.

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review98878 > Just to clarify: If the user _does not_ provide a start node but the root node is a `nsIDOMDocument`, then reassign `startNode` to its document element. > > I think the `startNode` assignment is a bit hard to reason about in this case because it is assigned once and then reassigned if two if-conditions are met. Can we consider assigning it only once? > > I think this should be functionally equivalent: > > ```js > let rootNode = container.shadowRoot || container.frame.document; > let startNode; > switch (strategy) { > case element.Strategy.Anon: > case element.Strategy.AnonAttribute: > if (!opts.startNode && rootNode instanceof Ci.nsIDOMDocument) { > startNode = rootNode.documentElement; > break; > } > > default: > startNode = opts.startNode || rootNode; > break; > } > ``` We should really avoid such a fall-through to default. Keep in mind that other case statements might be added in the future or other if conditions for the anon nodes. Then this construct will be totally confusing and no-one will see when the default case is called. > Why the underscore prefix? I have taken this logic from all the WebQA project which use page object models. But you are right. In those cases we don't need the underscore. > Use `self.assertIsInstance` if you can. We should keep type. Otherwise subclasses of HTMLElement - if there will be any in the future - would also match. > Perhaps also run this check given `start_node`? Oh, totally! > Does `assertEquals` do comparisons of lists properly? I suspect you may want the `assertListEqual` you used above here. > > (I realise it used `assertEquals` from earlier, but I think that was probably wrong.) I will fix it while I'm on it!

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review98878 > We should really avoid such a fall-through to default. Keep in mind that other case statements might be added in the future or other if conditions for the anon nodes. Then this construct will be totally confusing and no-one will see when the default case is called. This is a valid concern. The only way I can see to avoid that is by moving the `!opts.startNode` test outside the switch statement: ```js let startNode; if (opts.startNode) { startNode = opts.startNode; } else { switch (strategy) { case element.Strategy.Anon: case element.Strategy.AnonAttribute: if (rootNode instanceof Ci.nsIDOMElement) { startNode = rootNode.documentElement; } break; default: startNode = rootNode; break; } } ``` > We should keep type. Otherwise subclasses of HTMLElement - if there will be any in the future - would also match. OK.

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review98984

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review98878 > This is a valid concern. The only way I can see to avoid that is by moving the `!opts.startNode` test outside the switch statement: > > ```js > let startNode; > if (opts.startNode) { > startNode = opts.startNode; > } else { > switch (strategy) { > case element.Strategy.Anon: > case element.Strategy.AnonAttribute: > if (rootNode instanceof Ci.nsIDOMElement) { > startNode = rootNode.documentElement; > } > break; > > default: > startNode = rootNode; > break; > } > } > ``` That's a way better solution. Lets take that one!

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review99098

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d8c35632d389 Searching anonymous elements has to use the documentElement as default start node. r=ato

Comment on attachment 8816481 [details] Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node. https://reviewboard.mozilla.org/r/97202/#review99414

Test-only patch we would like to have for the next ESR release. Please uplift to aurora. Thanks.