Bug 41372

Summary: popstate event is not fired until document opts in by calling pushstate.
Product: WebKit Reporter: Justin Lebar <justin.lebar+bug>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, commit-queue, fishd, ishermandom+bugs, mihaip
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
URL: http://people.mozilla.org/~jlebar/test/webkit/popstate-load.html
Attachments:
Description Flags
Test case for popstate not being fired
none
Test case for popstate not being fired
none
Patch
none
Patch
none
Patch none

Description Justin Lebar 2010-06-29 14:13:08 PDT
STR:

* Open the JS console
* Visit http://people.mozilla.org/~jlebar/test/webkit/popstate-load.html

Expected results:

* Console displays "popstate".

Actual results:

* Console displays nothing.

Chrome 5.0.375.86 on Ubuntu 10.04.

It seems that the popstate event is not fired until the document opts in by calling pushstate.  That is, if you visit the test page, click back, and then click forward, the page also doesn't get a popstate event.  But if you click the pushstate button, then going back and forward always gives a popstate, even when you're going forward to the original page.

Step 10 in the history traversal algorithm [1] suggests that this is the wrong behavior.  The line at [2] which says "the popstate event is fired when navigating to a session history entry  that represents a state object" is, I think, misleading.  I'll post to whatwg to see if we can get that changed.

[1]: http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#history-traversal
Comment 1 Justin Lebar 2010-06-29 14:14:41 PDT
Forgot to include a link above:

[2]: http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#event-definitions
Comment 2 Justin Lebar 2010-06-29 14:17:44 PDT
I should note that the current implementation also isn't consistent with the language from [2] ("the popstate event is fired when navigating to a session history entry that represents a state object").

After a pushstate, only the second history entry has a state object, but Chrome fires a popstate when you navigate to either the first or the second state object.
Comment 3 Darin Fisher (:fishd, Google) 2010-06-30 10:16:00 PDT
> After a pushstate, only the second history entry has a state object, but Chrome 
> fires a popstate when you navigate to either the first or the second state 
> object.

When you pushstate, WebKit sets a null state object on the previous history entry.
Comment 4 Justin Lebar 2010-06-30 17:29:03 PDT
To note Hixie's response to my e-mail:

> Section 6.5.9.1 [1] says:
>
> > The popstate event is fired when navigating to a session history entry that represents a state object.

Note that this is not a normative statement (there's no "must" or
other RFC2119 term). It's just an informative description.


> In contrast, section 6.5.9 [2] indicates in step 10 that a popstate
> event is fired if the history entry represents a state object or the
> first entry for a document.

Right, that's the actual requirement.

> I think we can resolve this in the spec by changing the line from
> 6.5.9.1 to:
>
> > The popstate event is fired when navigating to a session history
> > entry.

This text does need fixing (I've noted this e-mail and will fix it in due
course), but changing this text makes no difference to the requirments in
the spec.
Comment 5 Brady Eidson 2010-07-08 10:14:56 PDT
<rdar://problem/8171150>
Comment 6 Mihai Parparita 2010-08-24 16:22:15 PDT
Created attachment 65340 [details]
Test case for popstate not being fired

Looks like the 6.5.9.1 in spec was clarified:
http://html5.org/tools/web-apps-tracker?from=5273&to=5274

Justin, it looks like your test case at http://people.mozilla.org/~jlebar/test/webkit/popstate-load.html is no longer there. I've attached a (hopefully) similar test case. The output from Webkit is:

setting hash to #foo
going back
test complete

The output from Firefox 4.0b4:

popstate: null
setting hash to #foo
popstate: null
going back
popstate: null
test complete

