Closed
Bug 1342472
Opened 8 years ago
Closed 8 years ago
It is no longer possible to use backspace on Mac to delete a site exception (AppConstants is undefined)
Categories
(Firefox :: Settings UI, defect)
Tracking
()
VERIFIED
FIXED
Firefox 54
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
jaws
:
review+
gchang
:
approval-mozilla-aurora+
|
Details |
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 | ||
Updated•8 years ago
|
Assignee: nobody → standard8
Updated•8 years ago
|
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 hidden (mozreview-request) |
Assignee | ||
Comment 2•8 years ago
|
||
mozreview-review |
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 3•8 years ago
|
||
mozreview-review |
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+
Comment 4•8 years ago
|
||
Mark, can you add a test for this or should we make a new bug?
Comment 5•8 years ago
|
||
Gah for *almost* but not quite noticing this in review. :-(
Comment 6•8 years ago
|
||
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.
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Assignee | ||
Comment 7•8 years ago
|
||
(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)
Assignee | ||
Comment 8•8 years ago
|
||
(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.
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
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
Comment 12•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 13•8 years ago
|
||
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)
Updated•8 years ago
|
status-firefox-esr52:
--- → affected
tracking-firefox52:
--- → ?
tracking-firefox-esr52:
--- → ?
Version: unspecified → 51 Branch
Assignee | ||
Comment 14•8 years ago
|
||
(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)
Assignee | ||
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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+
Updated•8 years ago
|
Flags: qe-verify+
Comment 17•8 years ago
|
||
Too late for 52, not sure if it's worth taking in esr52?
Comment 18•8 years ago
|
||
bugherder uplift |
Comment 19•8 years ago
|
||
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+
Comment 20•8 years ago
|
||
(In reply to Hani Yacoub from comment #19)
> Is this the expected behavior in this case?
Flags: needinfo?(standard8)
Assignee | ||
Comment 21•8 years ago
|
||
(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)
Comment 22•8 years ago
|
||
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.
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•