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

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Blocks 1 bug, {pi-marionette-server})

Trunk
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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?
Flags: needinfo?(cmanchester)
Sure I'll check it out.
Flags: needinfo?(cmanchester)
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.
No longer blocks: m21s, 1143020
Summary: By.ANON_ATTRIBUTE fails with: Document.getAnonymousElementByAttribute does not implement interface Element → find_element(By.ANON_ATTRIBUTE) should use window.document as default root element if called via marionette directly
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
Mentor: dburns
Whiteboard: [good first bug][lang=py]
Hi! I would like to work on this bug.
Flags: needinfo?(dburns)
(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.
Flags: needinfo?(dburns)
Posted patch Require additions (obsolete) — Splinter Review
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.
Attachment #8769060 - Flags: feedback?(dburns)
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.
Attachment #8769060 - Flags: feedback?(dburns) → feedback-
Posted patch Required Changes and test (obsolete) — Splinter Review
I have made the changes which you had asked for in comment 7. I hope its good now.
Attachment #8769060 - Attachment is obsolete: true
Attachment #8771264 - Flags: review?(dburns)
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
Attachment #8771264 - Flags: review?(dburns) → review-
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.
Flags: needinfo?(shrutijasoria1996)
Keywords: good-first-bug
Whiteboard: [good first bug][lang=py] → [lang=py]
Sorry, I will not be able to work on this bug anymore.
Flags: needinfo?(shrutijasoria1996)
I will work on this bug now so we can have it as part of the next ESR release.
Assignee: nobody → hskupin
Mentor: dburns
Status: NEW → ASSIGNED
Keywords: good-first-bug
Whiteboard: [lang=py]
Attachment #8771264 - Attachment is obsolete: true
Attachment #8816481 - Flags: review?(ato)

Comment 14

3 years ago
mozreview-review
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?
Attachment #8816481 - Flags: review?(ato) → review-
Assignee

Comment 15

3 years ago
mozreview-review-reply
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.
Assignee

Comment 16

3 years ago
mozreview-review-reply
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`.
Assignee

Comment 17

3 years ago
mozreview-review-reply
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.
Flags: needinfo?(ato)
Summary: find_element(By.ANON_ATTRIBUTE) should use window.document as default root element if called via marionette directly → find_element(By.ANON_ATTRIBUTE) should use window's documentElement as default start node if not specified
Comment hidden (mozreview-request)
Flags: needinfo?(ato)
Attachment #8816481 - Flags: review?(dburns)

Comment 19

3 years ago
mozreview-review-reply
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.
Assignee

Comment 20

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Assignee

Comment 22

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

3 years ago
mozreview-review
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.)
Attachment #8816481 - Flags: review?(ato) → review-

Comment 26

3 years ago
mozreview-review-reply
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.
Assignee

Comment 27

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 29

3 years ago
mozreview-review-reply
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

3 years ago
mozreview-review
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
Attachment #8816481 - Flags: review?(ato) → review-
Assignee

Comment 31

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 33

3 years ago
mozreview-review
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
Attachment #8816481 - Flags: review?(ato) → review+

Comment 34

3 years ago
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 35

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d8c35632d389
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 36

3 years ago
mozreview-review
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
Attachment #8816481 - Flags: review?(dburns) → review+
Test-only patch we would like to have for the next ESR release. Please uplift to aurora. Thanks.
Whiteboard: [checkin-needed-aurora]

Comment 38

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/071ee21b49a6
Flags: in-testsuite+
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.