search

WICG / portals

A proposal for enabling seamless navigations between sites or pages
https://wicg.github.io/portals/
Other
946 stars 66 forks source link

Delay activation on initial loading; src="" changes reset portal BC #253

Closed domenic closed 4 years ago

domenic commented 4 years ago

Closes #197 by causing all changes to src="" to close the portal browsing context, and re-create it.

Closes #227 by causing activate() to delay promise resolution/rejection until the initial load in a new browsing context has finished.

Old content @jakearchibald, @kjmcnee, please take a look. Other reviewers also appreciated. Two main things to consider: - Is this the behavior we want? I know @jakearchibald was skeptical about the "After the initial load" behavior, but I think it's better, and I believe @kjmcnee indicated that it's implementable. The alternative is to delay activation like we do with initial load, but that seems like a worse user experience. - Is this explainer structure reasonable? I'm wondering if, e.g., we should move all discussion of activation failure from the bfcache section into here, or merge this with the bfcache section somehow. Also the flow feels a bit weird, in that we have to spend so much time up front explaining closed portals. But I couldn't find a better place to put it.

Preview | Diff

jeremyroman commented 4 years ago

left a comment in response to Jake's on line 338; GitHub doesn't seem to want to notify?

domenic commented 4 years ago

Alright, I've kind of morphed this into a bigger change. Now it includes spec changes, and tackles #197 at the same time. I'd like @jeremyroman's review in particular for spec mechanics. If @kjmcnee has time his re-review would also be appreciated, but I think conceptually there's not too much delta from when he last looked at it; we just incorporated the src=""-changes-cause-reset which had previously been discussed.

jeremyroman commented 4 years ago

LGTM. This makes sense to me (obviously we still have to make corresponding impl changes).

kjmcnee commented 4 years ago

Alright, I've kind of morphed this into a bigger change. Now it includes spec changes, and tackles #197 at the same time. I'd like @jeremyroman's review in particular for spec mechanics. If @kjmcnee has time his re-review would also be appreciated, but I think conceptually there's not too much delta from when he last looked at it; we just incorporated the src=""-changes-cause-reset which had previously been discussed.

Still LGTM. Sorry for the delay.