The default bug view has changed. See this FAQ.

stylo: UA style sheets parsed with author features should be added at the UA level

NEW
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
4 months ago
6 hours ago

People

(Reporter: heycam, Assigned: heycam, NeedInfo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

4 months ago
Since bug 1293809, we no longer apply document-level style sheets to native anonymous content.  However, some UA sheets are parsed with document level features enabled, rather than agent level features.  This is done just to limit the chance of accidentally adding "unsafe" rules, I guess, but it has the effect of those sheets being added as document-level sheets in the Stylist.

The scrollbars sheet is one such sheet, so this is causing scrollbars not to work.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

4 months ago
mozreview-review
Comment on attachment 8816863 [details]
Use SheetType to determine Servo Stylesheet origin, not SheetParsingMode.

https://reviewboard.mozilla.org/r/97394/#review97670
Attachment #8816863 - Flags: review?(ecoal95) → review+

Comment 7

4 months ago
mozreview-review
Comment on attachment 8816864 [details]
Update regen.py.

https://reviewboard.mozilla.org/r/97396/#review97672
Attachment #8816864 - Flags: review?(ecoal95) → review+
I'm not sure whether part 1 is the right way to go. Redirecting to dbaron. Actually, how do scrollbars work on Firefox if it doesn't work when its style sheet is treated as doc-level one?
Attachment #8816866 - Flags: review?(xidorn+moz) → review?(dbaron)
Having a sheet type in addition to parsing mode everywhere doesn't look sensible to me... What would we lose if we just make every sheet use the parsing mode they desire?
(Assignee)

Comment 10

4 months ago
I don't know if there are places other than those UA sheets in nsLayoutStylesheetCache where we use a parsing mode that doesn't match the sheet type.  I will look.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> Actually, how do scrollbars work on Firefox if it doesn't work when its
> style sheet is treated as doc-level one?

It's not treated that way in Gecko, just with Stylo.

Comment 11

4 months ago
mozreview-review
Comment on attachment 8816862 [details]
Bug 1321754 - Part 2: Pass SheetType to Servo FFI functions that create sheets.

https://reviewboard.mozilla.org/r/97392/#review97800

This patch makes sense to me. I wonder whether we can just replace parsing mode with sheet type. I have no idea how it is useful to have two things basically for the same goal.
Attachment #8816862 - Flags: review?(xidorn+moz) → review+
(In reply to Cameron McCormack (:heycam) from comment #0)
> This is done just to
> limit the chance of accidentally adding "unsafe" rules, I guess, but it has
> the effect of those sheets being added as document-level sheets in the
> Stylist.

Are you sure this wasn't just an accident?
Flags: needinfo?(cam)
(Assignee)

Comment 13

4 months ago
I think so.  The comment above the LoadSheet calls that use a more restricted parsing mode but for an agent sheet say things like "this sheet doesn't need unsafe rules".
Flags: needinfo?(cam)
(Assignee)

Comment 14

4 months ago
A try push shows no failures when I align the sheet type and parsing mode for the sheets loaded by nsLayoutStylesheetCache, so maybe we indeed don't need to record both of these:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71ab03448022ca8920d7463479fd97470486b503&group_state=expanded
(Assignee)

Comment 15

4 months ago
Ah, that was the wrong try push.  This one instead shows some failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a75ebdb80f778be7f27e4391506311353fe02038&selectedJob=32317399
(Assignee)

Comment 16

4 months ago
Bobby pointed out that in the WebExtensions world, we probably want to restrict extensions from adding style sheets with unsafe rules, but still want to allow them to be added in the user agent level.  So that probably indicates we still want to keep the concepts of sheet level and parsing mode separate.
(Assignee)

Updated

3 months ago
See Also: → bug 1291376
As a record, I think we in general shouldn't allow anything to touch user agent sheet if we don't allow it to use any unsafe rules. Basically, important user agent level rule should be considered unsafe to change.

Extensions should either be restricted to use at most the user style level, or be allowed to use whatever they want at their own risk of breaking the browser, since allowing them to use user agent level could be dangerous enough.
(Assignee)

Comment 18

3 months ago
I had trouble searching for a definitive answer, but Rob Wu told me that WebExtensions don't allow you to load a style sheet at anything other than the document level, so at some point in the future when we no longer have XPCOM addons this problem might indeed be solved for us.  (Although it is still possible system / go faster addons would want to load sheets at different levels.)

Searching on DXR, there do seem to be many addons that are passing AGENT_SHEET to style sheet service methods.  I did wonder whether we could just silently treat AGENT_SHEET as USER_SHEET, but thinking some more I am concerned that addons rely on (accidentally or deliberately) the relative specificity of rules in their sheets and Firefox's sheets.

I think for now we should do the safest thing for addon compatibility and keep the concepts of "level a sheet is added at" and "features available when parsing the sheet" separate.  As a followup, I think we can change Servo so that the Origin is not recorded on a sheet itself, but instead something that the Stylist keeps track of.  That way we can create a sheet with a given parsing mode and be able to add to the Stylist at an arbitrary cascade level.
I believe that style sheet service does use agent level parsing mode for style sheets with type AGENT_SHEET, and some of our tests may also depend on that behavior.

Addons are not encouraged to do that, though.
So I'm still not happy about having 2 different values for the cascade level, one of which is sometimes wrong in order to prevent allowing unsafe rules?

What about, instead:
 * having a single cascade level enum that's always correct
 * having a flags field that (today) has just one flag, which is whether to allow unsafe rules
 * having an MOZ_RELEASE_ASSERT that the unsafe rules flag is only set when the cascade level is UA sheet
?
Flags: needinfo?(cam)

Comment 21

3 months ago
mozreview-review
Comment on attachment 8816866 [details]
Bug 1321754 - Part 1: Record on StyleSheet objects whether they are UA, user or document style sheets.

https://reviewboard.mozilla.org/r/97402/#review103582

see comment 20
Attachment #8816866 - Flags: review?(dbaron) → review-
Priority: -- → P1
Assignee: nobody → cam
Cameron, Per comment 20, are your patches in this bug still relevant? Just trying to figure out whether I should keep this bug on your plate (on the grounds that you're partway through it) or pass it along to someone else (on the grounds that you've got a lot on your plate).
You need to log in before you can comment on or make changes to this bug.