Skip to content
This repository has been archived by the owner on Jul 18, 2022. It is now read-only.

HTML spec should call visibilitychange algorithms directly. #51

Closed
Tracked by #38
marcoscaceres opened this issue Jun 27, 2019 · 22 comments
Closed
Tracked by #38

HTML spec should call visibilitychange algorithms directly. #51

marcoscaceres opened this issue Jun 27, 2019 · 22 comments
Assignees
Milestone

Comments

@marcoscaceres
Copy link
Member

The spec currently depends on:

  • session history entry
  • unloading document visibility change steps

But for whatever reason those are not being exported from the HTML spec. The second one should definitely be exported, but folks here should check with the HTML editors if there is some reason they didn't export "session history entry".

@yoavweiss
Copy link
Contributor

Isn't this an issue on the HTML spec to expose those things?

@marcoscaceres
Copy link
Member Author

It could be :) I was just too lazy to file them/send-PRs-to HTML for them.

@toddreifsteck toddreifsteck self-assigned this Aug 1, 2019
@toddreifsteck
Copy link
Member

@toddreifsteck please ensure that HTML spec issues are opened for these references.

@marcoscaceres
Copy link
Member Author

In relation to #55, I filed whatwg/html#4892 (comment).

@domenic responded with:

I'm not sure what to do here, as the usages of these terms are by a spec which pretty clearly monkey-patches HTML in bad ways. For example, the usage of "session history entry" is in an algorithm that reads "When the user agent determines that the visibility of the Document of the top-level browsing context has changed ... If traversing to a session history entry ...". The correct thing to do here is to actually update HTML's session history traversal algorithm, so that implementers working on that algorithm can see how it works.

That is to say, if the specifications were properly layered, "session history entry" would not be a concept that is referred to by specs outside of HTML. So I don't think we should export it, and encourage further abuse of this sort.

The other two "change steps" though are definitely meant to be exported, so that portion of the patch makes sense. Although I note that "session history document visibility change steps" isn't actually used by the Page Visibility spec, and the way in which it uses "unloading document visibility change steps" is incorrect. (It doesn't define the steps, it just references them during its monkey-patch algorithm.)

@toddreifsteck
Copy link
Member

I've read over this and have a good grip on how to make these updates.. but I need another half day of free time and my day job has taken me elsewhere. I'd like to complete this and expect to do so this Saturday 10/12 in my off hours. If I'm unable, I'll need to hand this off. I'll share update here.

@toddreifsteck toddreifsteck removed their assignment Oct 18, 2019
@toddreifsteck
Copy link
Member

Unfortunately, I haven't had time to take this on. Removing my name to allow another good soul to complete this.

@mmocny
Copy link

mmocny commented Jul 29, 2020

Caught up on the history here, and seems to me like a more substantial update to the way this spec is defined. Should this be L2 blocking, or L3? This wording was already present in L1, which is already a recommendation.

@mmocny
Copy link

mmocny commented Jul 30, 2020

From discussion at WebPerf WG, this need not block L2 since it is spec-health related and not affecting functionality/testing/implementation, plus the same wording already existings in L1 which is already published.

@yoavweiss Is that correct? (Also: I am not sure if I can update milestone tag on this issue.)

@domenic
Copy link

domenic commented Jul 30, 2020

That's frustrating. In general, foundational spec health issues should be addressed as soon as possible, and not pushed off because they've been unhealthy for a long time.

@marcoscaceres
Copy link
Member Author

I agree with @domenic. Incorrect layering to HTML will more than likely lead to implementation issues. Having said that, republishing a CR seems appropriate as we address these issues - but we should make a small push to address them before we start talking about any L3 stuff.

@yoavweiss
Copy link
Contributor

The intent is not to push this off, just not have it blocking the re-publishing of L2.
The plan is to branch L2, continue L3 and this work on ToT and then potentially backport this change to L2.
Depending on the magnitude of the backporting work, we can then determine if it's worthwhile to have backporting block moving L2 to PR and beyond.

