23060 2008-12-31 14:41:07 -0800 REGRESSION (r38629): Cannot scroll a WebHTMLView using Home/End/Page up/Page down 2009-01-02 07:18:50 -0800 1 1 1 Unclassified WebKit HTML Editing 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED InRadar, Regression P2 Normal --- 1 zwarich zwarich darin oldest_to_newest 103933 0 zwarich 2008-12-31 14:41:07 -0800 After r38629, all keyboard events get sent by Editor to the EditorClient, even if the selection is not editable. If the event's command is unsupported by WebHTMLView, WebHTMLView mistakingly thinks that the event was handled when it was not. When using the page up / page down keys, the events generated are of the form scrollPageUp rather than movePageUp, so they are unsupported by WebHTMLView and cause this bug to occur. 103934 1 zwarich 2008-12-31 14:41:31 -0800 <rdar://problem/6467830> 103935 2 26339 zwarich 2008-12-31 14:45:08 -0800 Created attachment 26339 Proposed patch Here's a patch that fixes the problem. This is probably testable in DRT, but I'd rather get the fix out of the way before deciding how to test it. 103936 3 26339 darin 2008-12-31 14:57:55 -0800 Comment on attachment 26339 Proposed patch Typo "mistakenly" rather than "mistakingly". > - bool eventWasHandled = true; > + bool eventWasHandled = false; This changes the value when the delegate returns YES and when coreFrame is 0. Did you really mean to change it in those cases? Why? > - else { > + else if ([self _canEdit]) { > _private->selectorForDoCommandBySelector = selector; > [super doCommandBySelector:selector]; > _private->selectorForDoCommandBySelector = 0; > + eventWasHandled = true; > } What this really amounts to is: if _canEdit returns false, then set eventWasHandled to false and don't call super's doCommandBySelector So for one thing, you can write it that way: else if (![self_canEdit]) eventWasHandled = false; else ... Without changing the surrounding code, if you like. But for another, I don't understand the rationale here. Maybe we should never treat calling [super doCommandBySelector:] as "eventWasHandled true" -- I don't see what _canEdit has to do with it. 103937 4 zwarich 2008-12-31 15:02:00 -0800 (In reply to comment #3) > (From update of attachment 26339 [review]) > Typo "mistakenly" rather than "mistakingly". > > > - bool eventWasHandled = true; > > + bool eventWasHandled = false; > > This changes the value when the delegate returns YES and when coreFrame is 0. > Did you really mean to change it in those cases? Why? > > > - else { > > + else if ([self _canEdit]) { > > _private->selectorForDoCommandBySelector = selector; > > [super doCommandBySelector:selector]; > > _private->selectorForDoCommandBySelector = 0; > > + eventWasHandled = true; > > } > > What this really amounts to is: > > if _canEdit returns false, then set eventWasHandled to false and don't call > super's doCommandBySelector > > So for one thing, you can write it that way: > > else if (![self_canEdit]) > eventWasHandled = false; > else > ... > > Without changing the surrounding code, if you like. Yeah, I accidentally changed the delegate case. I originally had it like you said, but thought it would be better to make the default false rather than true. I should never doubt my instincts. ;-) > But for another, I don't understand the rationale here. Maybe we should never > treat calling [super doCommandBySelector:] as "eventWasHandled true" -- I don't > see what _canEdit has to do with it. I would have thought about doing that, but I couldn't answer this question: when does [super doCommandBySelector:] ever handle an event? Presumedly there is some case if the code was added long ago. 103938 5 26339 zwarich 2008-12-31 15:02:23 -0800 Comment on attachment 26339 Proposed patch Clearing review flag pending a new patch. 103939 6 darin 2008-12-31 15:06:01 -0800 (In reply to comment #4) > I would have thought about doing that, but I couldn't answer this question: > when does [super doCommandBySelector:] ever handle an event? Presumedly there > is some case if the code was added long ago. I think the case has something to do with input methods, but I really don't know. 103940 7 26340 zwarich 2008-12-31 15:16:59 -0800 Created attachment 26340 Proposed patch Here is the revised patch. I don't want to change the default in the editable case if you don't know exactly why the message is passed to super. Also, 'mistakingly' is valid English, just a bit out of use. I changed it though. ;-) The same comment stands about the test. 104003 8 26340 darin 2009-01-01 17:39:58 -0800 Comment on attachment 26340 Proposed patch r=me I think this would be slightly better if it had a comment explaining what's going on. 104081 9 zwarich 2009-01-02 07:18:50 -0800 Fixed in r39549, with a comment added like Darin suggested. 26339 2008-12-31 14:45:08 -0800 2008-12-31 15:16:59 -0800 Proposed patch pagedown.diff text/plain 2369 zwarich SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDM5NTMy KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjQgQEAKKzIwMDgtMTIt MzEgIENhbWVyb24gWndhcmljaCAgPGN3endhcmljaEB1d2F0ZXJsb28uY2E+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQnVnIDIzMDYwOiBSRUdSRVNT SU9OIChyMzg2MjkpOiBDYW5ub3Qgc2Nyb2xsIGEgV2ViSFRNTFZpZXcgdXNpbmcgSG9tZS9FbmQv UGFnZSB1cC9QYWdlIGRvd24KKyAgICAgICAgPGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0yMzA2MD4KKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzY0Njc4MzA+CisKKyAg ICAgICAgQWZ0ZXIgcjM4NjI5LCBhbGwga2V5Ym9hcmQgZXZlbnRzIGdldCBzZW50IGJ5IEVkaXRv ciB0byB0aGUgRWRpdG9yQ2xpZW50LCBldmVuCisgICAgICAgIGlmIHRoZSBzZWxlY3Rpb24gaXMg bm90IGVkaXRhYmxlLiBJZiB0aGUgZXZlbnQncyBjb21tYW5kIGlzIHVuc3VwcG9ydGVkIGJ5Cisg ICAgICAgIFdlYkhUTUxWaWV3LCBXZWJIVE1MVmlldyBtaXN0YWtpbmdseSB0aGlua3MgdGhhdCB0 aGUgZXZlbnQgd2FzIGhhbmRsZWQgd2hlbiBpdAorICAgICAgICB3YXMgbm90LiBXaGVuIHVzaW5n IHRoZSBwYWdlIHVwIC8gcGFnZSBkb3duIGtleXMsIHRoZSBldmVudHMgZ2VuZXJhdGVkIGFyZSBv ZgorICAgICAgICB0aGUgZm9ybSBzY3JvbGxQYWdlVXAgcmF0aGVyIHRoYW4gbW92ZVBhZ2VVcCwg c28gdGhleSBhcmUgdW5zdXBwb3J0ZWQgYnkKKyAgICAgICAgV2ViSFRNTFZpZXcgYW5kIGNhdXNl IHRoaXMgYnVnIHRvIG9jY3VyLgorCisgICAgICAgIFRoZSBmaXggaXMgdG8gY2hhbmdlIHRoZSBk ZWZhdWx0IG9mIGV2ZW50V2FzSGFuZGxlZCB0byBmYWxzZSBpbiBkb0NvbW1hbmRCeVNlbGVjdG9y CisgICAgICAgIHdoZW4gZWRpdGluZyBpcyBkaXNhYmxlZC4KKworICAgICAgICAqIFdlYlZpZXcv V2ViSFRNTFZpZXcubW06CisgICAgICAgICgtW1dlYkhUTUxWaWV3IGRvQ29tbWFuZEJ5U2VsZWN0 b3I6XSk6CisKIDIwMDgtMTItMjYgIERhbiBCZXJuc3RlaW4gIDxtaXR6QGFwcGxlLmNvbT4KIAog ICAgICAgICBSZXZpZXdlZCBieSBTYW0gV2VpbmlnLgpJbmRleDogV2ViVmlldy9XZWJIVE1MVmll dy5tbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBXZWJWaWV3L1dlYkhUTUxWaWV3Lm1tCShyZXZpc2lvbiAzOTUy NSkKKysrIFdlYlZpZXcvV2ViSFRNTFZpZXcubW0JKHdvcmtpbmcgY29weSkKQEAgLTUxODcsNyAr NTE4Nyw3IEBAIHN0YXRpYyB2b2lkIGV4dHJhY3RVbmRlcmxpbmVzKE5TQXR0cmlidXQKICAgICAg ICAgLy8gTWFrZSBzdXJlIHRoYXQgb25seSBkaXJlY3QgY2FsbHMgdG8gZG9Db21tYW5kQnlTZWxl Y3Rvcjogc2VlIHRoZSBwYXJhbWV0ZXJzIGJ5IHNldHRpbmcgdG8gMC4KICAgICAgICAgX3ByaXZh dGUtPmludGVycHJldEtleUV2ZW50c1BhcmFtZXRlcnMgPSAwOwogCi0gICAgICAgIGJvb2wgZXZl bnRXYXNIYW5kbGVkID0gdHJ1ZTsKKyAgICAgICAgYm9vbCBldmVudFdhc0hhbmRsZWQgPSBmYWxz ZTsKIAogICAgICAgICBXZWJWaWV3ICp3ZWJWaWV3ID0gW3NlbGYgX3dlYlZpZXddOwogICAgICAg ICBGcmFtZSogY29yZUZyYW1lID0gY29yZShbc2VsZiBfZnJhbWVdKTsKQEAgLTUxOTUsMTAgKzUx OTUsMTEgQEAgc3RhdGljIHZvaWQgZXh0cmFjdFVuZGVybGluZXMoTlNBdHRyaWJ1dAogICAgICAg ICAgICAgRWRpdG9yOjpDb21tYW5kIGNvbW1hbmQgPSBbc2VsZiBjb3JlQ29tbWFuZEJ5U2VsZWN0 b3I6c2VsZWN0b3JdOwogICAgICAgICAgICAgaWYgKGNvbW1hbmQuaXNTdXBwb3J0ZWQoKSkKICAg ICAgICAgICAgICAgICBldmVudFdhc0hhbmRsZWQgPSBjb21tYW5kLmV4ZWN1dGUoZXZlbnQpOwot ICAgICAgICAgICAgZWxzZSB7CisgICAgICAgICAgICBlbHNlIGlmIChbc2VsZiBfY2FuRWRpdF0p IHsKICAgICAgICAgICAgICAgICBfcHJpdmF0ZS0+c2VsZWN0b3JGb3JEb0NvbW1hbmRCeVNlbGVj dG9yID0gc2VsZWN0b3I7CiAgICAgICAgICAgICAgICAgW3N1cGVyIGRvQ29tbWFuZEJ5U2VsZWN0 b3I6c2VsZWN0b3JdOwogICAgICAgICAgICAgICAgIF9wcml2YXRlLT5zZWxlY3RvckZvckRvQ29t bWFuZEJ5U2VsZWN0b3IgPSAwOworICAgICAgICAgICAgICAgIGV2ZW50V2FzSGFuZGxlZCA9IHRy dWU7CiAgICAgICAgICAgICB9CiAgICAgICAgIH0KIAo= 26340 2008-12-31 15:16:59 -0800 2009-01-01 17:39:58 -0800 Proposed patch pagedown.diff text/plain 2018 zwarich SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDM5NTMy KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMjQgQEAKKzIwMDgtMTIt MzEgIENhbWVyb24gWndhcmljaCAgPGN3endhcmljaEB1d2F0ZXJsb28uY2E+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQnVnIDIzMDYwOiBSRUdSRVNT SU9OIChyMzg2MjkpOiBDYW5ub3Qgc2Nyb2xsIGEgV2ViSFRNTFZpZXcgdXNpbmcgSG9tZS9FbmQv UGFnZSB1cC9QYWdlIGRvd24KKyAgICAgICAgPGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0yMzA2MD4KKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzY0Njc4MzA+CisKKyAg ICAgICAgQWZ0ZXIgcjM4NjI5LCBhbGwga2V5Ym9hcmQgZXZlbnRzIGdldCBzZW50IGJ5IEVkaXRv ciB0byB0aGUgRWRpdG9yQ2xpZW50LCBldmVuCisgICAgICAgIGlmIHRoZSBzZWxlY3Rpb24gaXMg bm90IGVkaXRhYmxlLiBJZiB0aGUgZXZlbnQncyBjb21tYW5kIGlzIHVuc3VwcG9ydGVkIGJ5Cisg ICAgICAgIFdlYkhUTUxWaWV3LCBXZWJIVE1MVmlldyBtaXN0YWtlbmx5IHRoaW5rcyB0aGF0IHRo ZSBldmVudCB3YXMgaGFuZGxlZCB3aGVuIGl0CisgICAgICAgIHdhcyBub3QuIFdoZW4gdXNpbmcg dGhlIHBhZ2UgdXAgLyBwYWdlIGRvd24ga2V5cywgdGhlIGV2ZW50cyBnZW5lcmF0ZWQgYXJlIG9m CisgICAgICAgIHRoZSBmb3JtIHNjcm9sbFBhZ2VVcCByYXRoZXIgdGhhbiBtb3ZlUGFnZVVwLCBz byB0aGV5IGFyZSB1bnN1cHBvcnRlZCBieQorICAgICAgICBXZWJIVE1MVmlldyBhbmQgY2F1c2Ug dGhpcyBidWcgdG8gb2NjdXIuCisKKyAgICAgICAgVGhlIGZpeCBpcyB0byBjaGFuZ2UgdGhlIGRl ZmF1bHQgb2YgZXZlbnRXYXNIYW5kbGVkIHRvIGZhbHNlIGluIGRvQ29tbWFuZEJ5U2VsZWN0b3IK KyAgICAgICAgd2hlbiBlZGl0aW5nIGlzIGRpc2FibGVkLgorCisgICAgICAgICogV2ViVmlldy9X ZWJIVE1MVmlldy5tbToKKyAgICAgICAgKC1bV2ViSFRNTFZpZXcgZG9Db21tYW5kQnlTZWxlY3Rv cjpdKToKKwogMjAwOC0xMi0yNiAgRGFuIEJlcm5zdGVpbiAgPG1pdHpAYXBwbGUuY29tPgogCiAg ICAgICAgIFJldmlld2VkIGJ5IFNhbSBXZWluaWcuCkluZGV4OiBXZWJWaWV3L1dlYkhUTUxWaWV3 Lm1tCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFdlYlZpZXcvV2ViSFRNTFZpZXcubW0JKHJldmlzaW9uIDM5NTI1 KQorKysgV2ViVmlldy9XZWJIVE1MVmlldy5tbQkod29ya2luZyBjb3B5KQpAQCAtNTE5NSwxMSAr NTE5NSwxMiBAQCBzdGF0aWMgdm9pZCBleHRyYWN0VW5kZXJsaW5lcyhOU0F0dHJpYnV0CiAgICAg ICAgICAgICBFZGl0b3I6OkNvbW1hbmQgY29tbWFuZCA9IFtzZWxmIGNvcmVDb21tYW5kQnlTZWxl Y3RvcjpzZWxlY3Rvcl07CiAgICAgICAgICAgICBpZiAoY29tbWFuZC5pc1N1cHBvcnRlZCgpKQog ICAgICAgICAgICAgICAgIGV2ZW50V2FzSGFuZGxlZCA9IGNvbW1hbmQuZXhlY3V0ZShldmVudCk7 Ci0gICAgICAgICAgICBlbHNlIHsKKyAgICAgICAgICAgIGVsc2UgaWYgKFtzZWxmIF9jYW5FZGl0 XSkgewogICAgICAgICAgICAgICAgIF9wcml2YXRlLT5zZWxlY3RvckZvckRvQ29tbWFuZEJ5U2Vs ZWN0b3IgPSBzZWxlY3RvcjsKICAgICAgICAgICAgICAgICBbc3VwZXIgZG9Db21tYW5kQnlTZWxl Y3RvcjpzZWxlY3Rvcl07CiAgICAgICAgICAgICAgICAgX3ByaXZhdGUtPnNlbGVjdG9yRm9yRG9D b21tYW5kQnlTZWxlY3RvciA9IDA7Ci0gICAgICAgICAgICB9CisgICAgICAgICAgICB9IGVsc2UK KyAgICAgICAgICAgICAgICBldmVudFdhc0hhbmRsZWQgPSBmYWxzZTsKICAgICAgICAgfQogCiAg ICAgICAgIGlmIChwYXJhbWV0ZXJzKQo=