Bug 41372 – popstate event is not fired until document opts in by calling pushstate.

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Justin Lebar]

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

[Justin Lebar]

Forgot to include a link above:

[2]: http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#event-definitions

[Justin Lebar]

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.

[Darin Fisher (:fishd, Google)]

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

[Justin Lebar]

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.

[Brady Eidson]

<rdar://problem/8171150>

[Mihai Parparita]

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.

[Justin Lebar]

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

[Mihai Parparita]

Created attachment 65467 [details]
Test case for popstate not being fired

[Mihai Parparita]

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

[Justin Lebar]

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

[Mihai Parparita]

(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

[Mihai Parparita]

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

[Mihai Parparita]

Created attachment 65581 [details]
Patch

[Mihai Parparita]

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

[Mihai Parparita]

Created attachment 65584 [details]
Patch

Forgot to add new tests

[Brady Eidson]

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.

[Mihai Parparita]

Created attachment 65788 [details]
Patch

[Mihai Parparita]

(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

[Mihai Parparita]

Brady, I was wondering if you have any other comments about the patch.

[Brady Eidson]

Comment on attachment 65788 [details]
Patch

Okay looks fine.

[WebKit Commit Bot]

Comment on attachment 65788 [details]
Patch

Clearing flags on attachment: 65788

Committed r66628: <http://trac.webkit.org/changeset/66628>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.