@yoavweiss
Copy link
Contributor

OK, so IIUC, what we want to do here is:

@domenic & @mmocny - does the above make sense to you?

@domenic
Copy link

domenic commented Aug 3, 2020

  • In HTML's traverse the history, before step 4.6, we want to call determine the visibility state, and if the result is "visible", run the now visible algorithm. (or encapsulate all that in a single algorithm to be run from HTML)

  • Export the "change steps" from @marcoscaceres' PR

I think you probably want to run it between 4.6.2 and 4.6.4? I.e., you want to run it at precisely the place that HTML currently calls out to "session history document visibility change steps".

In particular you don't want to fire "visibilitychange" if the document is moving from page showing = true to page showing = true. Also, you want to run it as part of the same task, probably.

So I think you want to define "session history document visibility change steps", given document, as something like:

  1. Fire an event named visibilitychange with its bubbles attribute initialized to true at document.

  2. Run the external now visible algorithm given document, if one is defined by another specification.

(although as a separate issue it might be good to remove the latter...)

Similarly you would want to define the "unloading document visibility change steps" given document as something like:

  1. Fire an event named visibilitychange with its bubbles attribute initialized to true at document.

  2. Run the external now hidden algorithm given document, if one is defined by another specification.

Once you have those two in place, HTML could link to them directly, instead of using our current inverted layering where HTML defines the term and Page Visibility links to it.

  • Make the algorithm defined in reacting to visibility changes exclude navigations, as I believe the only case today where we want the visibility change event to fire on navigations is for history travesals. @philipwalton - can you confirm?

Agreed. And at that point you can simplify it, by removing steps 2.1 and 3.1 (as well as part of the predicate of step 3).

@mmocny
Copy link

mmocny commented Aug 4, 2020

Thanks folks for the help and detailed concrete suggestions. I'll try to take a stab at writing this up. (Heads up, I'm partially OOO this week, so ETA is next week)

@yoavweiss
Copy link
Contributor

Thanks!! :)

@mmocny
Copy link

mmocny commented Sep 29, 2020

Thanks @domenic for the concrete steps. After spending some time diving into this, and discussing with folks during Web Perf Hackathon, here's the status:

  1. HTML already has hooks for session history document visibility change steps and unloading document visibility change steps in the right place, but Page Visibility is oddly monkey patching instead of just correctly defining algorithms for these.

  2. But, Page Visibility already defines now visible algorithm and now hidden algorithm which basically have the same definition we want for (1), and we should consider just merging them. This introduces naming issues, but we already want to update HTML to call this algorithm directly, anyway.

  3. Finally, both the now visible algorithm and now hidden algorithm are almost exactly the same steps, because visibilitychange event itself is "stateless". The only difference are the exposed hooks external now visible algorithm and external now hidden algorithm. However...

    1. These hooks were exposed for screen-orientation, but that spec hasn't actually updated to use the hooks yet.

    2. Screen-orientation already uses If doc is not visible to abort early, so a hook for "any visibility change" vs specifically "now visible" would probably suffice.

This leaves a few questions/options:

  1. Is it better to expose catch-all "hooks" (like in HTML and Page Visibility currently), for other specs to reference, or, is it better to add explicit calls out to each spec?

    • Note that HTML already has a [NOTE] This is specifically intended for use by Page Visibility. [PAGEVIS], and
    • @domenic also already suggested "Once you have those two in place, HTML could link to them directly" and "it might be good to remove the latter...", and
    • Web Perf WG folks also agreed it would be better to just replace all these hooks with explicit calls.
  2. If we do remove hooks, can we define only one single algorithm (for any type of visibility change), or, should we still create distinct named algorithms for shown/hidden?

I'm taking a stab are replacing all with explicit calls, and using a single algorithm. (I could re-add distinct shown/hidden algorithms in terms of the common one if useful)

@domenic
Copy link

domenic commented Sep 29, 2020

