`blocking=render` should override fetch priority
xiaochengh opened this issue · 33 comments
We recently introduced the concept of render-blocking. Since marking an element as render-blocking indicates that the resource is important, it does not make sense if we fetch the resource with a low fetch priority.
I'm going to put up a PR about this. The basic idea is that if an element has both fetchpriority=low and blocking=render attrbutes, then we'll ignore fetchpriority=low and fetch the resource with auto priority.
Would it make sense to apply this more generally? e.g. if we have a render blocking script with fetchpriority=low, implementations are likely to also ignore the hint. Maybe we can add that logic to the spec as well.
I don't know that it needs to be called out specifically, at least in the priority-hints spec. The hinted fetch priority is already only taken as one of many signals that is considered when setting the actual priority. Specifically, it's step 6 of the fetch integration where the internalpriority takes the priority hint as input.
What we probably want is just for the fetch integration to also take in render-blocking as a signal when setting internal priority and leave it at that.
(This issue seems to be a better place for discussions so I'll also discuss here)
Thanks for the comments!
What we probably want is just for the fetch integration to also take in render-blocking as a signal when setting internal priority and leave it at that.
This also makes more sense to me than overriding the priority hint. Thanks for the idea!
So should we also add a flag to Request to indicate render-blocking behavior?
(Btw we already have such a flag in Blink's implementation)
I think adding a read-only attribute to fetch Request makes sense and makes the spec cleaner as far as how it integrates with fetch. That also opens up the possibility to extend it to being set on fetches if we want it to be.
I think what we have here is just a staging problem.
Right now Fetch takes no input from anyone on how to determine what it currently calls priority.
After both blocking=render is integrated, and Priority Hints is integrated, then we will rename that to internalpriority, and we will use both the fetchpriority="" attribute state and the blocking="" attribute state to determine internalpriority. That will all happen in the sections of the HTML spec that perform fetches for the link, style, and script elements. (The iframe element will only take the fetchpriority="" attribute state into account.)
But right now blocking=render is integrated into HTML, and fetchpriority="" is not. We thus have several options for how to integrate:
-
Update HTML to compute fetch priority from blocking="" for link, style, and script. Update Priority Hints to be rebased on top of that new version of HTML, so that it takes into account both blocking="" and fetchpriority="" when computing internalpriority.
-
Leave HTML as-is for now. Update Priority Hints to compute internalpriority based on both blocking="" and fetchpriority="".
I was advising (2) based on the fact that fetch priority right now is very vague and it didn't make much sense to just make it a bit more rigorous, when we knew a big change was coming down the pipeline with Priority Hints. It would also be less work for everyone involved: we just integrate #70 and then later merge Priority Hints into HTML, instead of editing HTML, rebasing Priority Hints to include the equivalent of those HTML edits, and then later merging Priority Hints into HTML.
But, I admit doing (1) is a bit more proper. After all, blocking=render is an existing part of the HTML spec, and the fact that its influence on fetch priority is not yet specced is a gap that we should perhaps fix ASAP. Let me know what you prefer.
So should we also add a flag to Request to indicate render-blocking behavior?
I don't think this is necessary, if the only thing it influences is request priority. For example it isn't something that we'd let people set with fetch(), IMO.
Step #15 of the Fetching process model of fetch already has the setting of the priority. Priority-hints renames it to internalpriority to eliminate the name collision but the aggregation of data sources for setting the internal priority is already part of fetch.
The priority-hints updates won't really help because you'd still need to plumb blocking=render through to fetch for the existing supported element types, just updating the existing priority part of the fetch spec to take it as input. priority-hints can then rename it (and adopt the updated language to include blocking) as it is merged in.
As far as not updating fetch, I don't think there is a way to plumb the blocking=render state into fetch without adding it to Request in some form and updating the processing model on the HTML side to pass the state info to fetch (pretty much mirroring what priority-hints had done).
The way to plumb blocking=render into fetch without updating fetch is the same way you plumb fetchpriority="" into fetch without updating fetch. You just modify the priority you pass in. This does not require adding a new field.
Anyway, if you prefer (1), we can do that instead of (2). It'll just be more work (including for the priority hints spec).
fetchpriority added a new priority field to fetch and renamed the existing one (and updated the HTML processing models to set the new priority-hints priority attribute). The existing priority field is an opaque vendor-specific placeholder that doesn't take input from any of the HTML spec directly.
Right, but now it will.
BTW I realized a concrete difference here between what you're proposing and what is in #70 is that in a service worker, in your version, it will be possible to observe request.priority === "low" for a blocking=render resource. That doesn't seem desirable to me, but perhaps it is to you and to web developers, I'm not sure.
I don't see a way to plumb blocking=render to fetch requests without updating the fetch spec, either. Did I miss anything?
If we need to update the fetch spec, the most straightforward thing (to me) to do is to standarize the RenderBlockingBehavior flag that we already have, and add it to fetch requests.
it will be possible to observe request.priority === "low" for a blocking=render resource
Maybe rename priority to priorityHint? Since it's not the final priority but just a developer-provided hint.
Anyway, this is already orthogonal to render-blocking.
FWIW, it was named priority after much bikeshedding because it is the developer-requested priority, either through the fetchPriority HTML attribute or as a proprty of the Request on the fetch() API call directly (and it is developer-exposed for setting).
+1 to standardize the blocking property/flag values and adding it to fetch as a read-only attribute (unless you want to allow for it to be set on fetch() requests by developers as well).
Hmm, well, I still feel that having both blocking="" and fetchpriority="" contribute to the priority/internalpriority is the best approach. But if you both feel the opposite way, then we should get @annevk's opinion as Fetch editor to make the final call.
Okay, so we have these concepts:
- Developer-controlled "fetch priority" (enum)
- Developer-controlled "blocking" (boolean)
- Implementation-controlled "actual priority" (any)
And it sounds like if "blocking" is used, then "fetch priority" ends up ignored. ("Actual priority" would take both into account as well as other information. I'm not sure it makes sense for callers of fetch to directly set "actual priority" as currently designed. It doesn't really have a meaningful type.)
So the question is whether developers should get to learn (in service workers) whether "fetch priority" is ignored. Several alternatives were proposed:
- Make it clear "fetch priority" is a hint through naming.
- Also expose "blocking" on
Requestobjects and require developers to look at both "fetch priority" and "blocking". - Make "blocking" change the computed value of "fetch priority".
On the face of it 3 seems rather attractive as developers would only have to care about a single field. It also more accurately reflects how an implementation would look at it. However, as "blocking" is still relevant for implementations, perhaps 2 ought to be done as well at some point? I.e., have
- Make "blocking" change the computed value of "fetch priority", but also consider exposing "blocking" on
Requestobjects as it does have an impact on things.
I don't think 1 is great as it doesn't make it clear when it will be actively ignored and only doing 2 seems like giving developers complexity we could have resolved.
Hope that helps.
Then it sounds like we have two separate issues:
- Whether to add a developer-controled flag ("blocking", "renderblocking", etc, TBD) to
Requestobjects, and make it affect the "actual priority". This is part of the FETCH spec - Whether to change the computed value of "fetch priority" when element is render-blocking
I think we have all agreed on 1? If so, I'll start working on a FETCH spec PR, and we can leave 2 open here.
I think passing "blocking" through to fetch as its own field on request concepts is reasonable as the browser needs that information, but for exposing it on Request objects we should have a compelling use case of sorts first, I think. And that use case should be separate from needing to know the rough priority as 3 (from my comment) would also address that.
@annevk As far as I know, there were no plans to have a "computed" version of the fetch priority aside from the opaque implementation-defined internal priority and that the priority that exists in fetch is a pass-through of what is set in HTML as fetchPriority for the elements that support it and that render-blocking and priority would both influence the computed internal-priority but render-blocking may not necessarily be exposed to JS (doesn't look like it is currently).
Are you recommending that we change that and have the computation be performed before the attribute is passed from html to fetch by evaluating both fetchPriority and blocking at the time that HTML creates the Request and have that set the priority that is exposed to code as readable on the request object?
Either way is doable for Chrome's implementation since we don't yet expose the attribute on Request itself (just RequestInit) but I was assuming that the raw inputs would be maintained and passed through and the implementation-specific bit would be on the way out of the net stack below fetch.
If fetch's read-only priority attribute on request is a form of the computed priority, it could be a bit more complicated since Chrome's computation takes the destination, initiator, request-blocking and fetch-priority (as well as a few other factors like where the element was discovered in the DOM) and the visible size of the element into account before calculating the real computed priority and that computed priority has a lot more levels than the high/low that is exposed through the current enum.
We could split the logic in two and have blocking merge with fetchPriority explicitly for the priority that is exposed to JS on the Request object and document the effect that blocking should have on the computed priority but I'm not sure that all implementors would want to make the evaluation early and lose the discrete states when computing the actual internal priority.
@valenting FYI
HTML passes two request signals to Fetch, fetch priority and blocking. Fetch combines these into an internal priority it can use for when to invoke service workers (presumably, though this isn't defined) and networking. Fetch also takes care to expose the fetch priority (not the internal priority) to service workers.
It's my understanding that when you have a blocking signal, the fetch priority is completely ignored. From that perspective it makes sense to me that Fetch would "normalize" fetch priority when it gets a blocking signal. This would only be observable in service workers I think.
I think it's safe to say "for font preloads when render-blocking is set then fetch priority is implied to be high". I'm not positive that that will be the case for all other surfaces where blocking is specified (though to be fair, the internal fetch state is specifically render-blocking and not some other not-yet-implemented value like document or DOM).
As far as I know, Chrome has really only implemented blocking=render for preloaded fonts but the HTML spec allows for it to be set in a lot of other cases, including implicitly.
Even for render-blocking cases, I don't know that I can fairly speak for all engines that render-blocking=true will always be the same as priority=high. Maybe it's good enough for the purposes of what is exposed to a worker for either decision-making or creating a new fetch that doesn't use the existing request for initialization.
One edge case that comes to mind is if a script has both defer and blocking=render but there are more defer scripts ahead of it in the dom. Presumably those will still need to be executed in order but you wouldn't want to fetch the last one before the ones before it. You could retroactively upgrade the previous fetches to also be render-blocking but there's no good mechanism for that (and they may have already been processed).
I think I'd be more comfortable either exposing render-blocking as a separate read-only attribute on Request or not exposing priority rather than having render-blocking implicitly modify priority directly. Not exposing priority feels like it would be limiting some useful information the workers could use for derivative fetches but maybe it's worth waiting to see if there is a need before exposing it.
That reads a bit different from what @xiaochengh said upthread. @xiaochengh thoughts on the above?
I now think this is equivalent to: do we simply want to expose the impl-defined internal priority or not? If there's any prior discussion, its conclusion should still apply here. There's no need for a specific discussion that involves blocking.
The reason si that the fetch spec already allows this internal priority to differ from the exposed priority in an impl-defined way. The involvement of blocking doesn't make a crucial difference here.
Also, the primary reason I filed this issue was to support plumbing render-blocking to a request to allow overriding the actual priority, and at that time I didn't realize the difference between the request concept and the Request object (see this comment). Since the task was already done by whatwg/fetch#1432, I think this issue can be closed.
I don't see how we could expose "internal priority".
As I saw it this discussion was about "blocking" invalidating/obviating "priority" and as such that should probably be exposed directly rather than requiring everyone to check both fields. Notwithstanding that we're not exposing "blocking" at the moment.
Probably a disconnect somewhere along the lines (and didn't help that what priority refers to is in-flight and changing).
For Chrome's implementation, internal priority will likely look at render-blocking first when making decisions and priority will usually not impact the decision if render-blocking is set. That said, it won't explicitly override or ignore priority, the input signal from it will just be less useful.
I'd be more inclined to either expose both render-blocking and priority on Request or neither rather than have priority take on a value that doesn't match what engines are going to actually do.
Chrome doesn't currently expose priority on Request and most use cases are theoretical. It could make sense to hold back any changes to what is exposed on Request until concrete needs arise.
Until then, down-stream requests can still adopt the prioritization of intercepted requests by using the intercepted request to initialize the fetch. There just wouldn't be signal available if they need to alter the priority of independent fetches based on signals from the intercepted request.
I don't see how we could expose "internal priority".
I see exposing the "computed priority" sort of exposing the internal priority. At least the idea seems the same.
I'm inclined to not doing this, though I don't have a strong opinion here.
As I saw it this discussion was about "blocking" invalidating/obviating "priority" and as such that should probably be exposed directly rather than requiring everyone to check both fields. Notwithstanding that we're not exposing "blocking" at the moment.
I'm fine with either way, exposing blocking or not. I don't have a particular use case to support (as my case has been resolved) and I'm not familiar how the other use cases are like (service worker?), so feel free to leave me out of this discussion :)
@pmeenan so you're saying we'd only have the HTML attribute and Fetch has "blocking", "priority", and "internal priority" as private fields on requests only, not exposed as a public API?
Reducing scope seems definitely okay to me.
We should file a clear follow-up issue on the API though (probably against whatwg/fetch) in case we eventually want to expose it to document the tradeoffs we need to consider. In particular I think @domenic's concern above about a service worker seeing blocking=true and priority=low is something we need to consider carefully.
Fetch should still have priority as a public API on RequestInit to be able to hint at higher and lower priority fetches for whatever content type they are requesting.
The main question appears to be if the consumers need to be able to read any or some of the internal fields for making downstream decisions when intercepting requests.
Hmm, I guess there is some precedent for that (window), but largely that's not something we've been doing. It might be okay though. Same comment about follow-up issue applies.
I also just noticed that if you forward render-blocking fetches in a service worker the render-blocking state is lost. That's probably a bug? (Step 12 of https://fetch.spec.whatwg.org/#dom-request doesn't forward the state.)
At this point, now that we seem to be treating all three fields as orthogonal, I think it's most straightforward to expose priority as a public API on both RequestInit and Request. Unlike blocking, it doesn't seem particularly complicated.
I'd rather not expose priority on Request without blocking. Given that in most cases where blocking=true it'll end up ignored, I think that would lead to confusion in a service worker. (I agree that exposing blocking fully may be tricky though.)
I feel bad I made @pmeenan write a bunch of web platform tests for exposing priority in a service worker then. (And also testing, via service worker, that fetchpriority="" set things in a good way.) But I guess the conclusion is reasonable...
So the plan is:
- Expose
priorityinRequestInit(and also viafetchpriority=""HTML attribute) - Do not expose either
priorityorblockingonRequestfor now - Treat all three of
priority,blocking, and "internal priority" as separate inputs the UA uses to make prioritization decisions? (Not entirely sure on this.) - Make sure service workers properly pass through all three of them from their input argument by modifying step 12 of https://fetch.spec.whatwg.org/#dom-request appropriately.
Is that correct?
As long as we don't expose priority and blocking, passing along "internal priority" would suffice as it's the sum of those inputs plus others. And we are doing that (although it's still called priority) so I suspect there's no problem for blocking at the moment and I was wrong above. And "internal priority" is what UAs can use then for scheduling and transport layer settings.
Otherwise that seems correct, yeah.
Much happier to write tests that weren't needed than to expose something to the web that we end up regretting so I'm happy to make the changes to the tests and PR.
internal priority is the result of the prioritization decisions as @annevk mentions so it's not necessarily needed to cascade priority and render-blocking in step 12 of https://fetch.spec.whatwg.org/#dom-request but it kind of feels like they would be left in an undefined/default state if they weren't explicitly copied over as well.
I'll go ahead and make the suggested changes to the PR and include the copies in step 12 for now but I can also remove them if we think they should explicitly not be copied. I'll also remove the web platform tests for priority being exposed on Request.
I think this can be closed given where we ended up.