1194224 - Support for Shadow DOM in Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

Marionette fails to handle content inside shadow DOM. Notably, ElementManager.getKnownElement throws a StaleElementReferenceError because compareDocumentPosition does not handle shadow DOM elements well in relation to the content document. This means that the shadow DOM elements are treated as stale and inaccessible from the client point of view.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Even though it's not spelled out in the WebDriver specification that we should support Shadow DOM, it will be added reasonably soon.  This is why I've added it as a blocker fo rbug 721859.

Comment 2

[David Burns :automatedtester]

When this is done we need to try the approach proposed URL section of the bug

Comment 3

[Andreas Tolfsen ❲:ato❳]

It’s a bit of an open question how we will implement this.  If we accept the idea that Shadow DOMs in a WebDriver context are semantically similar to <iframe> elements, we would treat it as a separate browsing context:

To switch context into an <iframe> and shadow DOM element, respectively:

     Switch To Frame {element: <iframe web element>}
     Switch To Frame {element: <shadow DOM web element>}

To switch to the default browsing context (the primary browsing context of the top-level browsing context):

     Switch To Frame {value: ""}

This allows traversal into descendant shadow DOM’s, as with <iframe>’s.

Alternatively we can have a separate command endpoint specifically for shadow DOM, or rename the “Switch To Frame” command to something which catches the meaning of both, for example “Switch To Browsing Context”.

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Andreas Tolfsen (:ato) from comment #3)
> It’s a bit of an open question how we will implement this.  If we accept the
> idea that Shadow DOMs in a WebDriver context are semantically similar to
> <iframe> elements, we would treat it as a separate browsing context:
> 
> To switch context into an <iframe> and shadow DOM element, respectively:
> 
>      Switch To Frame {element: <iframe web element>}
>      Switch To Frame {element: <shadow DOM web element>}
> 
> To switch to the default browsing context (the primary browsing context of
> the top-level browsing context):
> 
>      Switch To Frame {value: ""}
> 
> This allows traversal into descendant shadow DOM’s, as with <iframe>’s.
> 
> Alternatively we can have a separate command endpoint specifically for
> shadow DOM, or rename the “Switch To Frame” command to something which
> catches the meaning of both, for example “Switch To Browsing Context”.

I was looking more into this and I got convinced that this is not the same thing as switching to frame. Essentially switching to frame changes the visibility scope of marionette (parent is not accessible) where when switching to frame does not really do that.

Although the ShadowRoot object has a document fragment interface it has quite a number of differences that are in itself semantic. Also in terms of Shadow DOM ShadowRoot is closely related to the host element where there's no such relation in context of frames.

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Yura Zenevich [:yzen] from comment #4)
> Essentially switching to frame changes the visibility scope
> of marionette (parent is not accessible) where when switching
> to frame does not really do that.

Can you clarify?  Did you mean “shadow DOM” somewhere here?