Is it better to expose catch-all "hooks" (like in HTML and Page Visibility currently), for other specs to reference, or, is it better to add explicit calls out to each spec?

My preference is to have explicit calls. My guess is that these hooks were added in the early days of web standardization, when we had some dream that things might be layered, with HTML not explicitly referencing other specs. Such a dream, if it ever existed, is long gone.

If we do remove hooks, can we define only one single algorithm (for any type of visibility change), or, should we still create distinct named algorithms for shown/hidden?

A single algorithm sounds good to me!

@npm1
Copy link
Contributor

npm1 commented Sep 29, 2020

Just to make sure my understanding is right, our plan is to have this:

  • PageVisibility has an algorithm that we say runs whenever user agent determines that document's visibility state has changed (hand-wavy, and has caused issues like The spec's definition of hidden is unclear and should be updated to better account for mobile use cases #59, but perhaps out of scope for this particular issue).
  • HTML invokes the above algorithm explicitly in a couple of places.
  • The algorithm defined in PageVisibility invokes some screen orientation algorithm (maybe only when visible per the usage in that spec). For this we need them to dfn something. The monkey patch from ScreenOrientation is not finished though, because what they proceed to do is say that some steps must run in the next 'animation frame task'... Sounds like something that should instead but added to the event loop processing model?

So the ScreenOrientation algorithm invoked from PageVisibility could be as simple as setting a boolean flag, and then in HTML in the event loop the HTML would invoke another algorithm from ScreenOrientation which would perform the steps if the flag is set, and would always unset the flag.

@mmocny
Copy link

mmocny commented Sep 30, 2020

I didn't think the ScreenOrientation change would need to be any more complicated than PageVisibility just calling an algorithm (which they would need to expose first). But I see, you are pointing out that ScreenOrientation has a second "monkey patch", into HTML, and that they'll need to expose a call there, too? Thanks for investigating that!

The first change here, would not add anything for ScreenOrientation at all, I think. It will just do your first 2 bullets, and undo the current hooks that were exposed in #47.

(I will re-open the issue(s) for improving ScreenOrientation integration afterwards, suggesting the steps you listed above)

@noamr
Copy link

noamr commented Sep 23, 2021

Can we maybe change the title for this issue? I'm tracking a big list of issues and it's hard to understand from the title what it's about.

@yoavweiss yoavweiss changed the title Unexpected HTML things to check HTML integration needs improvement Sep 23, 2021
@mmocny mmocny changed the title HTML integration needs improvement HTML spec should call visibilitychange algorithms directly. Sep 23, 2021
@mmocny
Copy link

mmocny commented Sep 23, 2021

Woops I missed that @yoavweiss already updated the title (darn github has such subtle changelog for these things).

noamr added a commit that referenced this issue Oct 5, 2021
noamr added a commit to noamr/html that referenced this issue Oct 5, 2021
Fire the event when unloading or traversing history,
instead of relying on hooks in other specs.

See w3c/page-visibility#51
and w3c/page-visibility#73
noamr added a commit to noamr/html that referenced this issue Oct 15, 2021
Fire the event when unloading or traversing history,
instead of relying on hooks in other specs.

See w3c/page-visibility#51
and w3c/page-visibility#73
domenic pushed a commit to whatwg/html that referenced this issue Oct 15, 2021
Fire the event when unloading or traversing history, instead of relying on hooks in other specs.

See w3c/page-visibility#51 and w3c/page-visibility#73.
@noamr
Copy link

noamr commented Nov 3, 2021

Done

@noamr noamr closed this as completed Nov 3, 2021
dandclark pushed a commit to dandclark/html that referenced this issue Dec 4, 2021
Fire the event when unloading or traversing history, instead of relying on hooks in other specs.

See w3c/page-visibility#51 and w3c/page-visibility#73.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Jun 3, 2022
Fire the event when unloading or traversing history, instead of relying on hooks in other specs.

See w3c/page-visibility#51 and w3c/page-visibility#73.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
7 participants