1594122 - Port the remaining XBL tests to work with Shadow DOM

Status: RESOLVED FIXED

(Core :: XBL, task)

Description

[Brian Grinstead [:bgrins]]

The following tests weren't removed in Bug 1587142 because they should be ported to Shadow DOM

• test_bug330925.xhtml
• test_bug372086.html
• test_bug319374.xhtml
• 348049-1.xhtml
• 1369954-1.xhtml
• 1371130.xhtml

Comment 1

[alexander :surkov (:asurkov)]

Attachment: Bug 1594122 - convert XBL test test_bug330925.xhtml to shadow DOM test

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8823aa7d4ff9
convert XBL test test_bug330925.xhtml to shadow DOM test r=bzbarsky

Comment 3

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8823aa7d4ff9

Comment 4

[alexander :surkov (:asurkov)]

Attachment: Bug 1594122 - convert XBL bindings to custom elements in test_bug372086.html

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78e9d7e282ce
convert XBL bindings to custom elements in test_bug372086.html r=bzbarsky

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/78e9d7e282ce

Comment 7

[alexander :surkov (:asurkov)]

Attachment: Bug 1594122 - convert XBL bindings to custom elements in test_bug319374.xhtml

Comment 8

[alexander :surkov (:asurkov)]

Attachment: Bug 1594122 - convert XBL bindings to custom elements in test_bug319374.xhtml

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7b53d25d5d4
convert XBL bindings to custom elements in test_bug319374.xhtml r=bzbarsky

Comment 10

[Alexandru Michis [:malexandru]]

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

Comment 11

[alexander :surkov (:asurkov)]

Attachment: Bug 1594122 - convert XBL binding to custom element in 348049-1.xhtml reftest

Comment 12

[alexander :surkov (:asurkov)]

interesting that reftest 348049-1.xhtml fails if it's xhtml page: no content is slotted into shadow DOM, but it works nicely if it's html (see D52854). Emilio, is it something you could look at?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #12)

interesting that reftest 348049-1.xhtml fails if it's xhtml page: no content is slotted into shadow DOM, but it works nicely if it's html (see D52854). Emilio, is it something you could look at?

btw, here is

• try server build https://treeherder.mozilla.org/#/jobs?repo=try&revision=272423cffbf269ea0413c7faf320ebd6d2159078&selectedJob=275844354
• changeset https://hg.mozilla.org/try/rev/b79861dba110cb6ff70ba5ad1f2e91823c0c32e8
• reftest analyzer https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/WZN_HcY7Tc26XX2q_4BYDg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

for xhtml version

Comment 14

[Boris Zbarsky [:bzbarsky]]

That's because the shadow DOM is the empty string due to not escaping the script string in any way and it containing tags, which the XML parser then parses. Wrapping <![CDATA[ ... ]]> around the script (which is more or less required in XHTML), fixes the problem.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #14)

That's because the shadow DOM is the empty string due to not escaping the script string in any way and it containing tags, which the XML parser then parses. Wrapping <![CDATA[ ... ]]> around the script (which is more or less required in XHTML), fixes the problem.

hm, I believe I tried wrapping it by CDATA, let me re-run it again.

Comment 16

[Brian Grinstead [:bgrins]]

Is it possible that setting innerHTML is failing due to a security restriction? You could programatically create that DOM with createElement / appendChild if so.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #16)

Is it possible that setting innerHTML is failing due to a security restriction? You could programatically create that DOM with createElement / appendChild if so.

I tried to make them running via chrome as well, which should help, right? but I'll start a fresh try build just in case as well

Comment 18

[Boris Zbarsky [:bzbarsky]]

Setting innerHTML is more restricted in chrome tests, so no, you don't want that. What we want here is to run it as untrusted XHTML, with CDATA wrapping the script, and that should work, afaict.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #18)

Setting innerHTML is more restricted in chrome tests, so no, you don't want that. What we want here is to run it as untrusted XHTML, with CDATA wrapping the script, and that should work, afaict.

apparently CDATA doesn't fix the issue https://treeherder.mozilla.org/#/jobs?repo=try&revision=27ccd81679605ffa9d3b82c84c85489f84605ee4&selectedJob=276192086

