Tie BC creation to availability of a valid src + BC connectedness. #138

jeremyroman · 2019-05-29T19:32:43Z

Resolves #137.

PTAL and let me know if you agree that this is the right behavior.

jeremyroman · 2019-05-29T19:54:50Z

Hmm, on further thought, what should happen if you set src on a portal that has a guest BC but is not yet inserted? I think it should navigate, but it's kinda weird regardless.

onportalactivate = e => {
  let portal = e.adoptPredecessor();
  portal.src = 'https://somewhere.else/';
  /* ... */
};

lucasgadani

Yeah, I agree that the portal element with a portal browsing context, but not browsing context connected is a bit weird, but I also think it should navigate. Other than that LGTM.

jeremyroman · 2019-05-30T20:24:12Z

Doing some more edge-case thinking here. What about this:

onportalactivate = e => {
  let portal = e.adoptPredecessor();

  // Does this drop the existing BC?
  portal.src = '';

  // If so, does this recreate it, or did the above forfeit the special status?
  portal.src = 'https://somewhere.else/';
}

I'm leaning toward making the concept of specially-extended lifetime for the result of adoptPredecessor more explicit.

I've been avoiding it because it's kind of indirect, but maybe it's semantically nicer. Something like:

A <portal> element has lifetime reasons, which is an ordered set of strings that is initially « ».

To extend element's lifetime due to reason, append reason to element's lifetime reasons.
To discontinue element's lifetime due to reason, remove reason from element's lifetime reasons. If this causes element's lifetime reasons to become empty, then discard element's guest browsing context and set it to null.

Whenever a portal element element becomes browsing-context connected, run the following steps:

Extend element's lifetime due to "browsing-context-connected".

(Set the source URL if appropriate, etc.)

Whenever a portal element element becomes browsing-context disconnected, run the following steps:

Discontinue element's lifetime due to "browsing-context-connected".

Plus wording to give an additional reason, say "predecessor-just-adopted", in adoptPredecessor, which is discontinued at the end of the steps to activate a portal browsing context.

This would imply that for the duration of the portalactivate event, you can treat the portal element the same as if it were BC-connected (you can set its source to invalid and invalid values, post messages, and so on). I think that's probably nicer than having the two cases be different, even if it seems to demand a little abstraction overhead.

WDYT (before I go to the effort of making such changes)?

@domenic might also want to comment since I'm kinda proposing to make this more complicated in the name of Internal Consistency™.

lucasgadani · 2019-06-04T19:06:04Z

Doing some more edge-case thinking here. What about this:

onportalactivate = e => {
  let portal = e.adoptPredecessor();

  // Does this drop the existing BC?
  portal.src = '';

I'd argue that yes, this does drop the BC.

  // If so, does this recreate it, or did the above forfeit the special status?
  portal.src = 'https://somewhere.else/';
}

No, because at this point the portal isn't BC-connected. The special status is only achievable by calling the adoptPredecessor API, once it's dropped, it's no possible to get it back.

I'm leaning toward making the concept of specially-extended lifetime for the result of adoptPredecessor more explicit.

I've been avoiding it because it's kind of indirect, but maybe it's semantically nicer. Something like:

A element has lifetime reasons, which is an ordered set of strings that is initially « ».
To extend element's lifetime due to reason, append reason to element's lifetime reasons.
To discontinue element's lifetime due to reason, remove reason from element's lifetime reasons. If this causes element's lifetime reasons to become empty, then discard element's guest browsing context and set it to null.
Whenever a portal element element becomes browsing-context connected, run the following steps:

Extend element's lifetime due to "browsing-context-connected".

(Set the source URL if appropriate, etc.)

Whenever a portal element element becomes browsing-context disconnected, run the following steps:

Discontinue element's lifetime due to "browsing-context-connected".

Plus wording to give an additional reason, say "predecessor-just-adopted", in adoptPredecessor, which is discontinued at the end of the steps to activate a portal browsing context.

