Closed Bug 1604970 Opened 4 years ago Closed 4 years ago

Delete key (del key) is broken when permissions.default.shortcuts = 2 (patch included)

Categories

(Firefox :: Keyboard Navigation, defect, P3)

71 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 74
Tracking Status
firefox74 --- fixed

People

(Reporter: chrishen, Assigned: chrishen)

References

Details

(Keywords: testcase)

Attachments

(3 files)

Attached file mini-input.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

  1. Set permissions.default.shortcuts = 2
  2. Type some text into a text input field on any webpage (the attached file mini-input.html provides a minimal test case).
  3. Move the text cursor so that one or more characters are to the right of it.
  4. Press the "Delete" key on your keyboard.

Steps to fix:

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

  1. 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"/>

  1. (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.

NI because bug 1445942, comment 15 suggests you intended to fix this.

Has STR: --- → yes
Component: Untriaged → DOM: UI Events & Focus Handling
Flags: needinfo?(enndeakin)
Keywords: testcase
Product: Firefox → Core
See Also: → 1445942

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.

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

Assignee: nobody → chrishen
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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

Flags: needinfo?(enndeakin)
Component: DOM: UI Events & Focus Handling → General
Product: Core → Firefox

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

The priority flag is not set for this bug.
:Dolske, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dolske)
Component: General → Keyboard Navigation
Flags: needinfo?(dolske) → needinfo?(gijskruitbosch+bugs)

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

Flags: needinfo?(gijskruitbosch+bugs)

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

Priority: -- → P3
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: