Closed Bug 1194224 Opened 4 years ago Closed 4 years ago

Support for Shadow DOM in Marionette

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed, pi-marionette-server)

Attachments

(3 files)

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.
Summary: Improve marionette testing of content within web-components. → Support for Shadow DOM in Marionette
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.
Depends on: 1194296
When this is done we need to try the approach proposed URL section of the bug
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”.
(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.
(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.
(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.
Attached patch 1194224 patch v1Splinter Review
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 <dburns@mozilla.com> is not currently accepting 'review' requests. (HTTP 500, API Error 225)
Attachment #8649459 - Flags: feedback?
Attachment #8649459 - Flags: feedback? → feedback?(dburns)
(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 <dburns@mozilla.com> 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 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
Attachment #8649459 - Flags: feedback?(dburns) → feedback+
SR in the example in comment 9 means ShadowRoot for clarity
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
Attachment #8651821 - Flags: review?(dburns)
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
Attachment #8651821 - Flags: review?(dburns)
Attachment #8651821 - Flags: review?(dburns)
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 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 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 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 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
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(-)
Attachment #8653048 - Flags: review?(dburns)
Sorry for the spam, had some trouble with publishing updates first.
Comment on attachment 8651821 [details]
MozReview Request: Bug 1194224 - adding support for Shadow DOM in marionette.

https://reviewboard.mozilla.org/r/16403/#review15793
Attachment #8651821 - Flags: review?(dburns) → review+
Attachment #8653048 - Flags: review?(dburns) → review+
Comment on attachment 8653048 [details]
MozReview Request: Bug 1194224 - fixing a bug and adding more tests.

https://reviewboard.mozilla.org/r/17347/#review15795
Another one since the above one had Gij failures that are persistent on inbound atm:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2fa8be2f868
Blocks: 1200400
https://hg.mozilla.org/mozilla-central/rev/2a85b5644503
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Keywords: dev-doc-needed
Duplicate of this bug: 1061698
Adding dependancies to the bugs where we need to use this: bug 1093585, bug 1094139, bug 1097605.
Blocks: 1204665
Blocks: 1208553
Andreas, can you help me finding somebody who could update the relevant part of the Marionette docs on MDN?
Flags: needinfo?(ato)
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() ?
Flags: needinfo?(yzenevich)
(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.
Flags: needinfo?(yzenevich)
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!
Flags: needinfo?(dburns)
I will get a release done now then
Flags: needinfo?(dburns)
Blocks: 1209552
Blocks: 1209584
No longer blocks: 1209552
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?
Flags: needinfo?(dburns)
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
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
Blocks: 1213301
You need to log in before you can comment on or make changes to this bug.