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

Remove event handling, as it's moving to the HTML spec. #73

Closed
wants to merge 4 commits into from

Conversation

noamr
Copy link

@noamr noamr commented Oct 5, 2021

Closes #51


Preview | Diff

noamr added a commit to noamr/html that referenced this pull request 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
Copy link
Author

noamr commented Oct 5, 2021

Should be merged in conjunction with whatwg/html#7153

index.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member

(Will take a proper look tomorrow… reviewing on my phone ☎️)

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Member

So... hmm... this is kinda the bulk of the spec... I wonder if we should just move the whole thing over to HTML? I guess we can do that incrementally.

@noamr, before merging, it might be good for us to figure out if we are going to break any specs there were calling into these steps. We probably are not, but we should give those specs a heads up.

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Tentative approval. Will keep an eye on the HTML side too.

@noamr
Copy link
Author

noamr commented Oct 6, 2021

So... hmm... this is kinda the bulk of the spec... I wonder if we should just move the whole thing over to HTML? I guess we can do that incrementally.

Yea that happened with a few of those specs, where they started disappearing and ended up mainly defining the interface.

@noamr, before merging, it might be good for us to figure out if we are going to break any specs there were calling into these steps. We probably are not, but we should give those specs a heads up.

Not sure how to do that.

@marcoscaceres
Copy link
Member

Not sure how to do that.

For example:
https://github.com/search?q=org%3Aw3c+%22now+visible+algorithm%22&type=code

(The file hint at the specs relying on the exported definition)

Doing the above is optional... we can just "break" the references and then just email the various WGs to let them know we've moved stuff... we can deal with that before we merge tho.

@noamr
Copy link
Author

noamr commented Oct 6, 2021

Not sure how to do that.

For example: https://github.com/search?q=org%3Aw3c+%22now+visible+algorithm%22&type=code

(The file hint at the specs relying on the exported definition)

Doing the above is optional... we can just "break" the references and then just email the various WGs to let them know we've moved stuff... we can deal with that before we merge tho.

The users are https://github.com/w3c/device-posture, https://github.com/w3c/screen-orientation and https://github.com/w3c/web-nfc. Once the HTML spec changes are in I'll create issues for those.

<a>Fire an event</a> named "`visibilitychange`" that bubbles, isn't
cancelable, and has no default action, at the |doc|.
</li>
<li>Run <dfn data-export="" data-dfn-type="abstract-op">external now
Copy link
Contributor

Choose a reason for hiding this comment

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

I vaguely remember other specifications relying on this. Worthwhile to dig through past commits to see why we added this.

Copy link
Author

@noamr noamr Oct 6, 2021

Choose a reason for hiding this comment

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

The comments on this issue say otherwise, but it's possible...
I think the discussion here went in the direction of using direct calls rather than hooks, I tend to support that - much more straightforward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see this already came up: #73 (comment)
Not saying we need to keep this, just that we need to give those specs a proper heads up.

noamr added a commit to noamr/html that referenced this pull request 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
It's defined inside the HTML spec.
@noamr
Copy link
Author

noamr commented Oct 15, 2021

I gave the other specs a heads up (opened issues referencing this PR).
The HTML changes are in.
@yoavweiss, care to review?

visible algorithm</a> before running the step to fire the
[=Window/pageshow=] event.
</li>
<li>Otherwise, <a>queue a task</a> that runs the <a>now visible
Copy link
Contributor

Choose a reason for hiding this comment

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

The events defined in HTML now seem to be run synchronously, which is not what was previously defined. Is this intentional? Do we know what implementations are doing here?

Copy link
Author

Choose a reason for hiding this comment

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

The HTML spec only defines the event order in the cases of unload/page hide. Those have to be synchronous otherwise they will never be handled

What's not defined is the non standard firing of that event

Copy link
Contributor

Choose a reason for hiding this comment

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

The HTML spec now fires the events directly, rather than queueing a task for them (which is what this spec defined before this PR). It'd be good to have tests that verify that the events indeed fire synchronously.

Can you expand on "Those have to be synchronous otherwise they will never be handled"?

/cc @rakina

Copy link
Author

