Skip to content
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

RFC6265bis: Use navigables concept #2480

Merged
merged 5 commits into from
May 3, 2023

Conversation

sbingler
Copy link
Collaborator

Closes #2372

The HTML spec recommends avoiding using "browser contexts" and instead recommends "navigables".

This PR refactors rfc6265bis to use "navigables".

@sbingler
Copy link
Collaborator Author

@johannhof PTAL

@jakearchibald If you have any time. These concepts look like they should be drop in replacements but I'd appreciate your feedback just in case.

Copy link

@jakearchibald jakearchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of nits, but LGTM

draft-ietf-httpbis-rfc6265bis.md Outdated Show resolved Hide resolved
Comment on lines 1095 to 1103
1. Let `top-document` be the active document in `document`'s navigable's
top-level traversable.

2. Let `top-origin` be the origin of `top-document`'s URI if `top-document`'s
sandboxed origin browsing context flag is set, and `top-document`'s origin
otherwise.

3. Let `documents` be a list containing `document` and each of `document`'s
ancestor browsing contexts' active documents.
ancestor navigables' active documents.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, I'd reword this to avoid assuming that a document is its navigable's active document. Or, if you're sure that this algorithm only runs when the chain of documents are active, add a line that asserts that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure we want this only for an active document chain, and I've added a note at the bottom of the algorithm saying as much.

@mikewest does that seem correct to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are prerendered documents "active"? If not, we need to make sure we don't make it impossible to define their "site for cookies" by excluding non-active documents.

That said, I'm not sure we need to care about activity. The point of this algorithm was to walk up the ancestor chain from the document in which a given request was initiated and to do some origin comparisons. I think it should run on the set of ancestor documents, active or not. I'm assuming there's still a single path up the ancestor chain, and it's not clear to me that any ancestor can be inactive without the document we're talking about also being inactive. In that case, 🤷 .

That said, this definition should probably move to HTML at some point, and the RFC should depend upon the embedder's definition of terms. :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are prerendered documents "active"?

Yes. I believe the goal is to spec them as a nested traversable, so it'll be the active entry of that traversable. I think @domenic has been thinking more about the spec here.

I think it should run on the set of ancestor documents, active or not. I'm assuming there's still a single path up the ancestor chain

Yes, you can get a document's container document https://html.spec.whatwg.org/multipage/document-sequences.html#doc-container-document

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to use https://html.spec.whatwg.org/#inclusive-ancestor-navigables . See e.g. this call site for something that does exactly what you're doing: https://html.spec.whatwg.org/#the-autofocus-attribute:inclusive-ancestor-navigables

You kind of need to care about activeness because you need a non-ambiguous way of going from navigables (which are in a tree) to documents (which are not). Each navigable has a single active document and potentially many inactive documents; the latter are stored in bfcache. You want to pick out the active document.

E.g. given:

parent navigable: A --> [B] --> C
                         |
child navigable:      [child]

where [] denotes the currently-active document, you want the set to contain B, but not A or C.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the feedback, I used the inclusive ancestor navigables concept. I also kept the note that all docs should be active, @mikewest do you have a preference to remove/keep it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy where this ended up. Will give it one more read-through before landing it.

@johannhof
Copy link
Collaborator

Thanks @sbingler, this looks conceptually right to me but I'll defer to @jakearchibald for detailed inspection :)

@sbingler
Copy link
Collaborator Author

@mikewest ping

Copy link
Member

@mikewest mikewest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. LGTM.

Thanks!

@mikewest
Copy link
Member

mikewest commented May 2, 2023

If you resolve the conflict with the previous patch, I'll land it.

If you don't have time, I'll do it (since I made you wait so long!).

@sbingler
Copy link
Collaborator Author

sbingler commented May 2, 2023

Thanks Mike, conflicts should be resolved

@mikewest mikewest merged commit b069fab into httpwg:main May 3, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Fix browsing context integration in 6265bis
5 participants