This would imply that for the duration of the portalactivate event, you can treat the portal element the same as if it were BC-connected (you can set its source to invalid and invalid values, post messages, and so on). I think that's probably nicer than having the two cases be different, even if it seems to demand a little abstraction overhead.

WDYT (before I go to the effort of making such changes)?

Overall, I think this edge case is so unlikely that it is not worth to overcomplicate the spec to accommodate it in a nicer way. I think it's reasonable to just say that the only way for the document to create the portal browsing context is by appending it to the dom and setting the source.

@domenic might also want to comment since I'm kinda proposing to make this more complicated in the name of Internal Consistency™.

I'd also like to see @domenic's opinion here, but I'd prefer to keep it simple unless there's an actual use-case that would be solved by this. In fact, even if we left it unspecified, it probably still wouldn't matter, since if implementations disagree on what to do when setting the src immediately after adopting wouldn't be an issue.

jeremyroman · 2019-06-04T19:38:17Z

I would definitely like to not leave it unspecified; I think there's no reason not to want implementations to be compatible about what these operations do and what their preconditions are.

The special status is only achievable by calling the adoptPredecessor API, once it's dropped, it's no possible to get it back.

I'm not claiming it can be recovered, I'm wondering whether setting src forfeited it early. The special status is ordinarily forfeited at the end of the portalactivate event.

My aim is simplicity. A more concise way of describing what I wrote above is: a portal may have a BC while it is browsing-context connected or it is the result of adoptPredecessor during portalactivate. While that is true, it is functional; when it becomes false, it dies.

I think that's simpler than needing to specify the behavior of all of the operations (set src, activate, post message) while in the adopted-but-not-connected state as distinct from both the connected state and the dead state.

jeremyroman · 2019-06-04T20:00:08Z

