Delete key (del key) is broken when permissions.default.shortcuts = 2 (patch included)
Categories
(Firefox :: Keyboard Navigation, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox74 | --- | fixed |
People
(Reporter: chrishen, Assigned: chrishen)
References
Details
(Keywords: testcase)
Attachments
(3 files)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0
Steps to reproduce:
- Set permissions.default.shortcuts = 2
- Type some text into a text input field on any webpage (the attached file mini-input.html provides a minimal test case).
- Move the text cursor so that one or more characters are to the right of it.
- Press the "Delete" key on your keyboard.
Steps to fix:
- The version of Firefox currently being distributed as part of Ubuntu 18.04 Bionic Beaver (71.0+build5-0ubuntu0.18.04.1, from https://packages.ubuntu.com/source/bionic/firefox) is fixed by the following one-line patch:
diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -180,7 +180,7 @@
<key id="key_paste"
key="&pasteCmd.key;"
modifiers="accel"/>
- <key id="key_delete" keycode="VK_DELETE" command="cmd_delete"/>
-
<key id="key_delete" keycode="VK_DELETE" command="cmd_delete" reserved="false"/>
<key id="key_selectAll" key="&selectAllCmd.key;" modifiers="accel"/><key keycode="VK_BACK" command="cmd_handleBackspace" reserved="false"/>
I am currently typing this bug report up in a browser built from that source package, patched as above, and the Delete key is now working correctly with permissions.default.shortcuts = 2 on.
- I haven't built from Trunk in order to test whether this bug is still present there, but given the lack of fixed bug reports indicating that it's not, I suspect that it is. If so, the analogous patch to the one above (differing only in the line number of the change) would be:
diff --git a/browser/base/content/browser-sets.inc b/browser/base/content/browser-sets.inc
--- a/browser/base/content/browser-sets.inc
+++ b/browser/base/content/browser-sets.inc
@@ -188,7 +188,7 @@
<key id="key_paste"
key="&pasteCmd.key;"
modifiers="accel"/>
- <key id="key_delete" keycode="VK_DELETE" command="cmd_delete"/>
-
<key id="key_delete" keycode="VK_DELETE" command="cmd_delete" reserved="false"/>
<key id="key_selectAll" key="&selectAllCmd.key;" modifiers="accel"/><key keycode="VK_BACK" command="cmd_handleBackspace" reserved="false"/>
- (Optional) Ideally, someone who knows a lot more than I do about Firefox internals should examine why normal, basic functioning of the "Delete" key in a text input field is treated as a website "overriding" a keyboard shortcut (a shortcut to do nothing?).
Note that in mini-input.html, there is absolutely no javascript or other active code. Indeed, there isn't even a form specified for the input field! Intuitively, at least, nothing can be being overriden (even when permissions.default.shortcuts = 0) , because there's nothing that could be overriding it. But the "Delete" key still works when permissions.default.shortcuts = 0, or when the above patch has been applied, but not when permissions.default.shortcuts = 2 and the above patch has not been applied.
Actual results:
Nothing happens; the text in the focused input field remains unaltered.
This is identical to the "del key" half of the bug "user pref 'permissions.default.shortcuts' screws up backspace and del keys" (https://bugzilla.mozilla.org/show_bug.cgi?id=1445942), which was marked CLOSED FIXED after the application of a patch that (only) fixed the behavior of the Backspace key.
Expected results:
The character to the right of the text cursor should be deleted.
Comment 1•4 years ago
|
||
NI because bug 1445942, comment 15 suggests you intended to fix this.
Assignee | ||
Comment 2•4 years ago
|
||
Here's the Firefox 71.0.1 (fetched from https://github.com/mozilla/gecko-dev/tree/release) version of the patch as an attachment (and flagged as a patch, and with the index line left in).
(Hopefully it's formatted correctly; https://developer.mozilla.org/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch seems to imply that additional metadata is needed as comments at the start of the file; https://wiki.mozilla.org/Bugzilla:Patches seems to imply that it is not.)
Is there a way to edit the original bug post to remove the inline (and markdown-mutilated) patches from it? I can't seem to find an edit button, and they're a bit redundant now.
Comment 3•4 years ago
|
||
(In reply to Chris Henry from comment #2)
patch as an attachment
Thank you, but I'm not sure what anyone will make of this, since patches are submitted and reviewed through Phabricator and there's no way to request a review on Bugzilla anymore.
(Hopefully it's formatted correctly; https://developer.mozilla.org/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch seems to imply that additional metadata is needed as comments at the start of the file; https://wiki.mozilla.org/Bugzilla:Patches seems to imply that it is not.)
Bear in mind the first page was last updated November 14th 2019, while the second was last updated March 6th 2014.
Is there a way to edit the original bug post
The Edit button is the leftmost in the row at the top right of each comment, though it may only available if you have editbugs privilege. I could flag someone who is able to edit all comments, but it doesn't seem worth the bother for something that's not privacy or security-sensitive.
Comment 4•4 years ago
|
||
This change looks ok, but it would be good to add a test similar to what was added by bug 1445942.
You can submit patches by installing moz-phab and running it from a command line. See https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
Updated•4 years ago
|
Assignee | ||
Comment 5•4 years ago
|
||
(In reply to Neil Deakin from comment #4)
This change looks ok, but it would be good to add a test similar to what was added by bug 1445942.
You can submit patches by installing moz-phab and running it from a command line. See https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html
Thanks for the info. I will look into patch submission using moz-phab when I get the chance---I have a basic familiarity with programming and command line tools in general, but I'm not familiar with Firefox development tools or internals. I just encountered a bug, couldn't find an answer on stackexchange, went to submit a bug report here, saw bug 1445942 in the "possibly related" list, and saw that the fix for half of bug 1445942 (the patch there, not including the test) was straightforward enough that I could easily apply it to the other half of the same bug.
I may also look into adding a test, if I can find time to figure out what exactly the test in bug 1445942 is doing and how to modify it; as I said, I'm not familiar with Firefox internals, and I'd rather submit a patch without a test, than submit it with a test that I'm not sure is actually testing everything it should.
Comment 6•4 years ago
|
||
The priority flag is not set for this bug.
:Dolske, could you have a look please?
For more information, please visit auto_nag documentation.
Updated•4 years ago
|
Comment 7•4 years ago
|
||
Comment 8•4 years ago
|
||
Chris, I've submitted the patch you've provided via phabricator, added a test that exercises the delete key in addition to the backspace key, and credited you as the author (though let me know if you would prefer if that did not happen!). I hope the patch helps with some of the questions you listed in comment #5 and that you'll consider contributing more to Firefox. :-)
Assignee | ||
Comment 9•4 years ago
|
||
(In reply to :Gijs (he/him) from comment #8)
Chris, I've submitted the patch you've provided via phabricator, added a test that exercises the delete key in addition to the backspace key, and credited you as the author (though let me know if you would prefer if that did not happen!). I hope the patch helps with some of the questions you listed in comment #5 and that you'll consider contributing more to Firefox. :-)
Thanks a lot. I'm perfectly happy to be credited as author; thanks both for that and for checking.
Updated•4 years ago
|
Comment 10•4 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/27ae4b0e5fad make delete key work correctly even when shortcut permissions are restricted, r=NeilDeakin
Comment 11•4 years ago
|
||
bugherder |
Description
•