Closed Bug 1447009 Opened 6 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

Attachments

(2 files)

Bug 1447009: Fix StyleSheet title getter to comply with the spec.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1447009: Ignore title if the element is not in the document.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
[Triage 2018/03/23 - P3]
Priority: -- → P3
Depends on: 1260720
No longer depends on: 1260720
Depends on: 1459497
Depends on: 1459498
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 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 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 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 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+
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.
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.
Depends on: 1489881
Regressions: 1645789
No longer regressions: 1645789
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: