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

Fire "visibilitychange" events explicitly #7153

Merged
merged 9 commits into from
Oct 15, 2021
Merged

Conversation

noamr
Copy link
Contributor

@noamr noamr commented 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

  • At least two implementers are interested (and none opposed):

    • This is not a behavior change.
  • Tests are written and can be reviewed and commented upon at:

    • wpt/page-visibility covers this.
  • Implementation bugs are filed:


/browsing-the-web.html ( diff )
/dom.html ( diff )
/indices.html ( diff )
/webappapis.html ( diff )

@marcoscaceres
Copy link
Member

Make sure "pagevisibility" is also defined in https://html.spec.whatwg.org/multipage/indices.html#events-2

source Outdated Show resolved Hide resolved
@noamr
Copy link
Contributor Author

noamr commented Oct 7, 2021

@domenic r?

@foolip
Copy link
Member

foolip commented Oct 7, 2021

Are there any tests for this? git grep pagevisibility shows not a single match in web-platform-tests for me.

@foolip
Copy link
Member

foolip commented Oct 7, 2021

Oh, the event name is "visibilitychange", I'll update the PR description.

@foolip foolip changed the title Fire "pagevisibility" events explicitly Fire "visibilitychange" events explicitly Oct 7, 2021
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

This is wonderful.

@domenic
Copy link
Member

domenic commented Oct 14, 2021

Are there tests with regard to the relative order between visibilitychange and pagehide/pageshow?

@domenic
Copy link
Member

domenic commented Oct 14, 2021

Some other possible test gaps (but maybe I just can't find the tests):

  • Test that cancelable is false
  • Tests for the non-unload case, e.g. restoration from bfcache

I don't think we necessarily need to block on total coverage, given that this is a refactoring, but it might be good to avoid busywork if we need to go back and correct things later.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Oh, found a potentially big missing piece: the definition of onvisibilitychange.

It looks like this is a special Document-only event handler, like onreadystatechange. So I'd search for the two places onreadystatechange shows up and add it to those.

@noamr
Copy link
Contributor Author

noamr commented Oct 15, 2021

Oh, found a potentially big missing piece: the definition of onvisibilitychange.

It looks like this is a special Document-only event handler, like onreadystatechange. So I'd search for the two places onreadystatechange shows up and add it to those.

It exists in the pagevisibility spec, but I can move it here instead (which would make the pagevisibility spec somewhat empty)

@noamr
Copy link
Contributor Author

noamr commented Oct 15, 2021

Oh, found a potentially big missing piece: the definition of onvisibilitychange.
It looks like this is a special Document-only event handler, like onreadystatechange. So I'd search for the two places onreadystatechange shows up and add it to those.

It exists in the pagevisibility spec, but I can move it here instead (which would make the pagevisibility spec somewhat empty)

Done in new PR version.

@domenic
Copy link
Member

domenic commented Oct 15, 2021

You also need to add it to Document's IDL block

@noamr
Copy link
Contributor Author

noamr commented Oct 15, 2021

You also need to add it to Document's IDL block

Done

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

I was going to fix the spacing issue myself and merge, but then I saw the reference issue.

<ref spec=PAGEVIS></p>
</li>
<ol>
<li><p>Set <var>document</var>'s <span>page showing</span> flag to false.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

This is two-space indents, but it should be 1-space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

<td> <dfn event for="Document"><code data-x="event-visibilitychange">visibilitychange</code></dfn>
<td> <code>Event</code>
<td> <code>Document</code>
<td> Fired at the <code>Document</code> object when the page becomes visible or hidden to the user. [[PAGEVIS]]
Copy link
Member

Choose a reason for hiding this comment

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

This will just produce the literal text [[PAGEVIS]] in the output. In HTML you need to use <ref spec=PAGEVIS>. (And, if you do that, you need to not delete the reference on line 126072.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Great, thanks so much; this is way cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants