Bug 151234 - visibilitychange:hidden doesn't fire during page navigations
Summary: visibilitychange:hidden doesn't fire during page navigations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
: 116769 116770 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-11-12 16:39 PST by Ojan Vafai
Modified: 2020-09-25 17:54 PDT (History)
24 users (show)

See Also:


Attachments
Patch (41.07 KB, patch)
2020-09-25 10:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (42.36 KB, patch)
2020-09-25 10:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (40.10 KB, patch)
2020-09-25 12:02 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (40.06 KB, patch)
2020-09-25 12:07 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (46.30 KB, patch)
2020-09-25 15:47 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (46.41 KB, patch)
2020-09-25 16:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2015-11-12 16:39:25 PST
visibility state should transition to hidden whenever a page might go away. Unlike unload/pagehide, we can fire these reliably. See https://github.com/w3c/page-visibility/issues/18 for more details, particularly, igrigorik's table near the top that shows where browser's don't fire this. The Firefox behavior of having green for all of the visibility-change-->hidden column is the one that matches the spec and makes for the best developer platform.

Ryosuke, are you the right person on WebKit to talk to about this?
Comment 1 Ilya Grigorik 2015-11-12 16:54:20 PST
FWIW, test page: http://output.jsbin.com/zubiyid/latest/quiet - load it, unload the page and visit it again.
Comment 3 Radar WebKit Bug Importer 2015-11-30 09:01:03 PST
<rdar://problem/23688763>
Comment 4 Lucas Forschler 2019-02-06 09:18:44 PST
Mass move bugs into the DOM component.
Comment 5 Daniel 2019-06-21 14:06:39 PDT
This is a duplicate of issue #116769.
Comment 6 Michael[tm] Smith 2020-09-24 22:19:37 PDT
Please see https://bugs.webkit.org/show_bug.cgi?id=116769#c7 for some details about where (back in 2013…) this issue was discussed, and the HTML spec change that was made in relation to it.

