Summary: | REGRESSION: Back button after form submission to the same URL fails to navigate | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Fisher (:fishd, Google) <fishd> | ||||||
Component: | Page Loading | Assignee: | Darin Fisher (:fishd, Google) <fishd> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | ||||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
URL: | http://www.uni-nets.com/chrometests/brokenforms.php | ||||||||
Attachments: |
|
Description
Darin Fisher (:fishd, Google)
2009-04-15 16:29:28 PDT
Here's the corresponding Chromium bug report: http://code.google.com/p/chromium/issues/detail?id=8976 Chrome 1.0 and Safari 3.2 do not have this bug. Created attachment 29537 [details]
v1 patch
The fix is a very simple change to FrameLoader. I verified that this does not regress any existing layout tests.
However, in order to write a test for this, I discovered a bug in the WorkQueue implementation. This issue is detailed in the WebKitTools/ChangeLog portion of my patch.
Created attachment 29538 [details]
v2 patch: same as before, but with equivalent changes to win/FrameLoadDelegate.cpp
Comment on attachment 29538 [details] v2 patch: same as before, but with equivalent changes to win/FrameLoadDelegate.cpp > // We also do not do anchor-style navigation if we're posting a form. > > #if ENABLE(WML) > - if (!formData && urlsMatchItem(item) && !m_frame->document()->isWMLDocument()) { > + if (!formData && !m_currentHistoryItem->formData() && urlsMatchItem(item) && !m_frame->document()->isWMLDocument()) { > #else > - if (!formData && urlsMatchItem(item)) { > + if (!formData && !m_currentHistoryItem->formData() && urlsMatchItem(item)) { > #endif > // Must do this maintenance here, since we don't go through a real page reload > saveScrollPositionAndViewStateToItem(m_currentHistoryItem.get()); It seems that if you're changing the code you also should change the comment before it, which describes the condition in a high-level way. Not your fault, and maybe best to not do this in this patch, but that WMLDocument check is done in a particuarly ugly way. Maybe we could just set up a boolean before the if statement instead of repeating the whole if statement twice. The extra open brace inside an #if also tends to confuse syntax highlighters. > + // if another load started, then wait for it to complete. > + if (topLoadingFrame != nil) > + return; Normally we wouldn't say != nil here. r=me > It seems that if you're changing the code you also should change the comment > before it, which describes the condition in a high-level way. Yes, will do. > Not your fault, and maybe best to not do this in this patch, but that > WMLDocument check is done in a particuarly ugly way. Maybe we could just set up > a boolean before the if statement instead of repeating the whole if statement > twice. The extra open brace inside an #if also tends to confuse syntax > highlighters. I agree. I spent a little time twiddling with this before uploading the patch, but I wasn't happy with the results. This current form is surprisingly readable since the variant is last. That said, duplicating code sucks. I'll change it. > > + // if another load started, then wait for it to complete. > > + if (topLoadingFrame != nil) > > + return; > > Normally we wouldn't say != nil here. Good to know. I'm not very familiar with ObjC coding conventions. Landed as: http://trac.webkit.org/changeset/42623 |