Choose a reason for hiding this comment

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

  • If traversing to a session history entry, run the now
    visible algorithm before running the step to fire the
    [=Window/pageshow=] event.’

    that’s the equivalent line in the current spec… synchronous

    if the document is being unloaded or suspended into BFcache, which are the cases handled by the HTML spec, when is the handler going to be called? The JS execution context is about to be gone…

  • Copy link
    Contributor

    Choose a reason for hiding this comment

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

    I re-read the algorithm, and you're right. For the cases of history traversal and unload, the events fire synchronously. They are firing async in "other cases" (e.g. tab moved to the background, tab switcher, app switcher, etc), but those other cases are not currently well-defined...

    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    https://html.spec.whatwg.org/multipage/webappapis.html#rendering-opportunity seems to use "page is in the background" without defining it. At worst, maybe we can use something like "page switches from background to foreground" to call the right visibility events?

    Copy link
    Author

    Choose a reason for hiding this comment

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

    noamr pushed a commit to noamr/html that referenced this pull request Oct 18, 2021
    - Expose the necessary attributes in the document WebIDL
    - Define "system visibility state" and document "visibility state"
      to represent the algorithms for updating the visibility
    - Update current reference of "page is in the background" to refer
      to the new definitions.
    
    To fully integrate specs referring to the Page Visibility spec
    with the changes in the HTML spec, i.e. defining the visibilitychange
    event, the different specs need to be called explicitly when the
    visibility of the page changes.
    
    For that, the visibilityState of the document needs to be properly
    defined within HTML.
    
    See w3c/page-visibility#74
    and w3c/page-visibility#73
    domenic pushed a commit to noamr/html that referenced this pull request Nov 3, 2021
    Defines the document.hidden and document.visibilityState APIs in the HTML Standard, while improving their definitions to be more rigorous. This is similar to 3285b98 which did the same for the visibilitychange event.
    
    Also updates the discussion of "page is in the background" to refer to the new definitions.
    
    See w3c/page-visibility#74 and w3c/page-visibility#73 for more background.
    domenic pushed a commit to noamr/html that referenced this pull request Nov 3, 2021
    Defines the document.hidden and document.visibilityState APIs in the HTML Standard, while improving their definitions to be more rigorous. This is similar to 3285b98 which did the same for the visibilitychange event.
    
    Also updates the discussion of "page is in the background" to refer to the new definitions.
    
    See w3c/page-visibility#74 and w3c/page-visibility#73 for more background.
    domenic pushed a commit to whatwg/html that referenced this pull request Nov 3, 2021
    Defines the document.hidden and document.visibilityState APIs in the HTML Standard, while improving their definitions to be more rigorous. This is similar to 3285b98 which did the same for the visibilitychange event.
    
    Also updates the discussion of "page is in the background" to refer to the new definitions.
    
    See w3c/page-visibility#74 and w3c/page-visibility#73 for more background.
    @noamr noamr closed this Nov 7, 2021
    @noamr noamr deleted the issue-51 branch November 7, 2021 16:19
    dandclark pushed a commit to dandclark/html that referenced this pull request 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.
    dandclark pushed a commit to dandclark/html that referenced this pull request Dec 4, 2021
    Defines the document.hidden and document.visibilityState APIs in the HTML Standard, while improving their definitions to be more rigorous. This is similar to 3285b98 which did the same for the visibilitychange event.
    
    Also updates the discussion of "page is in the background" to refer to the new definitions.
    
    See w3c/page-visibility#74 and w3c/page-visibility#73 for more background.
    mfreed7 pushed a commit to mfreed7/html that referenced this pull request 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.
    mfreed7 pushed a commit to mfreed7/html that referenced this pull request Jun 3, 2022
    Defines the document.hidden and document.visibilityState APIs in the HTML Standard, while improving their definitions to be more rigorous. This is similar to 3285b98 which did the same for the visibilitychange event.
    
    Also updates the discussion of "page is in the background" to refer to the new definitions.
    
    See w3c/page-visibility#74 and w3c/page-visibility#73 for more background.
    Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    HTML spec should call visibilitychange algorithms directly.
    3 participants