Add may-block reasons to NotRestoredReasons #10154
Conversation
There was a problem hiding this comment.
Thanks for doing all this work! I think this is crucial to getting interoperability if other browsers want to implement the bfcache blocking reasons API.
I know the most important thing to get settled here are the names. I added various small comments about some problematic ones, but I additionally have the following more wide-ranging concerns:
-
Insufficient consistency around reasons related to the initial navigation and the HTTP response. I think all of the following are related to properties of the HTTP response:
http-status-not-ok,non-http-https-scheme,http-not-get,no-response-head,cache-control-no-store,cache-control-no-cache,http-auth-required. Sometimes you prefix withhttp-, sometimes you don't. Sometimes you includeresponse, sometimes you don't. Forstatus-not-ok, you mention the noun, but fornot-get, you don't.My suggestions:
response-status-not-ok,response-scheme-not-http-or-https,response-method-not-get,response-head-missing(?),response-cache-control-no-store,response-cache-control-no-cache,response-auth-required. -
Inconsistency on how API names are incorporated. So far we have one API name reason,
websocket, derived from the API namedWebSocket. Some of your new reasons match this:serviceworker-*,webtransport,broadcastchannel. Some introduce hyphens:SharedWorker->shared-worker,MediaStream->media-stream. And for some I can't find the connection to the objects at all:SmartCardConnection->smart-card,IdleDetector->idle-manager,SpeechRecognition->speech-recognizer,PaymentRequest->payment-manager,OTPCredential->sms. (I wonder if in these case it's some Chromium internal codebase object?) Also, ??? ->indexed-db-event. Finally, some cases use specification names:picture-in-picture,keyboard-lock,webserial,webrtc.My suggestion would be to pick one of two paths here. If the failure is due to a very specific object being used, e.g. a
WebSocketor aSpeechRecognitionobject, then use that object's name, converted to all-lowercase with no hyphens. Otherwise, if tons of different APIs from a spec cause bfcache failures and you want to use one umbrella reason for all of them, then use the specification name. Prefer the former when possible, because it helps avoid exposing existing inconsistencies in how we name specs.
My next biggest concern after these inconsistencies is about the cases I've identified where I have no idea what the reason is indicating. I suspect if I don't know, other implementers won't, and so we won't get interoperability. We can help these cases by expanding on the descriptions until they become clearer.
In the end we may need more cross-specification links to do this. But I know adding those is a pain, so it's fine to leave that until the end of the process.
source
Outdated
| and reached the timeout limit.</dd> | ||
|
|
||
| <dt>"session-restored"</dt> | ||
| <dd>Session was restored for this page.</dd> |
There was a problem hiding this comment.
This means the page is loaded via session history because of tab restore. Reworded this.
|
Thanks Domenic for the review! I think I addressed all of your comments. I changed the reason name to use object name as much as possible, and only when that's not possible the name is the generic API name. |
|
@smaug---- Can you PTAL at this? Thanks in advance! |
There was a problem hiding this comment.
Great work. I can understand almost all of these reasons now, and the naming feels very consistent. The work on the service worker reasons in particular is very, very helpful.
I left a lot of editorial comments, but to highlight the important ones that impact reason names or similar:
- We should remove
database cookie-flushedis a bit strange; maybe-deletedor-removedwould be betterkeepalive-request->response-keepalive, I think? If I understand it correctly?- I don't understand
response-head-missing loading+subframe-navigatingshould probably become eithernavigating+frame-navigating, orloading+frame-loading- I'm not sure I understand
timeout
Note to @smaug----, with my HTML editor hat on:
This list includes a lot of reasons that are specific to Chromium implementation choices. I strongly believe that's OK, and furthermore I commend the Chromium team and @rubberyuzu for bringing this through the standards process, creating this sort of centralized registry. We should encourage this style, instead of specs which have free-form fields for this sort of analytics data. So we should not criticize some of these reasons as Chromium-only. I think that goes even for Chromium-only reasons based on Chromium-only specs (e.g. smart card API). It's better to have everything documented and centralized. Having a smartcardconnection bfcache blocking reason in HTML, doesn't sanction the existence of the Smart Card API in general.
If any other browser engine, like Gecko, wants to add their own very-specific reasons to the list, we should welcome that too.
But we would very much appreciate eyes from a second implementer to ensure that the reasons we're adding here are reusable if other browsers end up with similar implementation constraints. This means, basically, that the descriptions given here are clear enough to be interoperably implementable.
What I'm picturing here is a web developer, staring at their mountains of analytics data, and doing things like:
- Noticing that reason X is very high in both Firefox and Chrome. So they should fix their site!
- Noticing that reason X is high in Chrome, but not in Firefox. So they should bug Chromium developers to adopt whatever strategy Firefox took to avoid the problem!
These kind of use cases are enabled by us collaborating on the reason list, instead of letting it be a free-for-all string.
source
Outdated
| <dt>"<dfn | ||
| data-x="blocking-non-trivial-bcg" export> | ||
| <code>non-trivial-browsing-context-group</code></dfn>"</dt> | ||
| <dd>The <span>browsing context group</span> of this <code>Document</code> was non-trivial.</dd> |
There was a problem hiding this comment.
This means that related active contents exists for this document, i.e. the document called window.open().
rubberyuzu
force-pushed
the
nrr-add
branch
3 times, most recently
from
9ae1fff
to
6abbb90
Compare
This CL renames the reporting strings of NotRestoredReasons API so that they match the spec draft[1]. [1]: whatwg/html#10154 Bug:331754704 Change-Id: If805ad7e30c14ff64b440c8d9515eaee38370fd8
This CL renames the reporting strings of NotRestoredReasons API so that they match the spec draft[1]. [1]: whatwg/html#10154 Bug: 331754704 Change-Id: If805ad7e30c14ff64b440c8d9515eaee38370fd8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5392283 Commit-Queue: Yuzu Saijo <yuzus@chromium.org> Reviewed-by: Rakina Zata Amni <rakina@chromium.org> Cr-Commit-Position: refs/heads/main@{#1280059}
This CL renames the reporting strings of NotRestoredReasons API so that they match the spec draft[1]. [1]: whatwg/html#10154 Bug: 331754704 Change-Id: If805ad7e30c14ff64b440c8d9515eaee38370fd8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5392283 Commit-Queue: Yuzu Saijo <yuzus@chromium.org> Reviewed-by: Rakina Zata Amni <rakina@chromium.org> Cr-Commit-Position: refs/heads/main@{#1280059}
This CL renames the reporting strings of NotRestoredReasons API so that they match the spec draft[1]. [1]: whatwg/html#10154 Bug: 331754704 Change-Id: If805ad7e30c14ff64b440c8d9515eaee38370fd8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5392283 Commit-Queue: Yuzu Saijo <yuzus@chromium.org> Reviewed-by: Rakina Zata Amni <rakina@chromium.org> Cr-Commit-Position: refs/heads/main@{#1280059}
|
@smaug---- Please let me know if there's anything unclear. Thanks! |
|
Thanks Domenic and sorry for not noticing your review earlier! |
There was a problem hiding this comment.
This LGTM with a nit. I do not have any more substantial feedback or any rename suggestions.
I think it would be nicer for spec readers if everything that talked about another spec had a reference, like we currently do for "websocket" with [WEBSOCKETS] and "lock" with [WEBLOCKS].
However, adding these is such an annoying process when editing HTML. And they are only used in this one context. I don't feel like it should be required to merge this PR.
As a compromise, I would appreciate it if you could add such references for specs that are already in the bibliography. So e.g. add various [SERVICEWORKERS] and [INDEXEDDB] and so on. But you don't need to add new bibliography entries for every single reason.
|
Thanks! I added existing spec references. @smaug---- Can you PTAL? Thanks in advance |
…ec, a=testonly Automatic update from web-platform-tests [bfcache] Rename reasons to match the spec This CL renames the reporting strings of NotRestoredReasons API so that they match the spec draft[1]. [1]: whatwg/html#10154 Bug: 331754704 Change-Id: If805ad7e30c14ff64b440c8d9515eaee38370fd8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5392283 Commit-Queue: Yuzu Saijo <yuzus@chromium.org> Reviewed-by: Rakina Zata Amni <rakina@chromium.org> Cr-Commit-Position: refs/heads/main@{#1280059} -- wpt-commits: e99b8186f7272559b3d419445faef703958bfdd1 wpt-pr: 45408
…ec, a=testonly Automatic update from web-platform-tests [bfcache] Rename reasons to match the spec This CL renames the reporting strings of NotRestoredReasons API so that they match the spec draft[1]. [1]: whatwg/html#10154 Bug: 331754704 Change-Id: If805ad7e30c14ff64b440c8d9515eaee38370fd8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5392283 Commit-Queue: Yuzu Saijo <yuzus@chromium.org> Reviewed-by: Rakina Zata Amni <rakina@chromium.org> Cr-Commit-Position: refs/heads/main@{#1280059} -- wpt-commits: e99b8186f7272559b3d419445faef703958bfdd1 wpt-pr: 45408
|
@smaug---- Can you PTAL? Thanks |
|
@smaug---- @petervanderbeken Another ping. |
|
We'll go through this later today. Sorry about delay. |
|
We're ok to expose those reasons which the web site can affect to (and which are based on some specification), but nothing happening possibly in cross-origin iframes (like "frame-navigating") or in the UA itself (like "cookie-disabled" or "extension-messaging") should be exposed. |
source
Outdated
| <span data-x="ua-specific-blocking-reasons">user-agent specific blocking reasons</span>. | ||
| </dl> | ||
|
|
||
| <p>In addition to the list above, a user agent might prevent the page from being restored from |
There was a problem hiding this comment.
I'm not entirely sure how to phrase this, but I think this should be a little clearer that "a user agent might choose to expose a reason that prevented …".
For the reasons where there might be less consensus, it would be good to allow UAs not to expose certain reasons even if they correspond to something in this second list. I know masked allows it, but this sentence seems to imply that if a UA blocks for one of these then it will expose that reason.
For "frame-navigating", let me make it only for same-origin subframes that have an ongoing navigation. For "cookie-disabled", you can already tell by doing something like calling |
resolve mc c so far resolve mc resolve mc
Allow top layer elements to be nested within popovers This allows top layer elements, including the dialog element, to be nested inside of an open popover, by not closing the popover when the new top layer element is opened. Without this patch, opening a modal dialog inside of a popover will make the page inert and make the dialog invisible. Fixes whatwg#9998. See also whatwg/fullscreen#237. Editorial: order of comparisons For consistency: - greater than or equal to - less than or equal to Improve element reflection This attempts to make the following improvements: 1. Make it more clear that initializing attr is not the first step in an algorithm, but rather something that counts for all the list items. 2. Rewrite the associated element(s) fields as algorithms. As there are no downstream references so far this is a change we can still make. 3. Add another layer of caching that is separate from the FrozenArray to avoid having to compare a list of elements with a FrozenArray directly. This helps with whatwg#10219. Disable PageSwapEvent's activation on cross-origin redirects Closes whatwg#10196. Upstream Long Animation Frames monkey-patches Long Animation Frames (https://w3c.github.io/long-animation-frames/) expects a few calls from HTML and other specs, for reporting when tasks, rendering or JS entry points take place. This moves those calls from the Long Animation Frames spec to HTML. Preload: only allow certain values for as="" Closes whatwg#8332. Call the view transition page visibility change steps This allows the CSS view-transitions spec to react to page visibility changes. Specifically, skip the active transition once a page is hidden. See w3c/csswg-drafts#9543. Style marquee using overflow: hidden This matches Chromium and WebKit. Tests will be worked on in https://bugzilla.mozilla.org/show_bug.cgi?id=306344. Editorial: export Element's innerText getter and setter steps These will be used by Trusted Types (and eventually HTML once upstreamed) as part of shadowing this attribute to HTMLScriptElement. Add getHTML() and serializable shadow roots Corresponding DOM PR: whatwg/dom#1256. Closes whatwg#8867. Co-authored-by: Domenic Denicola <d@domenic.me> Make buttons respect display: none/contents in button layout Fixes whatwg#10238. This matches what is already implemented in browsers. Remove duplicate requirement for 'overflow' for marquee The duplication was introduced by whatwg#10243. Meta: make all the SVGs darkmode-aware Also tag them as such, so that they don't get a white background after whatwg/whatwg.org#439 is merged. Warn that the XML syntax is not recommended Closes whatwg#10237.
Isn't that already the case? We always mask things that happen in a cross-origin subframe. |
You're right. "frame-navigating" and all the other reasons are only exposed when happening in same-origin frames. |
Website can't affect to that, so it doesn't need to know about cookies affecting browser's internal decision whether to bfcache or not. |
| <dt>"<dfn data-x="blocking-extension-messaging" export><code>extension-messaging</code></dfn>"</dt> | ||
| <dd>Extensions were using <code data-x="">sendMessage()</code> function before unloading, or | ||
| extensions called <code data-x="">sendMessage()</code> to this <code>Document</code> while it | ||
| was in <a href="#note-bfcache">back/forward cache</a>.</dd> |
There was a problem hiding this comment.
Extensions aren't part of the web platform, so they very much should not be exposed.
This is a follow-up PR of #9360, where we added NotRestoredReasons.
This PR adds a list of NotRestoredReasons that user agents may choose to block with.
/document-lifecycle.html ( diff )
/infrastructure.html ( diff )
/nav-history-apis.html ( diff )