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

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: emilio, Assigned: emilio)

Tracking

(Depends on 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(2 attachments)

[Triage 2018/03/23 - P3]
Priority: -- → P3
Assignee

Updated

a year ago
Depends on: 1260720
Assignee

Updated

a year ago
No longer depends on: 1260720
Assignee

Updated

a year ago
Depends on: 1459497
Assignee

Updated

a year ago
Depends on: 1459498
Comment hidden (mozreview-request)
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?
Flags: needinfo?(emilio)
Replied on IRC: https://mozilla.logbot.info/layout/20180507#c14718452
Assignee: nobody → emilio
Flags: needinfo?(emilio)

Comment 5

a year ago
mozreview-review
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)?
Attachment #8973541 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8973541 - Flags: review+ → review?(cam)

Comment 9

a year ago
mozreview-review
Comment on attachment 8973541 [details]
Bug 1447009: Fix StyleSheet title getter to comply with the spec.

https://reviewboard.mozilla.org/r/241896/#review248410
Attachment #8973541 - Flags: review?(cam) → review+

Comment 10

a year ago
mozreview-review
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?
Attachment #8974098 - Flags: review?(cam) → review+

Comment 11

a year ago
Pushed by ecoal95@gmail.com:
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
(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 14

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dfde358e826
followup: Rename a test to appease wptlint. r=me
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10945 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged

Updated

9 months ago
Depends on: 1489881
You need to log in before you can comment on or make changes to this bug.