Summary: | popstate event is not fired until document opts in by calling pushstate. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Justin Lebar <justin.lebar+bug> | ||||||||||||
Component: | Page Loading | Assignee: | 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
Justin Lebar
2010-06-29 14:13:08 PDT
Forgot to include a link above: [2]: http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#event-definitions 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. > 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.
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. 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. (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. Created attachment 65467 [details]
Test case for popstate not being fired
(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. (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? (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 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 Created attachment 65581 [details]
Patch
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). Created attachment 65584 [details]
Patch
Forgot to add new tests
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. Created attachment 65788 [details]
Patch
(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 Brady, I was wondering if you have any other comments about the patch. Comment on attachment 65788 [details]
Patch
Okay looks fine.
Comment on attachment 65788 [details] Patch Clearing flags on attachment: 65788 Committed r66628: <http://trac.webkit.org/changeset/66628> All reviewed patches have been landed. Closing bug. |