CSS is renderblocking regardless of link media attribute

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
8 months ago

People

(Reporter: domfarolino, Assigned: emilio)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix, firefox58 ?, firefox61 fixed)

Details

()

Attachments

(1 attachment)

This bug is the result of a small conversation with bz in #content after I first brought this up in #whatwg (freenode).

Some people teach that to minimize the amount of CSS that is in the critical rendering path (amt of renderblocking CSS) a technique is to use the `media` attribute on `<link rel=stylesheet ...>` to let the browser be smart about which CSS files it should block rendering on. As it stands, Chrome Canary and Safari only block rendering on the loading/processing of stylesheets loaded via link elements whose media attr value matches that of the current UA environment. In Firefox we block rendering regardless of this value when it appears this is not necessary.

An example of this can be seen by opening a new tab, viewing the network pane in the devtools and visiting `https://shocking-territory.glitch.me/` to see how many CSS resources (each taking longer to load than the previous) must be loaded and processed before the initial page render. As a result of Firefox's behavior, the initial page render is much slower than that of Canary + Safari when it probably shouldn't be.

As bz mentioned the HTML spec should probably be changed to indicate this (bug: https://github.com/whatwg/html/issues/2886) BUT this can also be valid behavior (probably, boris?) without a spec change given some hand-wavy wording etc.
Priority: -- → P3
Sounds like a good thing to do, though I don't know if we have access to the media list early enough during stylesheet URI loading to do this. heycam: wdyt?
Flags: needinfo?(cam)
Seems reasonable to me.  We do have access to the media list before we kick off the network load.  I'm not sure what the exact mechanism is that we use to make style sheet loads block rendering.  (I thought they only affected document load event dispatch, and I didn't think that could delay painting by this long.)  I'll take a look when I get a moment.
Assignee: nobody → cam
Flags: needinfo?(cam)
I can describe that mechanism.

What stylesheets block is calls to PresShell::Initialize, and hence initial frame construction and hence layout.  That happens due to nsContentSink::StartLayout calling nsContentSink::WaitForPendingSheets, which checks nsContentSink::mPendingSheetCount.

mPendingSheetCount is manipulated by three things:

1) Calls nsStyleLinkElement::UpdateStyleSheet that set *aIsAlternate to false (implying that the sheet should block layout).

2) Calls to css::Loader::LoadStyleLink directly from nsContentSink::ProcessStyleLink that set *aIsAlternate to false.

3) nsContentSink::StyleSheetLoaded, which is a notification the CSS loader sends for things that it claimed were not alternate sheets.

So in practice, you will probably want to rename UpdateStyleSheet's aIsAlternate member to have the "is alternate or for an irrelevant medium" semantics, double-check that all the callers are OK with those semantics, then modify the CSS loader to set that boolean accordingly based on media.  Whether you also change these different-media sheets to have the lower-priority loading alternate sheets have is a bit less clear.  If you do, you'll just want to double-check that Loader::SetPreferredSheet handles them reasonably.

