1143071 - find_element(By.ANON_ATTRIBUTE) should use window's documentElement as default start node if not specified

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

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?

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Sure I'll check it out.

Comment 2

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

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.

Comment 3

[David Burns :automatedtester]

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

Comment 4

[Shruti Jasoria [:ShrutiJ]]

Hi! I would like to work on this bug.

Comment 5

[David Burns :automatedtester]

(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.

Comment 6

[Shruti Jasoria [:ShrutiJ]]

Attachment: Require additions

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 7

[David Burns :automatedtester]

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.

Comment 8

[Shruti Jasoria [:ShrutiJ]]

Attachment: Required Changes and test

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

Comment 9

[David Burns :automatedtester]

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

Comment 10

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

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.

Comment 11

[Shruti Jasoria [:ShrutiJ]]

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

Comment 12

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

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

Comment 13

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

Attachment: Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node.

Review commit: https://reviewboard.mozilla.org/r/97202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97202/

Comment 14

[Andreas Tolfsen ❲: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/#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 15

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

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 16

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

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 17

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

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 18

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

Comment on attachment 8816481 [details]
Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97202/diff/1-2/

Comment 19

[Andreas Tolfsen ❲: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/#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 20

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

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 21

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

Comment on attachment 8816481 [details]
Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97202/diff/2-3/

Comment 22

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

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 23

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

Comment on attachment 8816481 [details]
Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97202/diff/3-4/

Comment 24

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

Comment on attachment 8816481 [details]
Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97202/diff/4-5/

Comment 25

[Andreas Tolfsen ❲: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/#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 26

[Andreas Tolfsen ❲: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/#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 27

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

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 28

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

Comment on attachment 8816481 [details]
Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97202/diff/5-6/

Comment 29

[Andreas Tolfsen ❲: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/#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 30

[Andreas Tolfsen ❲: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/#review98984

Comment 31

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

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 32

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

Comment on attachment 8816481 [details]
Bug 1143071 - Searching anonymous elements has to use the documentElement as default start node.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97202/diff/6-7/

Comment 33

[Andreas Tolfsen ❲: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/#review99098

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8c35632d389
Searching anonymous elements has to use the documentElement as default start node. r=ato

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d8c35632d389

Comment 36

[David Burns :automatedtester]

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

Comment 37

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

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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/071ee21b49a6