Bug 41372 - popstate event is not fired until document opts in by calling pushstate.
: popstate event is not fired until document opts in by calling pushstate.
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
: http://people.mozilla.org/~jlebar/tes...
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2010-06-29 14:13 PST by
Modified: 2010-09-01 18:01 PST (History)


Attachments
Test case for popstate not being fired (769 bytes, text/html)
2010-08-24 16:22 PST, Mihai Parparita
no flags Details
Test case for popstate not being fired (922 bytes, text/html)
2010-08-25 13:30 PST, Mihai Parparita
no flags Details
Patch (23.01 KB, patch)
2010-08-26 11:05 PST, Mihai Parparita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (31.03 KB, patch)
2010-08-26 11:18 PST, Mihai Parparita
no flags Review Patch | Details | Formatted Diff | Diff
Patch (34.54 KB, patch)
2010-08-27 16:54 PST, Mihai Parparita
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-06-29 14:13:08 PST
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 From 2010-06-29 14:14:41 PST -------
Forgot to include a link above:

[2]: http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#event-definitions
------- Comment #2 From 2010-06-29 14:17:44 PST -------
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 From 2010-06-30 10:16:00 PST -------
> 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 From 2010-06-30 17:29:03 PST -------
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 From 2010-07-08 10:14:56 PST -------
<rdar://problem/8171150>
------- Comment #6 From 2010-08-24 16:22:15 PST -------
Created an attachment (id=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 From 2010-08-24 16:47:03 PST -------
(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 From 2010-08-25 13:30:21 PST -------
Created an attachment (id=65467) [details]
Test case for popstate not being fired
------- Comment #9 From 2010-08-25 14:07:42 PST -------
(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 From 2010-08-25 14:17:21 PST -------
(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 From 2010-08-25 14:56:32 PST -------
(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 From 2010-08-25 18:14:03 PST -------
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 From 2010-08-26 11:05:01 PST -------
Created an attachment (id=65581) [details]
Patch
------- Comment #14 From 2010-08-26 11:08:17 PST -------
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 From 2010-08-26 11:18:14 PST -------
Created an attachment (id=65584) [details]
Patch

Forgot to add new tests
------- Comment #16 From 2010-08-26 14:37:08 PST -------
(From update of attachment 65584 [details])
> 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 From 2010-08-27 16:54:07 PST -------
Created an attachment (id=65788) [details]
Patch
------- Comment #18 From 2010-08-27 16:55:04 PST -------
(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 From 2010-09-01 16:00:47 PST -------
Brady, I was wondering if you have any other comments about the patch.
------- Comment #20 From 2010-09-01 16:15:14 PST -------
(From update of attachment 65788 [details])
Okay looks fine.
------- Comment #21 From 2010-09-01 18:01:23 PST -------
(From update of attachment 65788 [details])
Clearing flags on attachment: 65788

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