I hope we can get this resolved — because among other possible consequences, I take it that this may be preventing web developers from being able to use document.sendBeacon() in the way it’s intended to be used.
Comment 7 Ryosuke Niwa 2020-09-24 22:26:49 PDT
FWIW, pagehide event is still fired so that would be the workaround for now. We should really fix this bug though...
Comment 8 Michael[tm] Smith 2020-09-24 22:59:34 PDT
(In reply to Ryosuke Niwa from comment #7)
> FWIW, pagehide event is still fired so that would be the workaround for now.
> We should really fix this bug though...

Hi Ryosuke — thanks much for the response.

Yeah, I have gleaned that using the pagehide event is the workaround — and I’m actually working on documenting things that way in MDN and the browser-compat-data repo (BCD), which is sort of how I ended up here.

So yeah I guess what I’m hoping for is that we get this resolved such that we don’t need to have MDN and BCD forever essentially telling developers, “You can rely on the visibilitychange event interoperably firing as expected in every current browser except Safari — for which you’ll also need to write extra special-case code to additionally check the pagehide event.”
Comment 9 Ryosuke Niwa 2020-09-24 23:38:01 PDT
FWIW, this is probably one of the easiest bugs to fix! I'd mention it in getting-started on webkit's slack and see if anyone finds it interesting enough to fix it.
Comment 10 Michael[tm] Smith 2020-09-25 02:31:38 PDT
FYI https://github.com/mdn/browser-compat-data/pull/6763 is a pull request I raised today to update the MDN Browser Compatibility data (tables in MDN) to give a clear indication web developers about this issue.
Comment 11 Chris Dumez 2020-09-25 10:15:11 PDT
Created attachment 409705 [details]
Patch
Comment 12 Chris Dumez 2020-09-25 10:47:24 PDT
Created attachment 409708 [details]
Patch
Comment 13 Ryosuke Niwa 2020-09-25 11:50:04 PDT
Comment on attachment 409708 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409708&action=review

> Source/WebCore/dom/Document.h:487
> +    void setVisibilityStateOverride(Optional<VisibilityState> visibilityState) { m_visibilityStateOverride = visibilityState; }

Since we don't need a generic override mechanism,
I'd prefer having a boolean state and call this something like setIsUnloaded.

> Source/WebCore/loader/FrameLoader.cpp:3280
> +            m_frame.document()->dispatchEvent(Event::create(eventNames().visibilitychangeEvent, Event::CanBubble::Yes, Event::IsCancelable::No));

We can't always fire event here. We should put this inside unloadEvebtPolicy check below.
Comment 14 Darin Adler 2020-09-25 11:52:13 PDT
Comment on attachment 409708 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409708&action=review

> Source/WebCore/loader/FrameLoader.cpp:3278
> +            //  Dispatch visibilitychange event.

I don’t think we need this comment, because the code says dispatchEvent, visibilitychangeEvent. If we want to make that more readable I suggest helper functions to get rid of some of the distracting additional items.

On the other hand I think it would be worthwhile to comment about why we are overriding the visibility state. That doesn't seem obvious to me. Possible topics would be why we need to override it now, why we didn’t do it earlier, and why it’s OK to leave it that way permanently.
Comment 15 Chris Dumez 2020-09-25 11:54:47 PDT
Comment on attachment 409708 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409708&action=review

>> Source/WebCore/loader/FrameLoader.cpp:3278
>> +            //  Dispatch visibilitychange event.
> 
> I don’t think we need this comment, because the code says dispatchEvent, visibilitychangeEvent. If we want to make that more readable I suggest helper functions to get rid of some of the distracting additional items.
> 
> On the other hand I think it would be worthwhile to comment about why we are overriding the visibility state. That doesn't seem obvious to me. Possible topics would be why we need to override it now, why we didn’t do it earlier, and why it’s OK to leave it that way permanently.

Once a document is unloaded, there is no going back. I will replace with a isUnloaded boolean as Ryosuke suggested.

>> Source/WebCore/loader/FrameLoader.cpp:3280
>> +            m_frame.document()->dispatchEvent(Event::create(eventNames().visibilitychangeEvent, Event::CanBubble::Yes, Event::IsCancelable::No));
> 
> We can't always fire event here. We should put this inside unloadEvebtPolicy check below.

In this scope, the unload event will be fired no matter what so I think we need to fire the visibilitychange event also unconditionally here too. unloadEventPolicy controls whether or not to fire the pagehide event here.
Comment 16 Darin Adler 2020-09-25 12:00:48 PDT
(In reply to Chris Dumez from comment #15)
> Once a document is unloaded, there is no going back.

Yes, I understood that. My point is that someone in the future might not, which is why I suggested a comment.
Comment 17 Chris Dumez 2020-09-25 12:02:07 PDT
Created attachment 409715 [details]
Patch
Comment 18 Chris Dumez 2020-09-25 12:02:45 PDT
(In reply to Darin Adler from comment #16)
> (In reply to Chris Dumez from comment #15)
> > Once a document is unloaded, there is no going back.
> 
> Yes, I understood that. My point is that someone in the future might not,
> which is why I suggested a comment.

I don't think a comment is still needed after applying Ryosuke's suggestion (see latest patch iteration) but please let me know if you disagree.
Comment 19 Chris Dumez 2020-09-25 12:07:42 PDT
Created attachment 409717 [details]
Patch
Comment 20 Darin Adler 2020-09-25 12:15:09 PDT
(In reply to Chris Dumez from comment #18)
> I don't think a comment is still needed after applying Ryosuke's suggestion
> (see latest patch iteration) but please let me know if you disagree.

Sure, looks fine.
Comment 21 EWS 2020-09-25 13:11:53 PDT
Committed r267589: <https://trac.webkit.org/changeset/267589>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409717 [details].
Comment 22 Simon Fraser (smfr) 2020-09-25 13:17:47 PDT
Does this resolve bug 116770?
Comment 23 Chris Dumez 2020-09-25 14:29:52 PDT
Reverted r267589 for reason:

Broke document.visibilityState when coming out of back/forward cache

Committed r267593: <https://trac.webkit.org/changeset/267593>
Comment 24 Chris Dumez 2020-09-25 14:32:02 PDT
(In reply to Simon Fraser (smfr) from comment #22)
> Does this resolve bug 116770?

I will but I had a bug. I am reworking the patch.
Comment 25 Chris Dumez 2020-09-25 15:47:36 PDT
Created attachment 409751 [details]
Patch
Comment 26 Chris Dumez 2020-09-25 15:50:08 PDT
(In reply to Chris Dumez from comment #24)
> (In reply to Simon Fraser (smfr) from comment #22)
> > Does this resolve bug 116770?
> 
> I will but I had a bug. I am reworking the patch.

This new iteration now correctly deals with the back/forward cache and we now match Firefox's behavior in this regard.
Comment 27 Ryosuke Niwa 2020-09-25 16:33:40 PDT
Comment on attachment 409751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=409751&action=review

> Source/WebCore/loader/FrameLoader.cpp:3284
> +            m_frame.document()->setHiddenDueToDismissal(true);

This is confusing name. How about setVisibilityHiddenDueToPageDismissal?
Comment 28 Chris Dumez 2020-09-25 16:34:43 PDT
(In reply to Ryosuke Niwa from comment #27)
> Comment on attachment 409751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409751&action=review
> 
> > Source/WebCore/loader/FrameLoader.cpp:3284
> > +            m_frame.document()->setHiddenDueToDismissal(true);
> 
> This is confusing name. How about setVisibilityHiddenDueToPageDismissal?

Yes, I had trouble finding a good name. I will go with your suggestion unless someone has a better idea.
Comment 29 Chris Dumez 2020-09-25 16:37:05 PDT
Created attachment 409760 [details]
Patch
Comment 30 EWS 2020-09-25 17:50:05 PDT
Committed r267614: <https://trac.webkit.org/changeset/267614>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409760 [details].
Comment 31 Chris Dumez 2020-09-25 17:54:43 PDT
*** Bug 116769 has been marked as a duplicate of this bug. ***
Comment 32 Chris Dumez 2020-09-25 17:54:58 PDT
*** Bug 116770 has been marked as a duplicate of this bug. ***