chrome, you were right, doesn't help it either: https://treeherder.mozilla.org/#/jobs?repo=try&revision=998446d5a414928bfd0dc303dbd8fee1ec415430

Note, innerHTML sets the shadow DOM: td and slot elements are in there, but slotting doesn't work

(In reply to Brian Grinstead [:bgrins] from comment #16)

Is it possible that setting innerHTML is failing due to a security restriction? You could programatically create that DOM with createElement / appendChild if so.

that works, confirmed. Should I alter the test this way? and perhaps file a followup for innerHTML slotting?

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #19)

(In reply to Brian Grinstead [:bgrins] from comment #16)

Is it possible that setting innerHTML is failing due to a security restriction? You could programatically create that DOM with createElement / appendChild if so.

that works, confirmed. Should I alter the test this way? and perhaps file a followup for innerHTML slotting?

Can you extract this into a reduced test case? I'm sure it's worth a bug, especially if it's exposed to web content. In the meantime the mechanism to create the slot looks tangential this test so I don't see a harm in using createElement instead here.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Those try links look off compared to the comment: the first one is for the no-innerHTML version (using CDATA, but that doesn't matter because there are no tag names) , the second for the CDATA one using innerHTML but also using chrome URLs.

The first one is failing because it's calling append on the return value of append, but append returns undefined. So it never actually appends the <td> to the shadow DOM.

That said, I just tested what happens if I fix that to do:

          this.shadowRoot.innerHTML = "<td><slot></slot></td>";

and that still fails, though now for an interesting reason: those td and slot elements are created in the null namespace! That's a bug in our code; I filed bug 1596458 on that and will post a patch shortly.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #21)

Those try links look off compared to the comment: the first one is for the no-innerHTML version (using CDATA, but that doesn't matter because there are no tag names) , the second for the CDATA one using innerHTML but also using chrome URLs.

The first one is failing because it's calling append on the return value of append, but append returns undefined. So it never actually appends the <td> to the shadow DOM.

sorry, wrong link for sure, here's correct one https://hg.mozilla.org/try/rev/646a67a18959203d1d76139c4d2a913a807e13f0

That said, I just tested what happens if I fix that to do:

          this.shadowRoot.innerHTML = "<td><slot></slot></td>";

and that still fails, though now for an interesting reason: those td and slot elements are created in the null namespace! That's a bug in our code; I filed bug 1596458 on that and will post a patch shortly.

thanks!

so should I wait for bug 1596458 fixed or should I proceed with appendChild approach, try server looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=92919821fe5a4e6b367b4c3409acb98a3c57ffc9&selectedJob=276211495

Comment 23

[Boris Zbarsky [:bzbarsky]]

The appendChild thing is fine if you have it ready to go.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #0)

• 1369954-1.xhtml

looking at the bug 1369954 which added this test, not sure I understand why <p> element is prefixed by xhtml namespace. Shouldn't it be in xhtml namespace by default like its <span> sibling?

Comment 25

[Boris Zbarsky [:bzbarsky]]

The <span> there is NOT in the XHTML namespace, fwiw. It's in the XBL namespace....

Comment 26

[Boris Zbarsky [:bzbarsky]]

Which is a bug in the test; it should have had "<xhtml:span>", looks like, to match the reference.

Comment 27

[alexander :surkov (:asurkov)]

Attachment: Bug 1594122 - convert XBL binding to custom element in reftest 1369954-1.xhtml

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d395b70f3ff8
convert XBL binding to custom element in 348049-1.xhtml reftest r=bzbarsky

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a21f7f0b362e
convert XBL binding to custom element in reftest 1369954-1.xhtml r=bzbarsky

Comment 30

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d395b70f3ff8
https://hg.mozilla.org/mozilla-central/rev/a21f7f0b362e

Comment 31

[alexander :surkov (:asurkov)]

Attachment: Bug 1594122 - convert XBL bindings to custom elements in 1371130.xhtml reftest

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d60ae80f33b
convert XBL bindings to custom elements in 1371130.xhtml reftest r=bzbarsky

Comment 33

[alexander :surkov (:asurkov)]

Attachment: Bug 1594122 - remove skip xbl tests option

Comment 34

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8d60ae80f33b

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c62f8fbfd80a
remove skip xbl tests option r=bzbarsky

Comment 36

[Natalia Csoregi [:nataliaCs]]

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