My favorites | Sign in
Project Home Downloads Wiki Issues Code Search
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 138324: window.history.pushState incorrectly exits fullscreen
55 people starred this issue and may be notified of changes. Back to list
 
Project Member Reported by scheib@chromium.org, Jul 20, 2012
While in HTML Fullscreen, navigation that removes the full screen element from the DOM should exit fullscreen. See spec http://dvcs.w3.org/hg/fullscreen/raw-file/tip/Overview.html#fully-exit-fullscreen

Using win_history.pushState(state, title, url) should not cause fullscreen to exit.

Jul 20, 2012
#1 scheib@chromium.org
Koz, happen to know how to differentiate navigation moving us away from a page, and window.history.pushstate?

FullscreenController::Observe is too sensitive now.
https://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/fullscreen/fullscreen_controller.cc&exact_package=chromium&q=FullscreenController::Observe&type=cs&l=46
Cc: koz@chromium.org
Jul 20, 2012
#2 scheib@chromium.org
(No comment was entered for this change.)
Labels: -Mstone-22 Mstone-21 ReleaseBlock-Stable
Jul 23, 2012
#3 scheib@chromium.org
(No comment was entered for this change.)
Status: Started
Labels: -ReleaseBlock-Stable
Jul 23, 2012
#4 koz@chromium.org
Sorry, I don't. I would have thought that is_navigation_to_different_page() would be enough to handle that case.
Aug 7, 2012
#5 pdo...@gmail.com
This bug seems to have made its way into the stable channel release in M21.
Aug 8, 2012
#6 scheib@chromium.org
pdokas, correct. This bug is a side effect of a crash fix. Unfortunately the fix for this is not trivial, as we don't currently send enough information to the browser to distinguish these navigations: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/ZFKzN96xxCY/discussion, and that will need to be fixed first.
Labels: -Pri-1 -Mstone-21 Pri-2 Mstone-22
Aug 23, 2012
#7 scheib@chromium.org
Stalled due to non trivial implementation required. To reopen needs to first create a blocking issue to enable the detection of this type of navigation browser side -- or perhaps solve instead inside WebKit implementation.
Status: Assigned
Labels: -Mstone-22
Sep 10, 2012
#8 scheib@chromium.org
 Issue 147418  has been merged into this issue.
Cc: yzs...@chromium.org scheib@chromium.org rsesek@chromium.org
Nov 19, 2012
#9 francoislaberge
I was wondering why our Website was no longer navigating in fullscreen. Finally hunted down this bug. Any movement on getting it back into fixing rotation? 

See our Game Console that breaks out of fullscreen now when ever you navigate:
http://playbrassmonkey.com/
Mar 10, 2013
#10 bugdro...@chromium.org
(No comment was entered for this change.)
Labels: -Area-WebKit -Feature-FullScreen -Feature-Input-MouseLock Cr-Content Cr-UI-Browser-FullScreen Cr-IO-MouseLock
Apr 10, 2013
#12 scheib@chromium.org
Migrating browser fullscreen issues to jhawkins.
Owner: jhawk...@chromium.org
Sep 12, 2013
#13 zgol...@gmail.com
Still an issue, and it really kills our js one page app in fullscreen.
Nov 11, 2013
#14 rsch...@chromium.org
(No comment was entered for this change.)
Labels: Hotlist-GoogleApps
Nov 11, 2013
#15 rsch...@chromium.org
Internal issue 11542947.
Nov 20, 2013
#16 komoro...@chromium.org
[+ Nate]

I was just talking to scheib@ about this. He investigated back in the day, but the changes would require work in Blink to wire things back out to Chrome so it could differentiate a REAL navigation (which should cancel fullscreen for security reasons) and a "fake" navigation (i.e. pushState).

Nate, do you know the relevant parts of Blink, or know who does?
Owner: ---
Cc: jap...@chromium.org
Nov 20, 2013
#17 jap...@chromium.org
Yeah, I think I inherited pushState. We plumb something similar partway already, but we'd need to tweak it and send it all the way out to chromium.
Dec 6, 2013
#18 ssavi...@google.com
Any update on milestone this may be targeted to?
Jan 6, 2014
#19 rsch...@chromium.org
Ping on this. Any updates?
Jan 6, 2014
#20 jap...@chromium.org
See 50298 for replumbing.
Jan 9, 2014
#21 f...@google.com
Awesome, this will be great for Docs, because the workaround we have in place doesn't work on all cases.

Greg said you're doing all the work to move history from the renderer to the browser process, so I'm assigning this to you so you can fix it once that's in place. Thank you!
Owner: jap...@chromium.org
Jan 28, 2014
#22 rsch...@chromium.org
I'm assuming this is in the state as 50298 still?
Apr 29, 2014
#23 rsch...@chromium.org
Do we know whether this is resolved too?
Apr 29, 2014
#24 francoislaberge
We have this problem on our website and it still exists:
http://playbrassmonkey.com/
Apr 29, 2014
#25 jap...@chromium.org
creis, do you have any thoughts on this?