For comparison: the alternative model (closer to what's currently written) is to predicate operations on the existence of an existing associated browsing context, and if the special state of having-a-guest-but-not-being-connected is lost then the portal element becomes inert until it becomes connected.

domenic

Very interesting.

I see the draw of the lifetime-reasons framework outlined in #138 (comment). In general, I do think we should have a clear statement of when a portal has a BC. We should state it somewhere as an invariant the specification upholds, and then work out spec text that makes it obvious that the invariant does hold.

I have no opinion on what that invariant should be, but I think the first step should be spelling it out precisely. I think it's not just

is browsing-context connected or it is the result of adoptPredecessor during portalactivate

because there's also some constraint on the validity of the src="" attribute, right?

But if we can state it precisely, then we could have a framework like:

"A portal should have a browsing context if all of the following are true: ...".
- Defining this may require the lifetime-reasons framework, or at least, some sort of boolean storing whether you're going through a portalactivate.
An "Update a portal's BC" algorithm which checks "should have a BC", and synchronizes the portal with that (either by closing or creating). Plus I guess here is where you would also navigate.
Everything that could change "should have a BC" then calls "update a portal's BC".

This is just off the top of my head, so I'm not sure it's perfect, but that's the direction I'd be thinking here.

domenic · 2019-06-10T18:21:49Z

index.bs

+          The user agent may choose to emit a warning if the author attempts to
+          use a <{portal}> element in a nested browsing context, as this is not
+          supported.
+        </p>

    1. If |element| has no <{portal/src}> attribute specified, or its value is the empty string,
        then [=close a portal element|close=] |element| and abort these steps.


Drive-by nit: "return", not "abort these steps", for both.

In general for all uses of the phrase "abort these steps", or is there some guidance for when each wording is preferred? If it's just an editorial change in general, maybe I should do a separate PR to globally replace the phrase.

Yeah, probably best separate. The guidance is not very clear. We use "return" to return from an algorithm, but if that algorithm has sub-steps, e.g. in parallel or a queued task, then we have stuck with "abort these steps" (i.e. "return" is reserved for "exit the whole algorithm").

In particular, https://infra.spec.whatwg.org/#algorithm-control-flow talks about using return, but it doesn't capture the remaining uses of "abort these steps" that we've evolved. I'll work on clarifying that in Infra.

domenic · 2019-06-10T18:23:03Z

index.bs

@@ -407,6 +417,24 @@ spec:url; type:dfn; text:scheme

    1. If the [=scheme=] of |url| is not an [=HTTP(S) scheme=], then abort these steps.

+        <div class="issue">
+          Would it be more consistent to treat this like an invalid URL, and close?
+        </div>


Making the failure case (including for being in a nested BC) uniformly close seems reasonable to me. Right now there are some cases where the algorithm returns early but doesn't close, which seems like an unnecessary complexity for the author's mental state machine.

domenic · 2019-06-10T18:24:49Z

index.bs

@@ -394,9 +394,19 @@ spec:url; type:dfn; text:scheme
  <section algorithm="htmlportalelement-setsourceurl">
    To <dfn for="HTMLPortalElement">set the source URL of a <{portal}> element</dfn> |element|, run the following steps:


"set the source URL" is feeling like a worse name for this algorithm now.

I'm not sure what a better name is. It's principally used when the embedder directs the portal BC to navigate by changing the src attribute. It is also called on BC connection, mainly because we didn't navigate earlier when src was set.

"Possibly navigate" sounds a little vague. Any suggestions?

jeremyroman · 2019-06-11T20:36:38Z

I'm not sure "has a BC" can be expressed without reference to state created just for that purpose, because if you invoke activate, nothing about its state changes except that it loses its BC. There's a similar case of being adopted by another document, which needs to discard the BC. Forcibly changing content attributes in such a case seems strange.

A weaker "may have a BC" constraint can more easily be described (roughly, BC-connected or in portalactivate dispatch); it could tell you that a <portal> definitely has no BC, but not stronger than that.

The lifetime-reasons thing could also be spelled as an algorithm that explicitly checks all of the possible reasons. The portalactivate reason would require an explicit boolean of state at least, so it didn't seem shorter. It'd look something like:

To check the lifetime of a <portal> element element, perform the following steps:

If element is browsing-context connected, then return.

If element's just-adopted flag is true, then return.

Discard element's guest browsing context.

Set element's guest browsing context to null.

This would be invoked at all points which could make one of the conditions false (BC disconnection, end of portalactivate dispatch).

…pt to be consistent.

jeremyroman · 2019-06-12T21:03:50Z

Hmm, tried to incorporate some of this feedback into the latest version of this PR. PTAL?

domenic · 2019-06-14T18:42:44Z

index.bs

+
+            1. Set |adoptedPredecessorElement|'s [=just-adopted flag=] to false.
+
+            1. If |element| [=may have a guest browsing context|may not have a guest browsing context=] and its [=guest browsing context=] is not null, then [=discard a browsing context|discard=] it.


The use of discard here vs. close below warrants a note, I think.

Done; wdyt?

index.bs

domenic · 2019-06-14T18:47:18Z

index.bs

@@ -701,14 +739,17 @@ spec:url; type:dfn; text:scheme

  A {{PortalActivateEvent}} has an associated <dfn for="PortalActivateEvent">predecessor browsing context</dfn>,
  which is a [=top-level browsing context=] or null, a <dfn for="PortalActivateEvent">successor window</dfn>, which is
-  a {{Window}}, and an <dfn for="PortalActivateEvent">activation promise</dfn>, which is a [=promise=].
+  a {{Window}}, an <dfn for="PortalActivateEvent">activation promise</dfn>, which is a [=promise=], and a
+  <dfn for="PortalActivateEvent">adopted predecessor element</dfn>, which is a <{portal}> element or null.


The fact that this is an element doesn't appear to be used. A boolean would work just as well. Right?

It's used to clear the "just adopted" flag after portalactivate dispatch.

You're right, my bad.

lucasgadani · 2019-06-14T21:51:27Z

index.bs

+  <section algorithm="htmlportalelement-may-have-guest-browsing-context">
+    To determine whether a <{portal}> element <dfn for="HTMLPortalElement">may have a guest browsing context</dfn>, run the following steps:
+
+    1. If |element| is [=browsing-context connected=], then return true.


Maybe also add "and its browsing context is top-level"?

Maybe this will complicate the algorithm below, but it might be worth changing, as it expresses more clearly the situations where a portal may have a browsing context.

Done; PTAL?

…lucasgadani

…lucasgadani.

Tie BC creation to availability of a valid src + BC connectedness.

ac51051

jeremyroman requested a review from lucasgadani May 29, 2019 19:34

lucasgadani reviewed May 29, 2019

View reviewed changes

more stuff

57d995f

domenic reviewed Jun 10, 2019

View reviewed changes

Merge branch 'master' into portals-lazy-bc

394043b

jeremyroman added 4 commits June 11, 2019 16:45

Note when an element can host a BC

87bb25d

start to refactor lifetime

1b2087d

a little more refactoring, including removing redundant two-way setting

772f647

Restructure in terms of "may have a guest browsing context" and attem…

c67ecaa

…pt to be consistent.

domenic mentioned this pull request Jun 14, 2019

Add and explain usage of "abort these steps" whatwg/infra#255

Closed

domenic reviewed Jun 14, 2019

View reviewed changes

Address some comments from @domenic.

b2c5fcd

domenic approved these changes Jun 14, 2019

View reviewed changes

lucasgadani reviewed Jun 14, 2019

View reviewed changes

Move nested BC check into the general 'may have a guest BC' check per @…

315b8a0

…lucasgadani.

lucasgadani approved these changes Jun 20, 2019

View reviewed changes

handle null a little better here

cc1abcb

jeremyroman merged commit de86b69 into WICG:master Jun 20, 2019

jeremyroman deleted the portals-lazy-bc branch June 20, 2019 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tie BC creation to availability of a valid src + BC connectedness. #138

Tie BC creation to availability of a valid src + BC connectedness. #138

jeremyroman commented May 29, 2019 •

edited by pr-preview bot

Loading

jeremyroman commented May 29, 2019

lucasgadani left a comment

jeremyroman commented May 30, 2019

lucasgadani commented Jun 4, 2019

jeremyroman commented Jun 4, 2019

jeremyroman commented Jun 4, 2019

domenic left a comment

domenic Jun 10, 2019

jeremyroman Jun 11, 2019

domenic Jun 11, 2019

domenic Jun 11, 2019

domenic Jun 10, 2019

jeremyroman Jun 12, 2019

domenic Jun 10, 2019

jeremyroman Jun 12, 2019

jeremyroman commented Jun 11, 2019

jeremyroman commented Jun 12, 2019

domenic Jun 14, 2019

jeremyroman Jun 14, 2019

domenic Jun 14, 2019

jeremyroman Jun 14, 2019

domenic Jun 14, 2019

lucasgadani Jun 14, 2019 •

edited

Loading

jeremyroman Jun 20, 2019

		@@ -394,9 +394,19 @@ spec:url; type:dfn; text:scheme
		<section algorithm="htmlportalelement-setsourceurl">
		To <dfn for="HTMLPortalElement">set the source URL of a <{portal}> element</dfn> \|element\|, run the following steps:


		1. Set \|adoptedPredecessorElement\|'s [=just-adopted flag=] to false.

		1. If \|element\| [=may have a guest browsing context\|may not have a guest browsing context=] and its [=guest browsing context=] is not null, then [=discard a browsing context\|discard=] it.

Tie BC creation to availability of a valid src + BC connectedness. #138

Tie BC creation to availability of a valid src + BC connectedness. #138

Conversation

jeremyroman commented May 29, 2019 • edited by pr-preview bot Loading

jeremyroman commented May 29, 2019

lucasgadani left a comment

Choose a reason for hiding this comment

jeremyroman commented May 30, 2019

lucasgadani commented Jun 4, 2019

jeremyroman commented Jun 4, 2019

jeremyroman commented Jun 4, 2019

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyroman commented Jun 11, 2019

jeremyroman commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasgadani Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyroman commented May 29, 2019 •

edited by pr-preview bot

Loading

lucasgadani Jun 14, 2019 •

edited

Loading