Closed Bug 1342940 Opened 3 years ago Closed 3 years ago

Add a test for using backspace on Mac when site exceptions sub-dialogs are displayed

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: standard8, Assigned: johannh)

References

Details

Attachments

(2 files)

Attached patch WIP testcaseSplinter Review
We need to add a test for bug 1342472.

I attempted this as part of the work for that bug, but I couldn't get the synthesised backspace key to propagate up the DOM tree the same way as when it is pressed manually.

I'm attaching the test case I got so far, I'm happy for anyone to pick it up.
I don't have any issues with the event except on Linux, where this seems to fall through the if clause. Fixed it.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
So it just came to my mind that Linux might use the same keyboard layout as Windows, with a delete key instead of backspace. If that's what you prefer we can also just change the test to trigger that instead.
Comment on attachment 8847117 [details]
Bug 1342940 - Add a test for login exception dialog interaction in about:preferences.

https://reviewboard.mozilla.org/r/120152/#review123414

Thank you for fixing the test and improving it. I think like you mentioned in the follow-up comment, we should fix the test to match the code, rather than the code to match the test.

::: browser/components/preferences/in-content/tests/browser_site_login_exceptions.js:2
(Diff revision 1)
> +"use strict";
> +/* eslint no-undef:"error" */

Please drop this line, no-undef is enabled by default for browser/components now, I'd left it in by mistake.

::: browser/components/preferences/permissions.js:336
(Diff revision 1)
>  
>    onPermissionKeyPress(aEvent) {
>      if (aEvent.keyCode == KeyEvent.DOM_VK_DELETE) {
>        this.onPermissionDeleted();
> -    } else if (AppConstants.platform == "macosx" &&
> +    } else if ((AppConstants.platform == "macosx" &&
> +                AppConstants.platform == "linux") ||

As per your follow-up I think rather than changing behaviours, we should just make the test use VK_DELETE for non-Mac as this reflects the layout issues better.
Attachment #8847117 - Flags: review?(standard8)
Johann, any chance we could get this updated and then maybe landed?
Flags: needinfo?(jhofmann)
(In reply to Mark Banner (:standard8) from comment #6)
> Johann, any chance we could get this updated and then maybe landed?

Yup, sorry, didn't get around to it. Thanks for reminding me.
Flags: needinfo?(jhofmann)
Comment on attachment 8847117 [details]
Bug 1342940 - Add a test for login exception dialog interaction in about:preferences.

https://reviewboard.mozilla.org/r/120152/#review150794

Great, thank you for the update. r=Standard8
Attachment #8847117 - Flags: review?(standard8) → review+
Fixed a failure that occurred after rebasing and converted the whole thing to use async/await. I'll land when the try is green.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b9658a599512
Add a test for login exception dialog interaction in about:preferences. r=standard8
https://hg.mozilla.org/mozilla-central/rev/b9658a599512
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.