Bug 3910

Summary: REGRESSION: Replying "Cancel" to the form repost nag leaves wrong b/f cursor
Product: WebKit Reporter: Trey Matteson <trey>
Component: New BugsAssignee: Dave Hyatt <hyatt>
Severity: Normal CC: sullivan
Priority: P2    
Version: 412   
Hardware: Mac   
OS: OS X 10.4   
URL: https://bugreport.apple.com/cgi-bin/WebObjects/RadarWeb.woa
Description Flags
This is a tentative patch, not ready for real review, but uploaded for discussion
proposed patch sullivan: review+

Description Trey Matteson 2005-07-08 14:14:02 PDT
If you get the "Do you really want to repost this form" nag sheet and reply by hitting Cancel, the cursor 
back/forward is set as if you accepted the navigation, even though you're still looking at the original 

Example:  Goto https://bugreport.apple.com/cgi-bin/WebObjects/RadarWeb.woa.  Login.

You're now at the Home page.  Hit "My originated bugs" at the top.

Go back.  Get the form repost nag.  Hit Cancel.

You are still viewing the originated bugs page, but if you click and hold the back and forward buttons, 
you'll see that Safari thinks you're back at the Home page.  One bad result is that now clicking back will 
move you back relative to this incorrect location.  Reload will also do the wrong thing.

I'm 99% positive that this bug didn't exist when the repost nag was first implemented, although I have 
not verified this in a live earlier version.
Comment 1 Joost de Valk (AlthA) 2005-07-08 14:32:07 PDT
Confirmed. Weird stuff :)
Comment 2 Trey Matteson 2005-07-11 22:36:32 PDT
Created attachment 2915 [details]
This is a tentative patch, not ready for real review, but uploaded for discussion
Comment 3 John Sullivan 2005-07-12 08:48:53 PDT
Comment on attachment 2915 [details]
This is a tentative patch, not ready for real review, but uploaded for discussion

Hi Trey. The line that declares *item can be moved into the if clause, unless
you left it outside because you were planning to do more here.

Can you explain why you check two conditions ([item isTargetItem] || [[self
webView] mainFrame] == self)? Which of these conditions is true in the form
repost cancel case? Why do you need the other?
Comment 4 Trey Matteson 2005-07-12 09:25:33 PDT
Agreed re: moving the *item declaration.

I believe [item isTargetItem] is true for all practical cases of the repost nag.  If the page has no frames, 
the second expression will always be true.  I added the second expression without a specific test case 
because I felt like if the loading of the main frame was ever stopped by the delegate, you're definitely 
not going to end up where the user was going, so might as well reset the b/f cursor.

On the other hand, let's say that on a multi frame page you went back, and the target frame was not the 
main frame, and in going back the target frame and some other frame were reloaded.  If the target 
frame succeeds but the other frame is stopped by the delegate (leaving a pretty goofy, mixed situation), 
we'll let the b/f cursor move stand, since at least the target, and presumably logically primary, frame 
did advance properly.

Looking at the code in _recursiveGoToItem:, which decides which frames reload and which are left 
alone, I think both of the oddball cases I describe above are very unlikely.  Perhaps one way would be a 
sick page that dynamically changes its frame names or frame tree structure (not just geometry, but 
adding and removing frames).  Overall I think the logic I propose gives adequate coverage to these 
corner cases.
Comment 5 John Sullivan 2005-07-12 09:47:44 PDT
Comment on attachment 2915 [details]
This is a tentative patch, not ready for real review, but uploaded for discussion

OK, sounds convincing. Why don't you think the patch is ready for "real review'
Comment 6 Trey Matteson 2005-07-12 14:39:48 PDT
Actually I discovered that I can't move that variable decl after all, since it was actually moved from a later 
place in the file.  My tentative patch didn't reflect this, but it is right in the real patch I have nominated.
Comment 7 John Sullivan 2005-07-12 14:48:54 PDT
Trey, did you forget to attach the new patch? I only see the old one. (And when you add a new one, you 
should mark the old one obsolete.)
Comment 8 Trey Matteson 2005-07-12 15:54:16 PDT
Created attachment 2934 [details]
proposed patch

Here is the real patch.  The only difference from before is that we are careful
to check the loadType in the frame where the navigation was refused, which can
be different than the loadType in the mainFrame, especially since when the nav
is refused we haven't even reached provisional state yet.
Comment 9 John Sullivan 2005-07-12 16:04:47 PDT
Comment on attachment 2934 [details]
proposed patch

Looks good.
Comment 10 Trey Matteson 2005-07-13 11:07:03 PDT
Please update Radar 3939316, which is the same issue.
Comment 11 John Sullivan 2005-07-13 11:37:33 PDT
I added a comment to the radar bug mentioning this bug number. When this is committed, the committer 
should also move that bug to integrate.
Comment 12 Adele Peterson 2005-07-15 23:07:24 PDT
fix committed.