-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the render-blocking mechanism and blocking=render
attribute
#7474
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good. I cannot find any problems with the normative mechanisms; they are very nice and rigorously specified. All that remains is tightening up the attribute definition and some nits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the very detailed review!
I also added handling for the style
element.
Since blocking is allowed to not happen (there is word "should" mentioned, tough that is lowercase in the pr) and I expect browsers will have some timer or such to ensure the page isn't unrendered forever (since I assume the previous page is often shown until the next page can be rendered, and we don't want the old page to render forever) should there be some event hinting that render blocking didn't succeed or was cancelled by the UA? Another option would be to tweak the naming a bit so that the attribute can be thought as a hint to UA. |
I think it's already implied that the UA won't be blocked forever. The Fetch spec allows a fetch group to be terminated, and then this PR unblocks rendering. So UA may use an internal timeout for any fetch (which I believe is what all current browsers do), and then unblocks rendering after the render-blocking resources time out. I will add a note for that. |
Well, is the intent that we only unblock rendering if the fetch times out (and thus the resource never loads successfully)? I thought there was going to be an allowance for a lower implementation-defined timeout, like, "the user has been staring at a blank screen for 10 seconds, but I'm pretty sure we can draw something more useful based on what's downloaded so far, so let's start having rendering opportunities now regardless of the blocking elements". |
For the current MVP, yes. We've thought about this in the proposal, and may allow setting an explicit timeout or other criteria to unblock in a future version. |
Hmm OK, I'm unsure which understanding the Gecko folks had; I thought there was going to be a separate implementation-defined timeout for rendering opportunities in the spec. (And there still could be, if other browsers want one; it's just Chrome might set their value to infinite.) Let's see what they say. In the meantime I'll do the review for the style element parts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style stuff looks great.
Thanks for the review! Before merging:
|
In this PR seems like a good idea.
We can ask @annevk and @smaug---- to chime in here from Mozilla, and/or file an issue on https://github.com/mozilla/standards-positions/ . Similarly, for Apple, we can ask @hober for her thoughts, or email webkit-dev. (Official position requests are required as part of the Chromium shipping process anyway, so it's a good idea.) Note that may lead to additional feedback; it's not just a binary yes/no on the current proposal!
Yes. |
Alright, I've sent requests for position to mozilla/standards-positions and webkit-dev |
I wasn't talking about fetch timeout. A request may just take a lot of time to succeed. And since the page may have started to run scripts at that point, it would be worrisome for the UA to show the previous page. And if previous page wasn't shown, something else needs to be painted. An all white page is one option, but I'd rather paint whatever is already there in the page |
I see. This might be out of the scope of the current MVP, though, which just tries to extend the already existing render-blocking behavior for stylesheets. IIUC in a previous meeting we've discussed the idea of painting something first, then blocking rendering for the critical resources, then resuming rendering after they are loaded. But this is a whole new mechanism, which I think is out of the scope of the current proposal & PR. |
It really doesn't feel like that big of an extension. As I noted in the review above, it's about one extra sentence. And Chromium can just not implement that (i.e., set their implementation-defined timeout to infinite). IMO we should do it. |
Ah, looks like I misunderstood @smaug----'s comment. Now I understand it as "if the render-blocking resources take too long, just unblock rendering", in which case we can just add that sentence as in @domenic's review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated:
- Added an implementation-defined timeout to allow unblocking rendering early
- Made parser-inserted
<link rel=stylesheet>
implicitly render-blocking - Changed callers of
unblock rendering
algorithm not to check whether the element is render-blocking, because this is unnecessary and makes unblocking on implicitly render-blocking stylesheets more complicated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits.
…ing script elements, a=testonly Automatic update from web-platform-tests Add tentative WPT tests for render-blocking script elements Tentative spec proposed at: whatwg/html#7474 Bug: 1271296 Change-Id: I2ab9818ff8c3f39bacdbbe8a8d7db1807e097653 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3441107 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#967534} -- wpt-commits: c5cb41894e3efc5d8e9524b060eff0650f47f3f5 wpt-pr: 32703
…blocking stylesheets, a=testonly Automatic update from web-platform-tests Add some tentative WPT tests for render-blocking stylesheets This patch adds a minimal test suite required by the HTML spec PR for render-blocking: whatwg/html#7474 Current browsers should already pass these tests. Since the Blink test runner forced frame updates, this patch also adds a NoForcedFrameUpdates flag to allow the tests to be run in a virtual test suite that doesn't force frame updates. Bug: 1271296 Change-Id: I66f9c13d461fc4838a26ee3d1a66db9d21289ef6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3417175 Reviewed-by: Xianzhu Wang <[email protected]> Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#963779} -- wpt-commits: 85ed97993fd3bb9ab0975a33a776166ab654f64b wpt-pr: 32553
…ed APIs, a=testonly Automatic update from web-platform-tests Add tentative WPT tests for render-blocked APIs This patch adds more tentative WPT tests to assert that when rendering is blocked [1], the following steps in update the rendering [2] are not run: - Flush autofocus candidates - Resize steps - Scroll steps - Evaluate media queries and report changes - Update animations and send events - Fullscreen steps - Animation frame callbacks - Intersection observer updates Ideally, we should also verify that the canvas context lost steps are not run, but we currently can't consistently reproduce canvas context losts in WPT, so this is not verified. [1] Proposed at whatwg/html#7474 [2] https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering Bug: 1271296 Change-Id: I67efd34cbb1db32be0b59e637cbb26689b89a51e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3425981 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#966570} -- wpt-commits: 23b2b87393ab000d302c3a64bf19d6192a6de1d1 wpt-pr: 32628
…ing script elements, a=testonly Automatic update from web-platform-tests Add tentative WPT tests for render-blocking script elements Tentative spec proposed at: whatwg/html#7474 Bug: 1271296 Change-Id: I2ab9818ff8c3f39bacdbbe8a8d7db1807e097653 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3441107 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#967534} -- wpt-commits: c5cb41894e3efc5d8e9524b060eff0650f47f3f5 wpt-pr: 32703
…blocking stylesheets, a=testonly Automatic update from web-platform-tests Add some tentative WPT tests for render-blocking stylesheets This patch adds a minimal test suite required by the HTML spec PR for render-blocking: whatwg/html#7474 Current browsers should already pass these tests. Since the Blink test runner forced frame updates, this patch also adds a NoForcedFrameUpdates flag to allow the tests to be run in a virtual test suite that doesn't force frame updates. Bug: 1271296 Change-Id: I66f9c13d461fc4838a26ee3d1a66db9d21289ef6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3417175 Reviewed-by: Xianzhu Wang <[email protected]> Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#963779} -- wpt-commits: 85ed97993fd3bb9ab0975a33a776166ab654f64b wpt-pr: 32553
…ed APIs, a=testonly Automatic update from web-platform-tests Add tentative WPT tests for render-blocked APIs This patch adds more tentative WPT tests to assert that when rendering is blocked [1], the following steps in update the rendering [2] are not run: - Flush autofocus candidates - Resize steps - Scroll steps - Evaluate media queries and report changes - Update animations and send events - Fullscreen steps - Animation frame callbacks - Intersection observer updates Ideally, we should also verify that the canvas context lost steps are not run, but we currently can't consistently reproduce canvas context losts in WPT, so this is not verified. [1] Proposed at whatwg/html#7474 [2] https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering Bug: 1271296 Change-Id: I67efd34cbb1db32be0b59e637cbb26689b89a51e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3425981 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#966570} -- wpt-commits: 23b2b87393ab000d302c3a64bf19d6192a6de1d1 wpt-pr: 32628
…ing script elements, a=testonly Automatic update from web-platform-tests Add tentative WPT tests for render-blocking script elements Tentative spec proposed at: whatwg/html#7474 Bug: 1271296 Change-Id: I2ab9818ff8c3f39bacdbbe8a8d7db1807e097653 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3441107 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#967534} -- wpt-commits: c5cb41894e3efc5d8e9524b060eff0650f47f3f5 wpt-pr: 32703
This patch adds a minimal test suite required by the HTML spec PR for render-blocking: whatwg/html#7474 Current browsers should already pass these tests. Since the Blink test runner forced frame updates, this patch also adds a NoForcedFrameUpdates flag to allow the tests to be run in a virtual test suite that doesn't force frame updates. Bug: 1271296 Change-Id: I66f9c13d461fc4838a26ee3d1a66db9d21289ef6 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3417175 Reviewed-by: Xianzhu Wang <[email protected]> Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#963779} NOKEYCHECK=True GitOrigin-RevId: c7ceefca8b78815cf41c3ebbf5d706ef04afe60c
This patch adds more tentative WPT tests to assert that when rendering is blocked [1], the following steps in update the rendering [2] are not run: - Flush autofocus candidates - Resize steps - Scroll steps - Evaluate media queries and report changes - Update animations and send events - Fullscreen steps - Animation frame callbacks - Intersection observer updates Ideally, we should also verify that the canvas context lost steps are not run, but we currently can't consistently reproduce canvas context losts in WPT, so this is not verified. [1] Proposed at whatwg/html#7474 [2] https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering Bug: 1271296 Change-Id: I67efd34cbb1db32be0b59e637cbb26689b89a51e Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3425981 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#966570} NOKEYCHECK=True GitOrigin-RevId: 867f21a67f494ae21a6ebbb3547baeb6f5ed7cbd
Tentative spec proposed at: whatwg/html#7474 Bug: 1271296 Change-Id: I2ab9818ff8c3f39bacdbbe8a8d7db1807e097653 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3441107 Reviewed-by: Domenic Denicola <[email protected]> Commit-Queue: Xiaocheng Hu <[email protected]> Cr-Commit-Position: refs/heads/main@{#967534} NOKEYCHECK=True GitOrigin-RevId: 9a718bf036ecaeae7cc4692183d8caec1ce391a1
These tests were added in the follow PRs: web-platform-tests#32553 web-platform-tests#32669 web-platform-tests#32703 web-platform-tests#32809 web-platform-tests#33291 web-platform-tests#33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857
These tests were added in these PRs: web-platform-tests#32553 web-platform-tests#32669 web-platform-tests#32703 web-platform-tests#32809 web-platform-tests#33291 web-platform-tests#33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857
These tests were added in these PRs: #32553 #32669 #32703 #32809 #33291 #33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857
…r tentative, a=testonly Automatic update from web-platform-tests Mark most render-blocking tests no longer tentative (#43113) These tests were added in these PRs: web-platform-tests/wpt#32553 web-platform-tests/wpt#32669 web-platform-tests/wpt#32703 web-platform-tests/wpt#32809 web-platform-tests/wpt#33291 web-platform-tests/wpt#33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857 -- wpt-commits: aa6bdcbb00f30914b87794cf82b015fc02d4cc5b wpt-pr: 43113
…r tentative, a=testonly Automatic update from web-platform-tests Mark most render-blocking tests no longer tentative (#43113) These tests were added in these PRs: web-platform-tests/wpt#32553 web-platform-tests/wpt#32669 web-platform-tests/wpt#32703 web-platform-tests/wpt#32809 web-platform-tests/wpt#33291 web-platform-tests/wpt#33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857 -- wpt-commits: aa6bdcbb00f30914b87794cf82b015fc02d4cc5b wpt-pr: 43113
…r tentative, a=testonly Automatic update from web-platform-tests Mark most render-blocking tests no longer tentative (#43113) These tests were added in these PRs: web-platform-tests/wpt#32553 web-platform-tests/wpt#32669 web-platform-tests/wpt#32703 web-platform-tests/wpt#32809 web-platform-tests/wpt#33291 web-platform-tests/wpt#33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857 -- wpt-commits: aa6bdcbb00f30914b87794cf82b015fc02d4cc5b wpt-pr: 43113
…r tentative, a=testonly Automatic update from web-platform-tests Mark most render-blocking tests no longer tentative (#43113) These tests were added in these PRs: web-platform-tests/wpt#32553 web-platform-tests/wpt#32669 web-platform-tests/wpt#32703 web-platform-tests/wpt#32809 web-platform-tests/wpt#33291 web-platform-tests/wpt#33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857 -- wpt-commits: aa6bdcbb00f30914b87794cf82b015fc02d4cc5b wpt-pr: 43113
…r tentative, a=testonly Automatic update from web-platform-tests Mark most render-blocking tests no longer tentative (#43113) These tests were added in these PRs: web-platform-tests/wpt#32553 web-platform-tests/wpt#32669 web-platform-tests/wpt#32703 web-platform-tests/wpt#32809 web-platform-tests/wpt#33291 web-platform-tests/wpt#33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857 -- wpt-commits: aa6bdcbb00f30914b87794cf82b015fc02d4cc5b wpt-pr: 43113 UltraBlame original commit: 58cb91eda01d9227e0e02791aa6562da04967a42
…r tentative, a=testonly Automatic update from web-platform-tests Mark most render-blocking tests no longer tentative (#43113) These tests were added in these PRs: web-platform-tests/wpt#32553 web-platform-tests/wpt#32669 web-platform-tests/wpt#32703 web-platform-tests/wpt#32809 web-platform-tests/wpt#33291 web-platform-tests/wpt#33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857 -- wpt-commits: aa6bdcbb00f30914b87794cf82b015fc02d4cc5b wpt-pr: 43113 UltraBlame original commit: 58cb91eda01d9227e0e02791aa6562da04967a42
…r tentative, a=testonly Automatic update from web-platform-tests Mark most render-blocking tests no longer tentative (#43113) These tests were added in these PRs: web-platform-tests/wpt#32553 web-platform-tests/wpt#32669 web-platform-tests/wpt#32703 web-platform-tests/wpt#32809 web-platform-tests/wpt#33291 web-platform-tests/wpt#33798 These were developed against HTML PRs which are now merged: whatwg/html#7474 whatwg/html#7603 whatwg/html#7857 -- wpt-commits: aa6bdcbb00f30914b87794cf82b015fc02d4cc5b wpt-pr: 43113 UltraBlame original commit: 58cb91eda01d9227e0e02791aa6562da04967a42
For #7131
(See WHATWG Working Mode: Changes for more details.)
/dom.html ( diff )
/index.html ( diff )
/indices.html ( diff )
/links.html ( diff )
/parsing.html ( diff )
/scripting.html ( diff )
/semantics.html ( diff )
/urls-and-fetching.html ( diff )
/webappapis.html ( diff )