3910 2005-07-08 14:14:02 -0700 REGRESSION: Replying "Cancel" to the form repost nag leaves wrong b/f cursor 2005-07-15 23:07:24 -0700 1 1 1 Unclassified WebKit New Bugs 412 Mac OS X 10.4 RESOLVED FIXED https://bugreport.apple.com/cgi-bin/WebObjects/RadarWeb.woa P2 Normal --- 1 trey hyatt sullivan oldest_to_newest 14235 0 trey 2005-07-08 14:14:02 -0700 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. 14237 1 joost 2005-07-08 14:32:07 -0700 Confirmed. Weird stuff :) 14427 2 2915 trey 2005-07-11 22:36:32 -0700 Created attachment 2915 This is a tentative patch, not ready for real review, but uploaded for discussion 14441 3 2915 sullivan 2005-07-12 08:48:53 -0700 Comment on attachment 2915 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? 14442 4 trey 2005-07-12 09:25:33 -0700 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. 14443 5 2915 sullivan 2005-07-12 09:47:44 -0700 Comment on attachment 2915 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? 14472 6 trey 2005-07-12 14:39:48 -0700 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. 14473 7 sullivan 2005-07-12 14:48:54 -0700 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.) 14475 8 2934 trey 2005-07-12 15:54:16 -0700 Created attachment 2934 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. 14477 9 2934 sullivan 2005-07-12 16:04:47 -0700 Comment on attachment 2934 proposed patch Looks good. 14590 10 trey 2005-07-13 11:07:03 -0700 Please update Radar 3939316, which is the same issue. 14592 11 sullivan 2005-07-13 11:37:33 -0700 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. 14732 12 adele 2005-07-15 23:07:24 -0700 fix committed. 2915 2005-07-11 22:36:32 -0700 2005-07-12 15:54:16 -0700 This is a tentative patch, not ready for real review, but uploaded for discussion 3910.patch text/plain 1793 trey IApJbmRleDogV2ViVmlldy5zdWJwcm9qL1dlYkZyYW1lLm0KPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQpSQ1MgZmlsZTog L2N2cy9yb290L1dlYktpdC9XZWJWaWV3LnN1YnByb2ovV2ViRnJhbWUubSx2CnJldHJpZXZpbmcg cmV2aXNpb24gMS4yMzgKZGlmZiAtdSAtcCAtcjEuMjM4IFdlYkZyYW1lLm0KLS0tIFdlYkZyYW1l Lm0gIDIwMDUvMDcvMTAgMDA6NDU6NTMgICAgIDEuMjM4CisrKyBXZWJGcmFtZS5tICAyMDA1LzA3 LzEyIDA1OjM0OjM0CkBAIC0yMjMyLDYgKzIyMzAsMTMgQEAgc3RhdGljIENGQWJzb2x1dGVUaW1l IF90aW1lT2ZMYXN0Q29tcGxldAogICAgIHJldHVybiBwYXRoOwogfQogCistICh2b2lkKV9yZXNl dEJhY2tGb3J3YXJkTGlzdAoreworICAgIFdlYkhpc3RvcnlJdGVtICpyZXNldEl0ZW0gPSBbc2Vs ZiBfY3VycmVudEJhY2tGb3J3YXJkTGlzdEl0ZW1Ub1Jlc2V0VG9dOworICAgIGlmIChyZXNldEl0 ZW0pCisgICAgICAgIFtbW3NlbGYgd2ViVmlld10gYmFja0ZvcndhcmRMaXN0XSBnb1RvSXRlbTpy ZXNldEl0ZW1dOworfQorCiAvLyBJZiB3ZSBiYWlsZWQgb3V0IG9mIGEgYi9mIG5hdmlnYXRpb24s IHdlIG1pZ2h0IG5lZWQgdG8gc2V0IHRoZSBiL2YgY3Vyc29yIGJhY2sgdG8gdGhlIGN1cnJlbnQK IC8vIGl0ZW0sIGJlY2F1c2Ugd2Ugb3B0aW1pc3RpY2FsbHkgbW92ZSBpdCByaWdodCBhd2F5IGF0 IHRoZSBzdGFydCBvZiB0aGUgb3BlcmF0aW9uLiBCdXQgd2hlbgogLy8gYWx0ZXJuYXRlIGNvbnRl bnQgaXMgbG9hZGVkIGZvciBhbiB1bnJlYWNoYWJsZVVSTCwgd2UgZG9uJ3Qgd2FudCB0byByZXNl dCB0aGUgYi9mIGN1cnNvci4KQEAgLTIzMTksOCArMjMyNCwxNSBAQCBzdGF0aWMgQ0ZBYnNvbHV0 ZVRpbWUgX3RpbWVPZkxhc3RDb21wbGV0CiAgICAgLy8gdGhyb3VnaCB0aGlzIG1ldGhvZCBhbHJl YWR5LCBuZXN0ZWQ7IG90aGVyd2lzZSwgX3ByaXZhdGUtPnBvbGljeURhdGFTb3VyY2Ugc2hvdWxk IHN0aWxsIGJlIHNldC4KICAgICBBU1NFUlQoX3ByaXZhdGUtPnBvbGljeURhdGFTb3VyY2UgfHwg W1tzZWxmIHByb3Zpc2lvbmFsRGF0YVNvdXJjZV0gdW5yZWFjaGFibGVVUkxdICE9IG5pbCk7CiAK KyAgICBXZWJIaXN0b3J5SXRlbSAqaXRlbSA9IFtfcHJpdmF0ZSBwcm92aXNpb25hbEl0ZW1dOwog ICAgIGlmICghcmVxdWVzdCkgewogICAgICAgICBbc2VsZiBfc2V0UG9saWN5RGF0YVNvdXJjZTpu aWxdOworICAgICAgICAvLyBJZiB0aGUgZGVsZWdhdGUgcHVudHMgb24gdGhlIG5hdmlnYXRpb24s IHdlIGhhdmUgdGhlIHByb2JsZW0gdGhhdCB3ZSBoYXZlIG9wdGltaXN0aWNhbGx5IG1vdmVkCisg ICAgICAgIC8vIHRoZSBiL2YgY3Vyc29yIGFscmVhZHksIHNvIG1vdmUgaXQgYmFjay4gIEZvciBz YW5pdHkgd2Ugb25seSBkbyB0aGlzIGlmIHRoZSBuYXZpZ2F0aW9uIG9mIHRoZQorICAgICAgICAv LyB0YXJnZXQgZnJhbWUgb3IgdG9wLWxldmVsIGZyYW1lIGlzIGNhbmNlbGVkLiAgQSBwcmltYXJ5 IGNhc2Ugb2YgdGhpcyBpcyB0aGUgdXNlciByZXNwb25kaW5nIAorICAgICAgICAvLyBDYW5jZWwg dG8gdGhlIGZvcm0gcmVwb3N0IG5hZyBzaGVldC4KKyAgICAgICAgaWYgKFtpdGVtIGlzVGFyZ2V0 SXRlbV0gfHwgW1tzZWxmIHdlYlZpZXddIG1haW5GcmFtZV0gPT0gc2VsZikKKyAgICAgICAgICAg IFtbW3NlbGYgd2ViVmlld10gbWFpbkZyYW1lXSBfcmVzZXRCYWNrRm9yd2FyZExpc3RdOwogICAg ICAgICByZXR1cm47CiAgICAgfQogICAgIAo= 2934 2005-07-12 15:54:16 -0700 2005-07-12 16:04:47 -0700 proposed patch 3910.2.patch text/plain 3656 trey SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09ClJDUyBmaWxlOiAvY3ZzL3Jvb3QvV2ViS2l0L0No YW5nZUxvZyx2CnJldHJpZXZpbmcgcmV2aXNpb24gMS4zMjIyCmRpZmYgLXAgLXUgLXIxLjMyMjIg Q2hhbmdlTG9nCi0tLSBDaGFuZ2VMb2cJMjAwNS8wNy8xMiAxNzo0NDowNwkxLjMyMjIKKysrIENo YW5nZUxvZwkyMDA1LzA3LzEyIDIyOjUyOjM2CkBAIC0xLDMgKzEsMTUgQEAKKzIwMDUtMDctMTIg IFRyZXkgTWF0dGVzb24gIDx0cmV5QHVzYS5uZXQ+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgSm9o biBTdWxsaXZhbi4KKworICAgICAgICBGaXhlZCBodHRwOi8vYnVnemlsbGEub3BlbmRhcndpbi5v cmcvc2hvd19idWcuY2dpP2lkPTM5MTAgLSBSRUdSRVNTSU9OOiBSZXBseWluZyAiQ2FuY2VsIiB0 byB0aGUgZm9ybSByZXBvc3QgbmFnIGxlYXZlcyB3cm9uZyBiL2YgY3Vyc29yCisKKyAgICAgICAg KiBXZWJWaWV3LnN1YnByb2ovV2ViRnJhbWUubToKKyAgICAgICAgKC1bV2ViRnJhbWUgX3Jlc2V0 QmFja0ZvcndhcmRMaXN0XSk6ICBuZXcgaGVscGVyIG1ldGhvZAorICAgICAgICAoLVtXZWJGcmFt ZSBfY29udGludWVMb2FkUmVxdWVzdEFmdGVyTmF2aWdhdGlvblBvbGljeTpmb3JtU3RhdGU6XSk6 CisgICAgICAgICAgSWYgdGhlIGRlbGVnYXRlIGJhaWxlZCBvbiB0aGUgbmF2aWdhdGlvbiwgdGVs bCB0aGUgbWFpbiBmcmFtZQorICAgICAgICAgIHRvIHJlc2V0IHRoZSBiL2YgY3Vyc29yIGJhY2sg dG8gd2hlcmUgaXQgd2FzIGJlZm9yZSB3ZSBzdGFydGVkLgorCiAyMDA1LTA3LTEyICBHZW9mZnJl eSBHYXJlbiAgPGdnYXJlbkBhcHBsZS5jb20+CiAKICAgICAgICAgLXJvbGxlZCBpbiBwYXRjaCBi eSBvcGVuZGFyd2luLm9yZ0BtaXR6cGV0dGVsLmNvbSAKSW5kZXg6IC4vV2ViVmlldy5zdWJwcm9q L1dlYkZyYW1lLm0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQpSQ1MgZmlsZTogL2N2cy9yb290L1dlYktpdC9XZWJWaWV3 LnN1YnByb2ovV2ViRnJhbWUubSx2CnJldHJpZXZpbmcgcmV2aXNpb24gMS4yMzgKZGlmZiAtcCAt dSAtcjEuMjM4IC4vV2ViVmlldy5zdWJwcm9qL1dlYkZyYW1lLm0KLS0tIC4vV2ViVmlldy5zdWJw cm9qL1dlYkZyYW1lLm0JMjAwNS8wNy8xMCAwMDo0NTo1MwkxLjIzOAorKysgLi9XZWJWaWV3LnN1 YnByb2ovV2ViRnJhbWUubQkyMDA1LzA3LzEyIDIyOjUyOjUwCkBAIC0yMjMyLDYgKzIyMzIsMTYg QEAgc3RhdGljIENGQWJzb2x1dGVUaW1lIF90aW1lT2ZMYXN0Q29tcGxldAogICAgIHJldHVybiBw YXRoOwogfQogCistICh2b2lkKV9yZXNldEJhY2tGb3J3YXJkTGlzdAoreworICAgIC8vIE5vdGUg dGhpcyBkb2Vzbid0IHZlcmlmeSB0aGUgY3VycmVudCBsb2FkIHR5cGUgYXMgYSBiL2Ygb3BlcmF0 aW9uIGJlY2F1c2UgaXQgaXMgY2FsbGVkIGZyb20KKyAgICAvLyBhIHN1YmZyYW1lIGluIHRoZSBj YXNlIG9mIGEgZGVsZWdhdGUgYmFpbGluZyBvdXQgb2YgdGhlIG5hdiBiZWZvcmUgaXQgZXZlbiBn ZXRzIHRvIHByb3Zpc2lvbmFsIHN0YXRlLgorICAgIEFTU0VSVChzZWxmID09IFtbc2VsZiB3ZWJW aWV3XSBtYWluRnJhbWVdKTsKKyAgICBXZWJIaXN0b3J5SXRlbSAqcmVzZXRJdGVtID0gW19wcml2 YXRlIGN1cnJlbnRJdGVtXTsKKyAgICBpZiAocmVzZXRJdGVtKQorICAgICAgICBbW1tzZWxmIHdl YlZpZXddIGJhY2tGb3J3YXJkTGlzdF0gZ29Ub0l0ZW06cmVzZXRJdGVtXTsKK30KKwogLy8gSWYg d2UgYmFpbGVkIG91dCBvZiBhIGIvZiBuYXZpZ2F0aW9uLCB3ZSBtaWdodCBuZWVkIHRvIHNldCB0 aGUgYi9mIGN1cnNvciBiYWNrIHRvIHRoZSBjdXJyZW50CiAvLyBpdGVtLCBiZWNhdXNlIHdlIG9w dGltaXN0aWNhbGx5IG1vdmUgaXQgcmlnaHQgYXdheSBhdCB0aGUgc3RhcnQgb2YgdGhlIG9wZXJh dGlvbi4gQnV0IHdoZW4KIC8vIGFsdGVybmF0ZSBjb250ZW50IGlzIGxvYWRlZCBmb3IgYW4gdW5y ZWFjaGFibGVVUkwsIHdlIGRvbid0IHdhbnQgdG8gcmVzZXQgdGhlIGIvZiBjdXJzb3IuCkBAIC0y MzE5LDggKzIzMjksMTggQEAgc3RhdGljIENGQWJzb2x1dGVUaW1lIF90aW1lT2ZMYXN0Q29tcGxl dAogICAgIC8vIHRocm91Z2ggdGhpcyBtZXRob2QgYWxyZWFkeSwgbmVzdGVkOyBvdGhlcndpc2Us IF9wcml2YXRlLT5wb2xpY3lEYXRhU291cmNlIHNob3VsZCBzdGlsbCBiZSBzZXQuCiAgICAgQVNT RVJUKF9wcml2YXRlLT5wb2xpY3lEYXRhU291cmNlIHx8IFtbc2VsZiBwcm92aXNpb25hbERhdGFT b3VyY2VdIHVucmVhY2hhYmxlVVJMXSAhPSBuaWwpOwogCisgICAgV2ViSGlzdG9yeUl0ZW0gKml0 ZW0gPSBbX3ByaXZhdGUgcHJvdmlzaW9uYWxJdGVtXTsKICAgICBpZiAoIXJlcXVlc3QpIHsKICAg ICAgICAgW3NlbGYgX3NldFBvbGljeURhdGFTb3VyY2U6bmlsXTsKKyAgICAgICAgLy8gSWYgdGhl IGRlbGVnYXRlIHB1bnRzIG9uIHRoZSBuYXZpZ2F0aW9uLCB3ZSBoYXZlIHRoZSBwcm9ibGVtIHRo YXQgd2UgaGF2ZSBvcHRpbWlzdGljYWxseSBtb3ZlZAorICAgICAgICAvLyB0aGUgYi9mIGN1cnNv ciBhbHJlYWR5LCBzbyBtb3ZlIGl0IGJhY2suICBGb3Igc2FuaXR5IHdlIG9ubHkgZG8gdGhpcyBp ZiB0aGUgbmF2aWdhdGlvbiBvZiB0aGUKKyAgICAgICAgLy8gdGFyZ2V0IGZyYW1lIG9yIHRvcC1s ZXZlbCBmcmFtZSBpcyBjYW5jZWxlZC4gIEEgcHJpbWFyeSBjYXNlIG9mIHRoaXMgaXMgdGhlIHVz ZXIgcmVzcG9uZGluZyAKKyAgICAgICAgLy8gQ2FuY2VsIHRvIHRoZSBmb3JtIHJlcG9zdCBuYWcg c2hlZXQuCisgICAgICAgIGlmICgoW2l0ZW0gaXNUYXJnZXRJdGVtXSB8fCBbW3NlbGYgd2ViVmll d10gbWFpbkZyYW1lXSA9PSBzZWxmKQorICAgICAgICAgICAgJiYgKF9wcml2YXRlLT5wb2xpY3lM b2FkVHlwZSA9PSBXZWJGcmFtZUxvYWRUeXBlRm9yd2FyZAorICAgICAgICAgICAgICAgIHx8IF9w cml2YXRlLT5wb2xpY3lMb2FkVHlwZSA9PSBXZWJGcmFtZUxvYWRUeXBlQmFjaworICAgICAgICAg ICAgICAgIHx8IF9wcml2YXRlLT5wb2xpY3lMb2FkVHlwZSA9PSBXZWJGcmFtZUxvYWRUeXBlSW5k ZXhlZEJhY2tGb3J3YXJkKSkKKyAgICAgICAgICAgIFtbW3NlbGYgd2ViVmlld10gbWFpbkZyYW1l XSBfcmVzZXRCYWNrRm9yd2FyZExpc3RdOwogICAgICAgICByZXR1cm47CiAgICAgfQogICAgIApA QCAtMjM0Miw3ICsyMzYyLDYgQEAgc3RhdGljIENGQWJzb2x1dGVUaW1lIF90aW1lT2ZMYXN0Q29t cGxldAogICAgIGlmIChzZWxmID09IFtbc2VsZiB3ZWJWaWV3XSBtYWluRnJhbWVdKQogICAgICAg ICBMT0coRG9jdW1lbnRMb2FkLCAibG9hZGluZyAlQCIsIFtbW3NlbGYgcHJvdmlzaW9uYWxEYXRh U291cmNlXSByZXF1ZXN0XSBVUkxdKTsKIAotICAgIFdlYkhpc3RvcnlJdGVtICppdGVtID0gW19w cml2YXRlIHByb3Zpc2lvbmFsSXRlbV07CiAgICAgaWYgKChsb2FkVHlwZSA9PSBXZWJGcmFtZUxv YWRUeXBlRm9yd2FyZCB8fAogICAgICAgICBsb2FkVHlwZSA9PSBXZWJGcmFtZUxvYWRUeXBlQmFj ayB8fAogICAgICAgICBsb2FkVHlwZSA9PSBXZWJGcmFtZUxvYWRUeXBlSW5kZXhlZEJhY2tGb3J3 YXJkKSAmJgo=