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?
FWIW, test page: http://output.jsbin.com/zubiyid/latest/quiet - load it, unload the page and visit it again.
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/
<rdar://problem/23688763>
Mass move bugs into the DOM component.
This is a duplicate of issue #116769.
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.
FWIW, pagehide event is still fired so that would be the workaround for now. We should really fix this bug though...
(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.”
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.
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.
Created attachment 409705 [details] Patch
Created attachment 409708 [details] Patch
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 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 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.
(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.
Created attachment 409715 [details] Patch
(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.
Created attachment 409717 [details] Patch
(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.
Committed r267589: <https://trac.webkit.org/changeset/267589> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409717 [details].
Does this resolve bug 116770?
Reverted r267589 for reason: Broke document.visibilityState when coming out of back/forward cache Committed r267593: <https://trac.webkit.org/changeset/267593>
(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.
Created attachment 409751 [details] Patch
(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 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?
(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.
Created attachment 409760 [details] Patch
Committed r267614: <https://trac.webkit.org/changeset/267614> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409760 [details].
*** Bug 116769 has been marked as a duplicate of this bug. ***
*** Bug 116770 has been marked as a duplicate of this bug. ***
*** Bug 194897 has been marked as a duplicate of this bug. ***