Bug 151234 – visibilitychange:hidden doesn't fire during page navigations

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ojan Vafai]

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?

[Ilya Grigorik]

FWIW, test page: http://output.jsbin.com/zubiyid/latest/quiet - load it, unload the page and visit it again.

[Ilya Grigorik]

Related issue https://bugs.webkit.org/show_bug.cgi?id=151610, and some additional context: https://www.igvita.com/2015/11/20/dont-lose-user-and-app-state-use-page-visibility/

[Radar WebKit Bug Importer]

<rdar://problem/23688763>

[Lucas Forschler]

Mass move bugs into the DOM component.

[Daniel]

This is a duplicate of issue #116769.

[sideshowbarker]

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.

[Ryosuke Niwa]

FWIW, pagehide event is still fired so that would be the workaround for now. We should really fix this bug though...

[sideshowbarker]

(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.”

[Ryosuke Niwa]

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.

[sideshowbarker]

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.

[Chris Dumez]

Created attachment 409705 [details]
Patch

[Chris Dumez]

Created attachment 409708 [details]
Patch

[Ryosuke Niwa]

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.

[Darin Adler]

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.

[Chris Dumez]

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.

[Darin Adler]

(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.

[Chris Dumez]

Created attachment 409715 [details]
Patch

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 409717 [details]
Patch

[Darin Adler]

(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.

[EWS]

Committed r267589: <https://trac.webkit.org/changeset/267589>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409717 [details].

[Simon Fraser (smfr)]

Does this resolve bug 116770?

[Chris Dumez]

Reverted r267589 for reason:

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

Committed r267593: <https://trac.webkit.org/changeset/267593>

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 409751 [details]
Patch

[Chris Dumez]

(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.

[Ryosuke Niwa]

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?

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 409760 [details]
Patch

[EWS]

Committed r267614: <https://trac.webkit.org/changeset/267614>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409760 [details].

[Chris Dumez]

*** Bug 116769 has been marked as a duplicate of this bug. ***

[Chris Dumez]

*** Bug 116770 has been marked as a duplicate of this bug. ***

[Sam Sneddon [:gsnedders]]

*** Bug 194897 has been marked as a duplicate of this bug. ***