I’m not sure if you’re arguing that the model of switching into a Shadow DOM doesn’t make sense, or if you have concerns about the sharing the command with <iframe>’s.

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Andreas Tolfsen (:ato) from comment #5)
> (In reply to Yura Zenevich [:yzen] from comment #4)
> > Essentially switching to frame changes the visibility scope
> > of marionette (parent is not accessible) where when switching
> > to frame does not really do that.
> 
> Can you clarify?  Did you mean “shadow DOM” somewhere here?
> 
> I’m not sure if you’re arguing that the model of switching into a Shadow DOM
> doesn’t make sense, or if you have concerns about the sharing the command
> with <iframe>’s.

Urgh, sorry yes:

Essentially switching to frame changes the visibility scope
of marionette (parent is not accessible) where when switching
to shadow dom does not really do that.

and yes I feel like switching to shadow dom makes sense but is better to be kept separate from switching to frame. Essentially, allowing us to be more explicit about the current frame and the current shadow root.

Comment 7

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1194224 patch v1

This is a WIP to gather some feedback.

I tried pusing to mozreview but got the following error:
error publishing review request 16401: Error publishing the review request: Bugzilla error: David Burns :automatedtester <[e-mail]> is not currently accepting 'review' requests. (HTTP 500, API Error 225)

Comment 8

[David Burns :automatedtester]

(In reply to Yura Zenevich [:yzen] from comment #7)
> Created attachment 8649459 [details] [diff] [review]
> 1194224 patch v1
> 
> This is a WIP to gather some feedback.
> 
> I tried pusing to mozreview but got the following error:
> error publishing review request 16401: Error publishing the review request:
> Bugzilla error: David Burns :automatedtester <[e-mail]> is not
> currently accepting 'review' requests. (HTTP 500, API Error 225)

Sorry, I forgot to switch that off after coming back from PTO. Sorry!

Comment 9

[David Burns :automatedtester]

Comment on attachment 8649459 [details] [diff] [review]
1194224 patch v1

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

This looks good.
I would like to see a test where there is a tree of trees or composed trees (http://w3c.github.io/webcomponents/spec/shadow/#trees-of-trees). Ideally we want switch_to_shadow_root() to move up a layer

E.g.

           A
           /\
          /  \
         SR   C
         /    /\
        D
       / \
      SR  
      /\
     E


So to get to E we would need to do
> A = marionette.find_element('id', 'A')
> marionette.switch_to_shadow_root(A)
> D = marionette.find_element('id', 'D')
> marionette.switch_to_shadow_root(D)
> E = marionette.find_element('id', 'E')
> marionette.switch_to_shadow_root() # Moves us up to D
> marionette.switch_to_shadow_root() # Moves us up to A

Comment 10

[David Burns :automatedtester]

SR in the example in comment 9 means ShadowRoot for clarity

Comment 11

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: MozReview Request: Bug 1194224 - adding support for Shadow DOM in marionette.

Bug 1194224 - adding support for Shadow DOM in marionette.

---
 testing/marionette/actions.js                      |  30 +--
 .../marionette/tests/unit/test_shadow_dom.py       |  58 +++++
 .../client/marionette/tests/unit/unit-tests.ini    |   2 +
 .../client/marionette/www/test_shadow_dom.html     |  25 ++
 testing/marionette/driver.js                       |  47 ++--
 .../driver/marionette_driver/marionette.py         |  15 ++
 testing/marionette/elements.js                     |  76 ++++--
 testing/marionette/listener.js                     | 261 ++++++++++++---------
 8 files changed, 348 insertions(+), 166 deletions(-)
 create mode 100644 testing/marionette/client/marionette/tests/unit/test_shadow_dom.py
 create mode 100644 testing/marionette/client/marionette/www/test_shadow_dom.html

Comment 12

[David Burns :automatedtester]

Comment on attachment 8651821 [details]
MozReview Request: Bug 1194224 - adding support for Shadow DOM in marionette.

https://reviewboard.mozilla.org/r/16403/#review15283

::: testing/marionette/listener.js:1689
(Diff revision 1)
> +    throw new NoSuchElementError('Unable to locate shadow root: ' + msg.json.element);

msg is not defined in this method, did you mean `id`? Perhaps we need a test for this

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8651821 [details]
MozReview Request: Bug 1194224 - adding support for Shadow DOM in marionette.

Bug 1194224 - adding support for Shadow DOM in marionette.

---
 testing/marionette/actions.js                      |  30 +--
 .../marionette/tests/unit/test_shadow_dom.py       |  58 +++++
 .../client/marionette/tests/unit/unit-tests.ini    |   2 +
 .../client/marionette/www/test_shadow_dom.html     |  25 ++
 testing/marionette/driver.js                       |  47 ++--
 .../driver/marionette_driver/marionette.py         |  15 ++
 testing/marionette/elements.js                     |  76 ++++--
 testing/marionette/listener.js                     | 261 ++++++++++++---------
 8 files changed, 348 insertions(+), 166 deletions(-)
 create mode 100644 testing/marionette/client/marionette/tests/unit/test_shadow_dom.py
 create mode 100644 testing/marionette/client/marionette/www/test_shadow_dom.html

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8651821 [details]
MozReview Request: Bug 1194224 - adding support for Shadow DOM in marionette.

Bug 1194224 - adding support for Shadow DOM in marionette.

---
 testing/marionette/actions.js                      |  30 +--
 .../marionette/tests/unit/test_shadow_dom.py       |  58 +++++
 .../client/marionette/tests/unit/unit-tests.ini    |   2 +
 .../client/marionette/www/test_shadow_dom.html     |  25 ++
 testing/marionette/driver.js                       |  47 ++--
 .../driver/marionette_driver/marionette.py         |  15 ++
 testing/marionette/elements.js                     |  76 ++++--
 testing/marionette/listener.js                     | 261 ++++++++++++---------
 8 files changed, 348 insertions(+), 166 deletions(-)
 create mode 100644 testing/marionette/client/marionette/tests/unit/test_shadow_dom.py
 create mode 100644 testing/marionette/client/marionette/www/test_shadow_dom.html

Fixing a bug with incorrect argument when throwing error on no shadow root, addding tests.

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8651821 [details]
MozReview Request: Bug 1194224 - adding support for Shadow DOM in marionette.

Bug 1194224 - adding support for Shadow DOM in marionette.

---
 testing/marionette/actions.js                      |  30 +--
 .../marionette/tests/unit/test_shadow_dom.py       |  58 +++++
 .../client/marionette/tests/unit/unit-tests.ini    |   2 +
 .../client/marionette/www/test_shadow_dom.html     |  25 ++
 testing/marionette/driver.js                       |  47 ++--
 .../driver/marionette_driver/marionette.py         |  15 ++
 testing/marionette/elements.js                     |  76 ++++--
 testing/marionette/listener.js                     | 261 ++++++++++++---------
 8 files changed, 348 insertions(+), 166 deletions(-)
 create mode 100644 testing/marionette/client/marionette/tests/unit/test_shadow_dom.py
 create mode 100644 testing/marionette/client/marionette/www/test_shadow_dom.html

Fixing an issue with wrong arg when throwing error on no shadow root; adding test

Comment 16

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8651821 [details]
MozReview Request: Bug 1194224 - adding support for Shadow DOM in marionette.

Bug 1194224 - adding support for Shadow DOM in marionette.

---
 testing/marionette/actions.js                      |  30 +--
 .../marionette/tests/unit/test_shadow_dom.py       |  58 +++++
 .../client/marionette/tests/unit/unit-tests.ini    |   2 +
 .../client/marionette/www/test_shadow_dom.html     |  25 ++
 testing/marionette/driver.js                       |  47 ++--
 .../driver/marionette_driver/marionette.py         |  15 ++
 testing/marionette/elements.js                     |  76 ++++--
 testing/marionette/listener.js                     | 261 ++++++++++++---------
 8 files changed, 348 insertions(+), 166 deletions(-)
 create mode 100644 testing/marionette/client/marionette/tests/unit/test_shadow_dom.py
 create mode 100644 testing/marionette/client/marionette/www/test_shadow_dom.html

Fixing an issue with wrong arg when throwing error on no shadow root; adding test

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8651821 [details]
MozReview Request: Bug 1194224 - adding support for Shadow DOM in marionette.

Bug 1194224 - adding support for Shadow DOM in marionette.

---
 testing/marionette/actions.js                      |  30 +--
 .../marionette/tests/unit/test_shadow_dom.py       |  58 +++++
 .../client/marionette/tests/unit/unit-tests.ini    |   2 +
 .../client/marionette/www/test_shadow_dom.html     |  25 ++
 testing/marionette/driver.js                       |  47 ++--
 .../driver/marionette_driver/marionette.py         |  15 ++
 testing/marionette/elements.js                     |  76 ++++--
 testing/marionette/listener.js                     | 261 ++++++++++++---------
 8 files changed, 348 insertions(+), 166 deletions(-)
 create mode 100644 testing/marionette/client/marionette/tests/unit/test_shadow_dom.py
 create mode 100644 testing/marionette/client/marionette/www/test_shadow_dom.html

Comment 18

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: MozReview Request: Bug 1194224 - fixing a bug and adding more tests.

Bug 1194224 - fixing a bug and adding more tests.

---
 .../marionette/client/marionette/tests/unit/test_shadow_dom.py   | 9 ++++++++-
 testing/marionette/client/marionette/www/test_shadow_dom.html    | 1 +
 testing/marionette/listener.js                                   | 2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

Sorry for the spam, had some trouble with publishing updates first.

Comment 20

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/16401/#review15769

Comment 21

[David Burns :automatedtester]

Comment on attachment 8651821 [details]
MozReview Request: Bug 1194224 - adding support for Shadow DOM in marionette.

https://reviewboard.mozilla.org/r/16403/#review15793

Comment 22

[David Burns :automatedtester]

Comment on attachment 8653048 [details]
MozReview Request: Bug 1194224 - fixing a bug and adding more tests.

https://reviewboard.mozilla.org/r/17347/#review15795

Comment 23

[Yura Zenevich [:yzen] (please use "needinfo")]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adedd33e5ffc

Comment 24

[Yura Zenevich [:yzen] (please use "needinfo")]

Another one since the above one had Gij failures that are persistent on inbound atm:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2fa8be2f868

Comment 25

[Yura Zenevich [:yzen] (please use "needinfo")]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a85b5644503

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a85b5644503

Comment 28

[Martijn Wargers (dead)]

Adding dependancies to the bugs where we need to use this: bug 1093585, bug 1094139, bug 1097605.

Comment 29

[Jean-Yves Perrier [:teoli]]

Andreas, can you help me finding somebody who could update the relevant part of the Marionette docs on MDN?

Comment 30

[Martijn Wargers (dead)]

Yura, should this also be documented at http://marionette-client.readthedocs.org/en/latest/reference.html ?
How do I get this latest marionette version installed with support for self.marionette.switch_to_shadow_root() ?

Comment 31

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #30)
> Yura, should this also be documented at
> http://marionette-client.readthedocs.org/en/latest/reference.html ?
> How do I get this latest marionette version installed with support for
> self.marionette.switch_to_shadow_root() ?

Yeah I agree, that's why I  added dev-docs-needed, not sure if that's the right flag though.

Comment 32

[No-Jun Park [:njpark]]

So just wanted to reiterate Martijn's second point, which marionette-client version currently has this fix?  We need this fix to make our tests work for new Music-NGA app since it uses shadow DOM quite often. 
Thanks!

Comment 33

[David Burns :automatedtester]

I will get a release done now then

Comment 34

[No-Jun Park [:njpark]]

hi David, I just upgraded to marionette-client=0.20, but there is no reference to 'self.marionette.switch_to_shadow_root()'.  Did I use the right version?

Comment 35

[David Burns :automatedtester]

There was an error with packaging. Marionette-client 0.20 was correct but that will not pulling the right marionette_driver package. I will have this fixed and released shortly.

See bug 1209698