Summary: | REGRESSION: Replying "Cancel" to the form repost nag leaves wrong b/f cursor | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Trey Matteson <trey> | ||||||
Component: | New Bugs | Assignee: | Dave Hyatt <hyatt> | ||||||
Status: | RESOLVED FIXED | ||||||||
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 | ||||||||
Attachments: |
|
Description
Trey Matteson
2005-07-08 14:14:02 PDT
Confirmed. Weird stuff :) Created attachment 2915 [details]
This is a tentative patch, not ready for real review, but uploaded for discussion
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?
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 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?
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. 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.) 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 on attachment 2934 [details]
proposed patch
Looks good.
Please update Radar 3939316, which is the same issue. 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. fix committed. |