style sheet additions (etc.) should check that rules might apply to the current document

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
2 years ago
9 months ago

People

(Reporter: dbaron, Assigned: bugs)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {perf})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(platform-rel +, firefox49 affected)

Details

(Whiteboard: [platform-rel-Facebook][qf:p2])

Sites that dynamically add style sheets while pages are already loaded often do this in a way that the added style sheets cannot affect elements in the current page.

We should optimize for this by improving the aStyleSheet->HasRules() checks in methods like PresShell::StyleSheetAdded, etc. (all of the ones that call RecordStyleSheetChange) to check that there is at least one rule in the sheet that doesn't require the presence of an id or class that's known not to be in the document.

This requires maintaining a data structure for all of the ids/classes that are in the document -- or perhaps even better -- all the ones we've resolved style for.

We'd still need to rebuild the rule cascade -- which might make the dynamic media query change handling a little bit tricky (see, e.g., bug 1089417).

We also might want to first measure to try to figure out, for important cases, how much performance improvement we'd get from this versus bug 1273302.
We have nsDocument::mIdentifierMap, which stores elements by id="" (and by name="").  Not sure if it's better to extend that to store elements by class="" too or to do as you suggest and have a separate table of IDs/classes that were used during previous selector matchings.

With bug 1213122 we will take all of the selectors in the added style sheet and only resolve style for elements in the document that do match them.  If that lands, we could skip adding selectors for those that fail this ID/class test.
FWIW, I believe this approach will be good enough to suppress the restyles for Facebook, and likely other sites that take a similar approach.  And it should be much cheaper than scanning the document, per-selector.

It's also probably worth issuing a console warning when we do have to restyle, and saying which selector was responsible.
platform-rel: --- → ?
Whiteboard: [platform-rel-Facebook]

Updated

2 years ago
platform-rel: ? → +

Updated

2 years ago
Rank: 5
(Assignee)

Comment 3

2 years ago
I'm inclined to implement this in Stylo rather than in the existing Gecko style system. 

+cc: Xidorn who's now working on Stylo's StyleSheet handling to put this on his radar.

Updated

2 years ago
Rank: 5 → 51
I wonder how it makes sense to add a style sheet that the page doesn't use at the moment. Is that just for preloading the stylesheet? For that, I think the website should either add a style sheet which is disabled initially, and enable it later, or just use the resouce hint to trigger preload.

I'm not sure whether we've optimized for adding disabled style sheet, but that seems to be a much easier optimization than trying to figure out whether the style sheet could potentially affect the current rendering.
This is apparently very common in large (and widely used!) sites that use large module systems for their content.  They ensure that they styles they add don't apply to the current page, and then add the content to the page that depends on those styles.

Updated

a year ago
Blocks: 1337841
Whiteboard: [platform-rel-Facebook] → [platform-rel-Facebook][qf]
Whiteboard: [platform-rel-Facebook][qf] → [platform-rel-Facebook][qf:p1]
Depends on: 1213122
(In reply to Jet Villegas (:jet) from comment #3)
> I'm inclined to implement this in Stylo rather than in the existing Gecko
> style system. 

Is this bug in the scope of stylo implementation ?
Flags: needinfo?(cam)
It's not one of the optimizations we discussed on Friday, but it's certainly something we could do in Stylo.  (And I agree that putting effort into doing it in Stylo rather than Gecko is the right thing to do.)  Given that this is on the Quantum Flow radar -- Bobby, do you think we should add this to our backlog?
Flags: needinfo?(cam) → needinfo?(bobbyholley)
I would really want to see this done as part of stylo (I thought about doing that as a followup, given there's so much to do already, but we could also try to do it now).

Se an analysis of the blink approach at https://bugzilla.mozilla.org/show_bug.cgi?id=1213122#c34. FWIW they don't have any class to element map, they just do a full DOM traversal. Given we need to potentially do that traversal to do styling too, it seems like a good compromise to me, but we could also have that kind of map.

I think this shouldn't be hard to do, specially for the easy cases like bug 1213122 and facebook (which uses mostly class scopes). I can take this once I'm done with bug 1355343 and dependencies if you want.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> I would really want to see this done as part of stylo (I thought about doing
> that as a followup, given there's so much to do already, but we could also
> try to do it now).
> 
> Se an analysis of the blink approach at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1213122#c34. FWIW they don't
> have any class to element map, they just do a full DOM traversal. Given we
> need to potentially do that traversal to do styling too, it seems like a
> good compromise to me, but we could also have that kind of map.
> 
> I think this shouldn't be hard to do, specially for the easy cases like bug
> 1213122 and facebook (which uses mostly class scopes). I can take this once
> I'm done with bug 1355343 and dependencies if you want.

Yes, I think it makes sense to do this. We can use our parallel traversal machinery to make the traversal fast. It should be easy to add a perf-reftest for this as well.
Flags: needinfo?(bobbyholley)
Filed bug 1357583 for doing this in stylo.
(Assignee)

Updated

a year ago
Assignee: nobody → bugs
Since this is a non-trivial style system change, we should aim to fix this in Stylo first (bug 1357583), and then we can "backport" the fix to Gecko (fixing this bug here) as needed.

--> adding dependency on bug 1357583
Depends on: 1357583
Given comment 11, I'm going to call this [qf:p2] as a Gecko bug.
Whiteboard: [platform-rel-Facebook][qf:p1] → [platform-rel-Facebook][qf:p2]
Just to be clear, we shouldn't have somebody work on this for the Gecko style system unless it becomes clear that stylo won't make 57.
dbaron, what was the original context where we saw this showing up on Facebook? If there's a workflow we can profile, it would be interesting to compare stylo vs gecko given bug 1357583.
Flags: needinfo?(dbaron)
The original context was that we were told about it verbally by folks who work at Facebook.  I think the thing to profile would probably be initial load of https://www.facebook.com/, perhaps with a preference for a slower network connection.  But I'm not entirely sure.
Flags: needinfo?(dbaron)
Given bug 1357583 is fixed (in Stylo) and I don't think we want to do work on the old style system, I'm going to close this.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.