Bug 3910 – REGRESSION: Replying "Cancel" to the form repost nag leaves wrong b/f cursor

• Component: New Bugs
• Status: RESOLVED FIXED

[Trey Matteson]

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

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.

[Joost de Valk (AlthA)]

Confirmed. Weird stuff :)

[Trey Matteson]

Created attachment 2915 [details]
This is a tentative patch, not ready for real review, but uploaded for discussion

[John Sullivan]

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?

[Trey Matteson]

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.

[John Sullivan]

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'
yet?

[Trey Matteson]

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.

[John Sullivan]

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

[Trey Matteson]

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.

[John Sullivan]

Comment on attachment 2934 [details]
proposed patch

Looks good.

[Trey Matteson]

Please update Radar 3939316, which is the same issue.

[John Sullivan]

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.

[Adele Peterson]

fix committed.