It looks like FullscreenController is listening to the NOTIFICATION_NAV_ENTRY_COMMITTED notification, and is checking LoadCommittedDetails::is_navigation_to_different_page to determine whether to exit fullscreen.

That bit is set by IsURLInPageNavigation(), which appears to be setting it to true for history.pushState() same-document navigation. I wouldn't have expected that. Is that what you would expect? Is there something else this should be relying on?
Cc: creis@chromium.org
Apr 30, 2014
#26 creis@chromium.org
Comment 25: I would expect IsURLInPageNavigation to be true for pushState, which would have made is_navigation_to_different_page false.  There's probably a bug if that's not happening.

I don't have a great understanding of IsURLInPageNavigation, but it relies on both a cue from the renderer (renderer_says_in_page) and the navigation type.  Looking closer, I don't see where AreURLsInPageNavigation would detect a pushState, so maybe it needs to.

@tsepez: Do you remember any context from r154287?  Should that be detecting pushState as an in-page navigation?
Cc: tsepez@chromium.org
Apr 30, 2014
#27 tsepez@chromium.org
@creis - history.pushState() *to the same URL* should be an in-page navigation (as told to us by the renderer).  I believe you can pushState to other pages in the same origin, for instance, in which case it wouldn't be an in-page.
Apr 30, 2014
#28 creis@chromium.org
Hmm, that's not how I understand pushState.  You can change the URL with pushState, but the document you're looking at doesn't actually change (or even change scroll position).  In that sense, it's kind of like an in-page navigation.
Apr 30, 2014
#29 morto...@googlemail.com
Please don't lose track of the original issue: leaving full-screen using PushState though the fullscreen element has not changed. This is the driving use-case. I should be able to modify the URL and not leave full-screen so long as the full-screen element remains in the DOM.

This is the exact issue I have on http://puzzlepuzzle.net/ where I have to turn off the history mechanism in Chrome to avoid leaving full-screen mode.
Apr 30, 2014
#30 francoislaberge
@comment #29 I've been thinking of somehow hacking my use of the History API in this case to somehow work with fullscreen mode. I just haven't thought about it enough to figure out how to do it. 
Apr 30, 2014
#31 kentak...@gmail.com
i found a workaround for this by checking document.webkitIsFullScreen... in that case just store your path changes in local var and fire your listener manually.  when you are not in fullscreen you can go back to your normal workflow.  most users wouldnt be checking he url in fullscreen mode anyway.
Apr 30, 2014
#32 francoislaberge
That definitely sounds like how I'd do it. Did you hack in passing around and restoring state? 
Apr 30, 2014
#33 francoislaberge
Can someone explain why this is so hard to fix? It really blocks cool slick UI behavior. Is there debate over security or some implementation complexity? Or is it just low priority in the scheme of things?
Apr 30, 2014
#34 francoislaberge
Also, it used to work as I filed a comment way back pointing out the regression:
https://code.google.com/p/chromium/issues/detail?id=138324#c9
May 19, 2014
#35 jap...@chromium.org
 Issue 374617  has been merged into this issue.
May 19, 2014
#36 melvyn....@gmail.com
@comment #29

Yes please fix the original issue.
May 19, 2014
#37 melvyn....@gmail.com
To: kentak...@gmail.com, francoislaberge
Re: #31 & #32

Can you please elaborate on the workaround that you have?  Can I see some code?  Thanks!

May 20, 2014
#38 j...@cloudpartyinc.com
We worked around it by wrapping the History API, so that right before we enter fullscreen we push a dummy state into the history (to detect the user pressing Back), and then while in fullscreen it just queues up all of the history changes, and the applies them when either we exit fullscreen, or the user tries to use Back in the browser.  Here's our wrapper modules for some example code (not usable as-is, has some dependencies on other modules, and not in the same module system you're using):
http://thesilentb.com/files/History.js
http://thesilentb.com/files/Fullscreen.js
May 22, 2014
#39 melvyn....@gmail.com
Re: #38 j...@cloudpartyinc.com

Thanks for the details!  I was able to work around it as well by doing pretty much the same thing.  It was really annoying.  I hope Google fixes the bug on their end soon.  Gentle bump!

May 27, 2014
#40 miu@chromium.org
(No comment was entered for this change.)
Cc: miu@chromium.org
May 28, 2014
#41 jap...@chromium.org
@tsepez: Re: history.pushState(), the blink implementation and my understanding of the spec is that it is *always* same document. Also, I believe the spec forbids pushState/replaceState to a different origin (step 2.4 in http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#dom-history-pushstate).

Would it be sufficient to change AreURLsInPageNavigation() to check the before and after origins before trusting renderer_says_in_page, instead of requiring the urls to match in their entirety?
May 28, 2014
#42 tsepez@chromium.org
@japhet - yep, that's my understanding now that I look at it again.  I think your change is reasonable from an origin perspective (if by "check before and after origins" you mean checking that they're the same origin).
May 28, 2014
#43 jap...@chromium.org
Yeah, that's what I meant. I'll draft that change and see what tests break.
May 28, 2014
#44 bugdro...@chromium.org
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=175011

