1447009 - Stylesheets in shadow DOM with title attribute shouldn't affect the preferred stylesheet of the document.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Per https://html.spec.whatwg.org/multipage/semantics.html#attr-style-title

Comment 1

[Jet Villegas (inactive)]

[Triage 2018/03/23 - P3]

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1447009: Fix StyleSheet title getter to comply with the spec.

Alternate Shadow DOM style sheets don't make much sense, since they're not
accessible from the document in most cases anyway.

However WebKit does consider title in order to chose the preferred name.

Waiting on a resolution from the WG, let's choose the behavior that is simpler
to implement.

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

Comment 3

[Cameron McCormack (:heycam)]

Why don't alternate style sheets in shadow trees make sense, given we still have UI in the browser for selecting which set of alternate style sheets to apply?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Replied on IRC: https://mozilla.logbot.info/layout/20180507#c14718452

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8973541 [details]
Bug 1447009: Fix StyleSheet title getter to comply with the spec.

https://reviewboard.mozilla.org/r/241896/#review247798

::: layout/reftests/webcomponents/stylesheet-title-ref.html:6
(Diff revision 1)
> +/*
> + * NOTE(emilio): Test not upstreamed pending resolution for
> + * https://github.com/w3c/csswg-drafts/issues/2646
> + */

Put this comment in the main test file too (or instead of here)?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1447009: Ignore title if the element is not in the document.

Turns out this was well specified, see the linked bits.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973541 [details]
Bug 1447009: Fix StyleSheet title getter to comply with the spec.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8973541 [details]
Bug 1447009: Fix StyleSheet title getter to comply with the spec.

Turns out this was well spec'd after all, see the discussion in the issue.

This is on top of bug 1459844.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8973541 [details]
Bug 1447009: Fix StyleSheet title getter to comply with the spec.

https://reviewboard.mozilla.org/r/241896/#review248410

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8974098 [details]
Bug 1447009: Ignore title if the element is not in the document.

https://reviewboard.mozilla.org/r/242396/#review248408

::: dom/base/nsStyleLinkElement.cpp:99
(Diff revision 1)
> +  if (aSelf.IsInUncomposedDoc()) {
> -  aSelf.GetAttr(kNameSpaceID_None, nsGkAtoms::title, aTitle);
> +    aSelf.GetAttr(kNameSpaceID_None, nsGkAtoms::title, aTitle);
> -  aTitle.CompressWhitespace();
> +    aTitle.CompressWhitespace();
> +  }

Maybe do aTitle.Truncate() in an else block, so that this function always sets aTitle and doesn't rely on it already being empty.

::: testing/web-platform/tests/css/css-scoping/stylesheet-title-002.html:26
(Diff revision 1)
> +  for (let sheet of host.shadowRoot.styleSheets)
> +    assert_equals(sheet.title, null);

Can you also test that assigning to .title has no effect?

Also, is it worth having similar tests using <link>, or do we already have tests for that?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdca6257abb6
Fix StyleSheet title getter to comply with the spec. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dca44947bff
Ignore title if the element is not in the document. r=heycam

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Cameron McCormack (:heycam) from comment #10)
> Comment on attachment 8974098 [details]
> Bug 1447009: Ignore title if the element is not in the document.
> 
> https://reviewboard.mozilla.org/r/242396/#review248408
> 
> ::: dom/base/nsStyleLinkElement.cpp:99
> (Diff revision 1)
> > +  if (aSelf.IsInUncomposedDoc()) {
> > -  aSelf.GetAttr(kNameSpaceID_None, nsGkAtoms::title, aTitle);
> > +    aSelf.GetAttr(kNameSpaceID_None, nsGkAtoms::title, aTitle);
> > -  aTitle.CompressWhitespace();
> > +    aTitle.CompressWhitespace();
> > +  }
> 
> Maybe do aTitle.Truncate() in an else block, so that this function always
> sets aTitle and doesn't rely on it already being empty.

I left it as-is because GetHref was already relying on the same.

> ::: testing/web-platform/tests/css/css-scoping/stylesheet-title-002.html:26
> (Diff revision 1)
> > +  for (let sheet of host.shadowRoot.styleSheets)
> > +    assert_equals(sheet.title, null);
> 
> Can you also test that assigning to .title has no effect?

Done, also that title attribute mutation didn't have an effect.

> Also, is it worth having similar tests using <link>, or do we already have
> tests for that?

We don't support link on Shadow DOM yet, so probably not (yet?). Though the test would be pretty similar I expect.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Oh, also SetIsVoid already truncates: https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/xpcom/string/nsTSubstring.cpp#835

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dfde358e826
followup: Rename a test to appease wptlint. r=me

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10945 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/cdca6257abb6
https://hg.mozilla.org/mozilla-central/rev/2dca44947bff
https://hg.mozilla.org/mozilla-central/rev/9dfde358e826

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged