It is no longer possible to use backspace on Mac to delete a site exception (AppConstants is undefined)

VERIFIED FIXED in Firefox 53

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({regression})

51 Branch
Firefox 54
Unspecified
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 wontfix, firefox52- wontfix, firefox-esr52- wontfix, firefox53 verified, firefox54 verified)

Details

Attachments

(1 attachment)

STR (note, permissions.xul is used in a few places).

1) Open preferences
2) Select security
3) Logins -> Exceptions
4) Enter a random website and select block.
5) Focus the newly added website
6) Press Backspace.

Actual Results

-> Nothing happens

Expected Result

-> The item is deleted.

I've identified this as a regression from bug 1294989.
Assignee: nobody → standard8
Summary: It is no longer possible to use backspace on Mac to delete a site permission (AppConstants is undefined) → It is no longer possible to use backspace on Mac to delete a site login exception (AppConstants is undefined)
Comment on attachment 8840964 [details]
Bug 1342472 - It is no longer possible to use backspace on Mac to delete a site permission (AppConstants is undefined).

https://reviewboard.mozilla.org/r/115354/#review116788

::: browser/components/preferences/permissions.js:339
(Diff revision 1)
>        this.onPermissionDeleted();
>      } else if (AppConstants.platform == "macosx" &&
>                 aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE) {
>        this.onPermissionDeleted();
>      }
> +    aEvent.preventDefault();

I've added this as when I fixed the code, pressing backspace would not only delete the item, but also close the sub-window.
Comment on attachment 8840964 [details]
Bug 1342472 - It is no longer possible to use backspace on Mac to delete a site permission (AppConstants is undefined).

https://reviewboard.mozilla.org/r/115354/#review116810

We should have an automated test for this.

::: browser/components/preferences/permissions.js:339
(Diff revision 1)
>        this.onPermissionDeleted();
>      } else if (AppConstants.platform == "macosx" &&
>                 aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE) {
>        this.onPermissionDeleted();
>      }
> +    aEvent.preventDefault();

We should only preventDefault when the keyCode is backspace and an item is selected, right? This looks like it will preventDefault for any keypress regardless of a selection.
Attachment #8840964 - Flags: review?(jaws) → review+
Mark, can you add a test for this or should we make a new bug?
Gah for *almost* but not quite noticing this in review. :-(
I set the flags regrading Marks comment that he identified this as a regression from bug 1294989, but I couldn't reproduce it.

What I noticed is that after I entered multiple addresses to block and then pressed "Backspace" key, all the added websites have been deleted not just the one that I highlighted.
(In reply to Hani Yacoub from comment #6)
> I set the flags regrading Marks comment that he identified this as a
> regression from bug 1294989, but I couldn't reproduce it.

Did you focus the newly selected website as per step 5? Presumably, you're on Mac as well?

> What I noticed is that after I entered multiple addresses to block and then
> pressed "Backspace" key, all the added websites have been deleted not just
> the one that I highlighted.

So that's the browser going "back" and essentially cancelling the changes you've done on the dialog.


Also changing summary, as this applies to any of the "Exceptions" dialog for sites (that are found in preferences).
Summary: It is no longer possible to use backspace on Mac to delete a site login exception (AppConstants is undefined) → It is no longer possible to use backspace on Mac to delete a site exception (AppConstants is undefined)
(In reply to Johann Hofmann [:johannh] from comment #4)
> Mark, can you add a test for this or should we make a new bug?

I've just tried writing a test but I can't get the same effect via the test case. It appears the backspace event doesn't want to bubble up. I'll file a new bug.
Blocks: 1342940
Comment on attachment 8840964 [details]
Bug 1342472 - It is no longer possible to use backspace on Mac to delete a site permission (AppConstants is undefined).

https://reviewboard.mozilla.org/r/115354/#review116810

As per previous comment, this turned out more complicated & took longer than expected - I've separated it out into bug 1342940.

> We should only preventDefault when the keyCode is backspace and an item is selected, right? This looks like it will preventDefault for any keypress regardless of a selection.

Yeah, I did the wrong thing there, I've moved it into the if statement.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18bf0db7a245
It is no longer possible to use backspace on Mac to delete a site permission (AppConstants is undefined). r=jaws
https://hg.mozilla.org/mozilla-central/rev/18bf0db7a245
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Too late for 51 and we've built 52 RC. Mark 51 won't fix.

Hi :standard8,
Do you think this is worth uplifting to Aurora53?
Flags: needinfo?(standard8)
Version: unspecified → 51 Branch
(In reply to Gerry Chang [:gchang] from comment #13)
> Do you think this is worth uplifting to Aurora53?

I'm in two minds - it isn't likely people will use this all the time, but it is a little jarring, so maybe we should as its a simple fix.
Flags: needinfo?(standard8)
Comment on attachment 8840964 [details]
Bug 1342472 - It is no longer possible to use backspace on Mac to delete a site permission (AppConstants is undefined).

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1294989
[User impact if declined]: Attempting to delete a site exception via backspace on Mac won't work correctly.
[Is this code covered by automated tests?]: No (bug 1342940)
[Has the fix been verified in Nightly?]: I've just verified it myself.
[Needs manual test from QE? If yes, steps to reproduce]: See STR
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple fix to correct the code by adding a define and stopping the event propagating too much.
[String changes made/needed]: None
Attachment #8840964 - Flags: approval-mozilla-aurora?
Comment on attachment 8840964 [details]
Bug 1342472 - It is no longer possible to use backspace on Mac to delete a site permission (AppConstants is undefined).

Fix a regression. Aurora53+.
Attachment #8840964 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Too late for 52, not sure if it's worth taking in esr52?
Verified as fixed on Firefox Nightly 54.0a1, 55.0a1 and Firefox Aurora 53.0a2 on Mac OS X 10.11 and on Mac OS X 10.10.

But pressing backspace key doesn't close the sub-window after deleting the highlighted item as mentioned in the second comment.

And on Windows 10 x 64 and Ubuntu 16.04 x 64 pressing the backspace key after focusing the newly selected website will delete all the websites added by me not just the one that I highlighted and close the sub-window.

Is this the expected behavior in this case?
Status: RESOLVED → VERIFIED
Flags: qe-verify+
(In reply to Hani Yacoub from comment #19)
> Is this the expected behavior in this case?
Flags: needinfo?(standard8)
(In reply to Hani Yacoub from comment #19)
> Verified as fixed on Firefox Nightly 54.0a1, 55.0a1 and Firefox Aurora
> 53.0a2 on Mac OS X 10.11 and on Mac OS X 10.10.
> 
> But pressing backspace key doesn't close the sub-window after deleting the
> highlighted item as mentioned in the second comment.

The list would still be focussed, so I would expect this.

> And on Windows 10 x 64 and Ubuntu 16.04 x 64 pressing the backspace key
> after focusing the newly selected website will delete all the websites added
> by me not just the one that I highlighted and close the sub-window.
> 
> Is this the expected behavior in this case?

On Windows/Linux the backspace key is effectively being for going back - i.e. cancelling the dialog. You need to use the delete key to delete a selected website.
Flags: needinfo?(standard8)
Calling this edge-case enough to wontfix for ESR52. Feel free to set the status back to affected and nominate for approval if you feel otherwise.
See Also: → 1365957
Duplicate of this bug: 1172267
You need to log in before you can comment on or make changes to this bug.