------------------------------------------------------------------
r175011 | japhet@chromium.org | 2014-05-28T23:35:41.039424Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/FrameLoader.cpp?r1=175011&r2=175010&pathrev=175011
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/DocumentLoader.cpp?r1=175011&r2=175010&pathrev=175011
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/DocumentLoader.h?r1=175011&r2=175010&pathrev=175011

Reset DocumentLoader::m_request's method to GET for pushState and replaceState

We currently correctly send a GET in the POST->replaceState->reload case, but
DocumentLoader::m_request still shows a method of POST, and as a result we
incorrectly report it as a POST to the browser process. This contributes to
unnecessarily showing a form resubmission warning in that case.

BUG=138324

Review URL: https://codereview.chromium.org/302543008
-----------------------------------------------------------------
May 28, 2014
#45 jap...@chromium.org
Oops, sorry. Put the wrong bug # in the commit message, ignore that bugdroid comment.
Jun 4, 2014
#46 bugdro...@chromium.org
------------------------------------------------------------------
r274734 | japhet@chromium.org | 2014-06-04T09:00:39.364252Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/android_webview/java/src/org/chromium/android_webview/AwContentsClient.java?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/frame_host/navigation_controller_impl_unittest.cc?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/frame_host/navigation_controller_impl.cc?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/android/java/src/org/chromium/content/browser/WebContentsObserverAndroid.java?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/translate/translate_manager_render_view_host_unittest.cc?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/frame_host/navigation_controller_impl.h?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/test/test_renderer_host.cc?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/android/web_contents_observer_android.cc?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/test_render_frame_host.cc?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/active_script_controller_unittest.cc?r1=274734&r2=274733&pathrev=274734
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java?r1=274734&r2=274733&pathrev=274734

Trust the renderer's same-document navigation flag if it is a same-origin nav.

Currently in AreURLsInPageNavigation, we only trust renderer_says_in_page if
the before and after urls are identical. This prevents us from correctly
classifying history.pushState and history.replaceState navigations as in-page.
Navigations via the history API are required to be same-origin, but can differ
by more than just the ref component, so we get the correct behavior without
the renderer process being able to lie about a cross-origin navigation.

BUG=138324
TEST=Added cases to NavigationControllerTest.IsInPageNavigation

Review URL: https://codereview.chromium.org/304763002
-----------------------------------------------------------------
Jun 4, 2014
#47 bugdro...@chromium.org
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d5cb21f876a51c4fddcb90954e0dd819a09a7a5

commit 8d5cb21f876a51c4fddcb90954e0dd819a09a7a5
Author: japhet@chromium.org <japhet@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Wed Jun 04 09:00:39 2014

Trust the renderer's same-document navigation flag if it is a same-origin nav.

Currently in AreURLsInPageNavigation, we only trust renderer_says_in_page if
the before and after urls are identical. This prevents us from correctly
classifying history.pushState and history.replaceState navigations as in-page.
Navigations via the history API are required to be same-origin, but can differ
by more than just the ref component, so we get the correct behavior without
the renderer process being able to lie about a cross-origin navigation.

BUG=138324
TEST=Added cases to NavigationControllerTest.IsInPageNavigation

Review URL: https://codereview.chromium.org/304763002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274734 0039d316-1c4b-4281-b951-d872f2087c98


Jun 4, 2014
#48 francoislaberge
Stupid question, but does this mean I can download Canary and this issue is resolved?
Jun 4, 2014
#49 jap...@chromium.org
It didn't make the cutoff for today's canary. The odds are good for tomorrow's canary and the next dev channel.

If you don't want to wait until tomorrow, you can download and manually install a binary from our continuous build: https://commondatastorage.googleapis.com/chromium-browser-continuous/index.html . Any build number 274734 or higher, though no guarantees that any given binary actually works :) 

Marking as fixed, since this shouldn't require further code changes. Let me know if you're still having problems as this makes its way to the various channels.
Status: Fixed
Jun 4, 2014
#50 jap...@chromium.org
 Issue 325180  has been merged into this issue.
Cc: agau...@chromium.org srsrid...@chromium.org kenjibaheux@chromium.org
Jun 5, 2014
#51 bugdro...@chromium.org
The following revision refers to this bug:
  https://chrome-internal.googlesource.com/bling/chromium.git/+/8d5cb21f876a51c4fddcb90954e0dd819a09a7a5

commit 8d5cb21f876a51c4fddcb90954e0dd819a09a7a5
Author: japhet@chromium.org <japhet@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Wed Jun 04 09:00:39 2014

Jun 9, 2014
#52 kenjibaheux@chromium.org
 Issue 325180  has been merged into this issue.
Jun 9, 2014
#53 corkoome...@gmail.com
fyi: slideshows in Microsoft One Drive stay full screen now (Canary 37.0.2039.0). Thanks!
Aug 7, 2014
#54 jap...@chromium.org
 Issue 171670  has been merged into this issue.
Cc: falken@chromium.org phil...@opera.com j...@opera.com jsc...@chromium.org
Sign in to add a comment

Powered by Google Project Hosting