It seems to be like Gecko is firing popstate too often here. We only actually do history traversal when going back (to the first entry in the document), so I would expect to only see the final popstate log line.
Comment 7 Justin Lebar 2010-08-24 16:47:03 PDT
(In reply to comment #6)
> Justin, it looks like your test case at http://people.mozilla.org/~jlebar/test/webkit/popstate-load.html is no longer there. I've attached a (hopefully) similar test case.

Sorry; I accidentally nuked my webspace a few weeks ago.  Your testcase looks similar.

> The output from Webkit is:
> 
> setting hash to #foo
> going back
> test complete
> 
> The output from Firefox 4.0b4:
> 
> popstate: null
> setting hash to #foo
> popstate: null
> going back
> popstate: null
> test complete
> 
> It seems to be like Gecko is firing popstate too often here. We only actually do history traversal when going back (to the first entry in the document), so I would expect to only see the final popstate log line.

I don't think "history traversal" means "go to a page you've been to before."  You can traverse the history to a page you've never visited before.

I checked with Hixie about this on #whatwg:

<Hixie> there are exactly two times popstate is fired
<Hixie> 1. during history traversal, if the document readiness is already "complete"
<Hixie> 2. after the browser stops parsing

(1) corresponds to http://www.whatwg.org/specs/web-apps/current-work/#history-traversal
(2) corresponds to http://www.whatwg.org/specs/web-apps/current-work/#the-end

The first popstate happens after we finish parsing the document.  Then when we set the hash to #foo.  This causes us to run algorithm 6.5.8 "Navigating to a fragment identifier" which causes a history traversal and a popstate.  Then we go back, which is also a history traversal.
Comment 8 Mihai Parparita 2010-08-25 13:30:21 PDT
Created attachment 65467 [details]
Test case for popstate not being fired
Comment 9 Mihai Parparita 2010-08-25 14:07:42 PDT
(In reply to comment #7)
> <Hixie> there are exactly two times popstate is fired
> <Hixie> 1. during history traversal, if the document readiness is already "complete"
> <Hixie> 2. after the browser stops parsing
> 
> (1) corresponds to http://www.whatwg.org/specs/web-apps/current-work/#history-traversal
> (2) corresponds to http://www.whatwg.org/specs/web-apps/current-work/#the-end
> 
> The first popstate happens after we finish parsing the document.  Then when we set the hash to #foo.  This causes us to run algorithm 6.5.8 "Navigating to a fragment identifier" which causes a history traversal and a popstate.  Then we go back, which is also a history traversal.

Thanks for the explanation, that makes sense.

What about the case where we navigate to two different fragments, and then go back twice (see updated test case attachment)? Gecko fires popstate for both history.back() calls, but it seems like first time (when going back from #state2 to #state1), the specified entry is not the first in the document, and it doesn't have a state object, so the conditions in step 10 of 6.5.9 (http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#history-traversal) don't apply.
Comment 10 Justin Lebar 2010-08-25 14:17:21 PDT
(In reply to comment #9)
> What about the case where we navigate to two different fragments, and then go back twice (see updated test case attachment)? Gecko fires popstate for both history.back() calls, but it seems like first time (when going back from #state2 to #state1), the specified entry is not the first in the document, and it doesn't have a state object, so the conditions in step 10 of 6.5.9 (http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#history-traversal) don't apply.

Hm...yes, I think you're reading that correctly.  FWIW I think that rule is way too confusing.  We should fire popState whenever the history entry changes, period.  That's much easier for people to understand.  Some pages might have to filter out noisy popstates, but that's better than the alternative.

Could you write to the WhatWG list about this?
Comment 11 Mihai Parparita 2010-08-25 14:56:32 PDT
(In reply to comment #10)
> Hm...yes, I think you're reading that correctly.  FWIW I think that rule is way too confusing.  We should fire popState whenever the history entry changes, period.  That's much easier for people to understand.  Some pages might have to filter out noisy popstates, but that's better than the alternative.

I agree.
 
> Could you write to the WhatWG list about this?

Done: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-August/thread.html#28162
Comment 12 Mihai Parparita 2010-08-25 18:14:03 PDT
Now that I think about it some more, per the spec, I'm not sure Firefox is right to be firing popstate when going from #state1 to #state2 either (in addition to when going back). In that case, #state2 (the new entry) is not the first entry in the document, and doesn't have a state object. Same thing for when going from the initial state to #state1.

However, this all may be moot if Hixie agrees that popstate should fire for all history traversals.

P.S. Thanks for getting Hixie to make this change http://html5.org/tools/web-apps-tracker?from=5345&to=5346
Comment 13 Mihai Parparita 2010-08-26 11:05:01 PDT
Created attachment 65581 [details]
Patch
Comment 14 Mihai Parparita 2010-08-26 11:08:17 PDT
The attached patch modifies WebKit to fire popstate when:
- Navigating to a new document
- Navigating within the document (i.e. when changing the fragment)
- Navigating to a history item (different document (in the page cache or not), same document, or created via replaceState)

This matches Gecko and the proposed HTML5 spec change (that Hixie hasn't weight in on yet).
Comment 15 Mihai Parparita 2010-08-26 11:18:14 PDT
Created attachment 65584 [details]
Patch

Forgot to add new tests
Comment 16 Brady Eidson 2010-08-26 14:37:08 PDT
Comment on attachment 65584 [details]
Patch

> diff --git a/WebCore/dom/Document.cpp b/WebCore/dom/Document.cpp
> index 90939d33bae3c100b19f133cceab418558464ffc..7048c484f63a1eed460487301801656bbcdf430d 100644
> --- a/WebCore/dom/Document.cpp
> +++ b/WebCore/dom/Document.cpp
> @@ -2003,6 +2003,10 @@ void Document::implicitClose()
>      enqueuePageshowEvent(PageshowEventNotPersisted);
>      if (m_pendingStateObject)
>          enqueuePopstateEvent(m_pendingStateObject.release());
> +    else {
> +        RefPtr<SerializedScriptValue> nullStateObject = SerializedScriptValue::create();
> +        enqueuePopstateEvent(nullStateObject.get());
> +    }

and

> diff --git a/WebCore/loader/FrameLoader.cpp b/WebCore/loader/FrameLoader.cpp
> index 24888f27e9ea190672e203ef85630d59a935e208..37ccbdcbe5f91ae02ac44ef71a8a35e71efb2530 100644
> --- a/WebCore/loader/FrameLoader.cpp
> +++ b/WebCore/loader/FrameLoader.cpp
> @@ -1169,10 +1169,13 @@ void FrameLoader::loadInSameDocument(const KURL& url, SerializedScriptValue* sta
>  
>      m_client->dispatchDidNavigateWithinPage();
>  
> -    if (stateObject) {
> +    if (stateObject)
>          m_frame->document()->statePopped(stateObject);
> -        m_client->dispatchDidPopStateWithinPage();
> +    else {
> +        RefPtr<SerializedScriptValue> nullStateObject = SerializedScriptValue::create();
> +        m_frame->document()->statePopped(nullStateObject.get());
>      }
> +    m_client->dispatchDidPopStateWithinPage();

Since we'll be using the null SerializedScriptValue a lot more often now (every history navigation), we may as well make it a static shared object.
Comment 17 Mihai Parparita 2010-08-27 16:54:07 PDT
Created attachment 65788 [details]
Patch
Comment 18 Mihai Parparita 2010-08-27 16:55:04 PDT
(In reply to comment #16)
> Since we'll be using the null SerializedScriptValue a lot more often now (every history navigation), we may as well make it a static shared object.

Sure, that makes sense. Latest version of the patch adds SerializedScriptValue::nullValue() to do this.

Also, the HTML5 spec was changed to reflect Gecko's behavior (and this patch): http://html5.org/tools/web-apps-tracker?from=5376&to=5377
Comment 19 Mihai Parparita 2010-09-01 16:00:47 PDT
Brady, I was wondering if you have any other comments about the patch.
Comment 20 Brady Eidson 2010-09-01 16:15:14 PDT
Comment on attachment 65788 [details]
Patch

Okay looks fine.
Comment 21 WebKit Commit Bot 2010-09-01 18:01:23 PDT
Comment on attachment 65788 [details]
Patch

Clearing flags on attachment: 65788

Committed r66628: <http://trac.webkit.org/changeset/66628>
Comment 22 WebKit Commit Bot 2010-09-01 18:01:29 PDT
All reviewed patches have been landed.  Closing bug.