We really need to simplify the CSS loader.  :(

Comment 4

Last year
(Note that the HTML Standard since changed to explicitly allow this, thanks to OP.)
Thanks for the description and all, bz. I should probably mention that I might no longer be able to get to this. I won't have enough time for probably a while if someone wanted to take it instead, else it'll be a while for me to commit the time to this. Thanks.
Ah, I just realized this was not assigned to me; whoops. Looks like I just assumed it was. Sorry!
Flags: needinfo?(emilio)
See Also: → 861870
Depends on: 1456435
Assignee: cam → emilio
Flags: needinfo?(emilio)
Comment hidden (mozreview-request)
Suggestions for testing this would be appreciated. I couldn't think of a way that wasn't racy...
Also, note that this is a best-effort thing, not flushing layout at the point of the load, per the perf issues highlighted in bug 861870. I don't think it's a huge issue to have layout up-to-date at the point of the load, since you can conceivably change it while the load is going on, and you can't suddenly start blocking again, for example... But let me know if you think otherwise.

FWIW, the test-case I've used to verify this is working as intended is a simple slow.php:

  <?php header('Content-Type: text/css'); sleep(5); ?>

And a test.html page that looks like:

  <!doctype html>
  <script>let start = Date.now();</script>
  <link rel="stylesheet" href="slow.php" media="print">
  <div>Hey! Hopefully seeing me before 5 seconds</div>
  <script>document.write(Date.now() - start);</script>

Which you can test locally with:

  php -S localhost:8000

And browsing to localhost:8000/test.html

Comment 11

Last year
mozreview-review
Comment on attachment 8970640 [details]
Bug 1386840: Defer loading and don't block rendering for non-matching stylesheets.

https://reviewboard.mozilla.org/r/239384/#review245578

r=me with the "change all the constructors" issue fixed.

Thank you for picking this up!

::: layout/style/Loader.cpp:167
(Diff revision 1)
>    , mIsLoading(false)
>    , mIsBeingParsed(false)
>    , mIsCancelled(false)
>    , mMustNotify(false)
>    , mWasAlternate(aIsAlternate)
> +  , mMediaMatched(aMediaMatches)

You need to set mMediaMatched in the other two constructors too, right?

For that matter, should the @import case get deferred as needed too?  That one is complicated because it will keep the parent sheet from applying...

::: layout/style/Loader.cpp:2104
(Diff revision 1)
>                                            aObserver, principal, requestingNode);
>    NS_ADDREF(data);
>  
> -  // If we have to parse and it's an alternate non-inline, defer it
> +  auto result = LoadSheetResult { Completed::No, isAlternate, matched };
> +
> +  MOZ_ASSERT(!result.ShouldBlock() == data->ShouldDefer(),

For some reason I find "result.ShouldBlock() == !data->ShouldDefer()" more readable... either way, though.

::: layout/style/Loader.cpp:2112
(Diff revision 1)
> +  // If we have to parse and it's a non-blocking non-inline sheet, defer it.
>    if (aURL &&
>        state == eSheetNeedsParser &&
>        mSheets->mLoadingDatas.Count() != 0 &&
> -      isAlternate == IsAlternate::Yes) {
> +      !result.ShouldBlock()) {
>      LOG(("  Deferring alternate sheet load"));

Fix the log message?
Attachment #8970640 - Flags: review?(bzbarsky) → review+
As far as testing goes, you could try something like this:

  <link id="test" rel="stylesheet" href="something" media="non-matching"></link>
  <script>
    var link = document.getElementById("test");
    // now either link.sheet should be null or link.sheet.cssRules should throw.
  </script>

This can still be racy in that the sheet load could complete before the </script> bit is reached...  If you really want to avoid that, you might be able to have an inline script document.write() the above snippet; pretty sure that should be reliable in Gecko's current setup.

Basically, the idea is to test whether script execution is blocking on the sheet load, which it shouldn't with these changes.
It's a bit more work, but you could also write a httpd.js-based test, if you want more control of when the linked style sheet finishes loading:

https://developer.mozilla.org/en-US/docs/Mozilla/httpd.js/HTTP_server_for_unit_tests

e.g. like test_load_events_on_stylesheets.html uses slow_ok_sheet.sjs.

Comment 14

Last year
mozreview-review
Comment on attachment 8970640 [details]
Bug 1386840: Defer loading and don't block rendering for non-matching stylesheets.

https://reviewboard.mozilla.org/r/239384/#review245652

r=me but please add tests.

::: layout/style/Loader.cpp:148
(Diff revision 1)
>                               const nsAString& aTitle,
>                               nsIURI* aURI,
>                               StyleSheet* aSheet,
>                               nsIStyleSheetLinkingElement* aOwningElement,
>                               bool aIsAlternate,
> +                             bool aMediaMatches,

As in the previous bug, might be good to use the MediaMatches type here.
Attachment #8970640 - Flags: review?(cam) → review+

Comment 15

Last year
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0741467f091
Defer loading and don't block rendering for non-matching stylesheets. r=bz,heycam
https://hg.mozilla.org/mozilla-central/rev/b0741467f091
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee

Updated

11 months ago
Duplicate of this bug: 861870
Assignee

Updated

8 months ago
Duplicate of this bug: 687000
You need to log in before you can comment on or make changes to this bug.