The Esc key should deny permission requests for persistent prompts

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Site Identity and Permission Panels
P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: past, Assigned: johannh)

Tracking

Trunk
Firefox 53
Points:
---
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox53 verified, firefox54 verified)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

The new persistent permission prompts don't handle the Esc key as a way to deny the request and dismiss the prompt. They should do so, as the Esc key was a common workflow of the previous prompts that we want to maintain.
(Reporter)

Updated

a year ago
Priority: -- → P1
Whiteboard: [fxprivacy]
(Assignee)

Updated

a year ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Iteration: --- → 53.3 - Dec 26
Flags: qe-verify?

Comment 3

a year ago
mozreview-review
Comment on attachment 8817735 [details]
Bug 1320719 - The Esc key should deny permission requests for persistent prompts.

https://reviewboard.mozilla.org/r/97946/#review100286

::: browser/base/content/test/popupNotifications/browser_popupNotification_5.js:76
(Diff revision 1)
>        let win = gBrowser.replaceTabWithWindow(gBrowser.addTab("about:blank"));
>        whenDelayedStartupFinished(win, function() {
> -        showNotification(notifyObj);
> +        let notification = showNotification(notifyObj);
>          let anchor = document.getElementById("default-notification-icon");
>          is(anchor.getAttribute("showing"), "true", "the anchor is shown");
> +        notification.remove();

I remember having to cleanup a bunch of notifications when adding a test in the past; could we prevent this from happening again by maybe adding something runNextTest from head.js to ensure there's no notification left at the end of a test before we start the next one?

::: toolkit/modules/PopupNotifications.jsm:222
(Diff revision 1)
>    this.iconBox = iconBox;
>    this.buttonDelay = Services.prefs.getIntPref(PREF_SECURITY_DELAY);
>  
>    this.panel.addEventListener("popuphidden", this, true);
>  
> +  this.window.addEventListener("keypress", this, true);

Are you sure you want this event handler to run for all key presses on the whole browser window, even when the notification panel isn't visible?

::: toolkit/modules/PopupNotifications.jsm:544
(Diff revision 1)
>            self._update();
>          }, 0);
>          break;
>        case "click":
>        case "keypress":
> +        if (aEvent.target == this.iconBox || this.iconBox.contains(aEvent.target)) {

In which case do we need the this.iconBox.contains(aEvent.target) test? The only existing addEventListener("keypress", ... call I can see is on the iconBox itself, not on a childnode.

::: toolkit/modules/PopupNotifications.jsm:550
(Diff revision 1)
> -        this._onIconBoxCommand(aEvent);
> +          this._onIconBoxCommand(aEvent);
> +        } else if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE) {
> +          // Esc key cancels the topmost notification, if there is one.
> +          let notification = this.panel.firstChild;
> +          if (notification) {
> +            this._onButtonEvent(aEvent, 'secondarybuttoncommand', notification);

nit: double quotes
Attachment #8817735 - Flags: review?(florian) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> Comment on attachment 8817735 [details]
> Bug 1320719 - The Esc key should deny permission requests for persistent
> prompts.
> 
> https://reviewboard.mozilla.org/r/97946/#review100286
> 
> :::
> browser/base/content/test/popupNotifications/browser_popupNotification_5.js:
> 76
> (Diff revision 1)
> >        let win = gBrowser.replaceTabWithWindow(gBrowser.addTab("about:blank"));
> >        whenDelayedStartupFinished(win, function() {
> > -        showNotification(notifyObj);
> > +        let notification = showNotification(notifyObj);
> >          let anchor = document.getElementById("default-notification-icon");
> >          is(anchor.getAttribute("showing"), "true", "the anchor is shown");
> > +        notification.remove();
> 
> I remember having to cleanup a bunch of notifications when adding a test in
> the past; could we prevent this from happening again by maybe adding
> something runNextTest from head.js to ensure there's no notification left at
> the end of a test before we start the next one?

I tried to do this but there are enough tests that rely on e.g. notifications persisting through the next test that make this endeavor pretty complex. If this becomes an actual problem and not just a nuisance we can solve it in its own bug. :) 

> ::: toolkit/modules/PopupNotifications.jsm:544
> (Diff revision 1)
> >            self._update();
> >          }, 0);
> >          break;
> >        case "click":
> >        case "keypress":
> > +        if (aEvent.target == this.iconBox || this.iconBox.contains(aEvent.target)) {
> 
> In which case do we need the this.iconBox.contains(aEvent.target) test? The
> only existing addEventListener("keypress", ... call I can see is on the
> iconBox itself, not on a childnode.
> 

You can tab through the anchor icons and toggle them with the space key, in which case we want to capture the event with the icon as target.
(Assignee)

Comment 7

a year ago
mozreview-review
Comment on attachment 8817735 [details]
Bug 1320719 - The Esc key should deny permission requests for persistent prompts.

https://reviewboard.mozilla.org/r/97946/#review102010

::: toolkit/modules/PopupNotifications.jsm:560
(Diff revision 3)
>        case "keypress":
> +        if (aEvent.target == this.iconBox || this.iconBox.contains(aEvent.target)) {
> -        this._onIconBoxCommand(aEvent);
> +          this._onIconBoxCommand(aEvent);
> +        } else if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE &&
> +          // Cancelling URL bar editing should not cancel the notification in the same action.
> +          (aEvent.target.id != "urlbar" || aEvent.target.getAttribute("pageproxystate") == "valid")) {

I don't really like how we're mixing browser and toolkit concerns here, but this is an issue that needs to be addressed somehow and I didn't find a cleaner way to do it. Suggestions welcome.
(In reply to Johann Hofmann [:johannh] from comment #6)

> > browser/base/content/test/popupNotifications/browser_popupNotification_5.js:
> > 76
> > (Diff revision 1)
> > >        let win = gBrowser.replaceTabWithWindow(gBrowser.addTab("about:blank"));
> > >        whenDelayedStartupFinished(win, function() {
> > > -        showNotification(notifyObj);
> > > +        let notification = showNotification(notifyObj);
> > >          let anchor = document.getElementById("default-notification-icon");
> > >          is(anchor.getAttribute("showing"), "true", "the anchor is shown");
> > > +        notification.remove();
> > 
> > I remember having to cleanup a bunch of notifications when adding a test in
> > the past; could we prevent this from happening again by maybe adding
> > something runNextTest from head.js to ensure there's no notification left at
> > the end of a test before we start the next one?
> 
> I tried to do this but there are enough tests that rely on e.g.
> notifications persisting through the next test that make this endeavor
> pretty complex.

We could either add a flag on the tests to skip the check for the specific tests that rely on this behavior, or have the check run only once per test file.

> If this becomes an actual problem and not just a nuisance we
> can solve it in its own bug. :)

Are you going to file a bug? :-)

Potential actual problems that could be caused by issues we can catch this way:
- leaks
- tests behaving differently when run alone or when run with the whole folder.
(Assignee)

Comment 9

a year ago
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> 
> We could either add a flag on the tests to skip the check for the specific
> tests that rely on this behavior, or have the check run only once per test
> file.

Alright, since you seem to really want this I'll see if I can make it work :)

Comment 10

a year ago
mozreview-review
Comment on attachment 8817735 [details]
Bug 1320719 - The Esc key should deny permission requests for persistent prompts.

https://reviewboard.mozilla.org/r/97946/#review102254

I played a bit with the patch locally but haven't found a magic clean solution for the urlbar case unfortunately. I wonder if what's blocking us here could be that the behavior we want isn't very well defined. Eg. if the web page uses the Escape key for something, do we still want that Escape keypress to deny the current permission prompt? If a page has a form where we offer completions, will the Escape key close both the autofill popup and the permission prompt?

::: toolkit/modules/PopupNotifications.jsm:556
(Diff revision 3)
>            self._update();
>          }, 0);
>          break;
>        case "click":
>        case "keypress":
> +        if (aEvent.target == this.iconBox || this.iconBox.contains(aEvent.target)) {

So is it the 'aEvent.target == this.iconBox' part that we don't need then?

I still dislike this test. Given that you have a capturing and a non-capturing listener, you could use that distinction to separate between the existing case and the new code you are adding.

if (aEvent.eventPhase == Event.CAPTURING_PHASE) {
  if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE ...
     ...
  return;
}

::: toolkit/modules/PopupNotifications.jsm:560
(Diff revision 3)
>        case "keypress":
> +        if (aEvent.target == this.iconBox || this.iconBox.contains(aEvent.target)) {
> -        this._onIconBoxCommand(aEvent);
> +          this._onIconBoxCommand(aEvent);
> +        } else if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE &&
> +          // Cancelling URL bar editing should not cancel the notification in the same action.
> +          (aEvent.target.id != "urlbar" || aEvent.target.getAttribute("pageproxystate") == "valid")) {

The searchbar has the same problem as the location bar.

I wonder if somehow we could catch that the keypress is going to a textbox of type autocomplete, and ignore the event if the autocomplete panel is open... but for the searchbar the event is received for the searchbar rather than its textbox, unfortunately.

::: toolkit/modules/PopupNotifications.jsm:1016
(Diff revision 3)
>        let anchorElement = anchors.values().next().value;
>        if (anchorElement) {
>          this._showPanel(notificationsToShow, anchorElement);
>        }
> +
> +      // Listen for keyboard interactions with the opened notifications.

This comment should be clearer and match what we are doing. Something like: "Setup a capturing event listener on the whole window to catch the Escape key while persistent notifications are visible."

::: toolkit/modules/PopupNotifications.jsm:1041
(Diff revision 3)
>              anchorElement.removeAttribute(ICON_ATTRIBUTE_SHOWING);
>          }
>        }
> +
> +      // Stop listening to keyboard events for notifications.
> +      this.window.removeEventListener("keypress", this);

Without adding a ", true" this is a no-op, as it's trying to remove a non-capturing listener that has never been added.
Attachment #8817735 - Flags: review?(florian) → review-
2c, without context and without trying it...

(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> Eg. if the
> web page uses the Escape key for something, do we still want that Escape
> keypress to deny the current permission prompt?

Yes.

> If a page has a form where
> we offer completions, will the Escape key close both the autofill popup and
> the permission prompt?

No.

IMO, the doorhanger appears "floating" over the page. Esc should only dismiss it and not effect the page in any way.

(chrome having focus is obviously trickier, and I'm ignoring the technical issues - this is just my personal expectation as a user :)
(Assignee)

Comment 12

a year ago
(In reply to Johann Hofmann [:johannh] from comment #9)
> (In reply to Florian Quèze [:florian] [:flo] from comment #8)
> > 
> > We could either add a flag on the tests to skip the check for the specific
> > tests that rely on this behavior, or have the check run only once per test
> > file.
> 
> Alright, since you seem to really want this I'll see if I can make it work :)

Scratch that, it's really more complex and potentially breaking than I expected. This should really be handled separately. FWIW, there's already a cleanup happening on a per-file basis. So I really don't think this is worth the trouble right now.
Comment hidden (mozreview-request)
(Assignee)

Comment 14

a year ago
This approach does not handle events if there's an element with focus in the chrome window. I think that's as close as we can get.

Comment 15

a year ago
mozreview-review
Comment on attachment 8817735 [details]
Bug 1320719 - The Esc key should deny permission requests for persistent prompts.

https://reviewboard.mozilla.org/r/97946/#review102578

(In reply to Johann Hofmann [:johannh] from comment #14)
> This approach does not handle events if there's an element with focus in the
> chrome window. I think that's as close as we can get.

Nice. Can we make the Escape key work when the focus is within the notification itself? (Click on the 'Microphone to share:' label to force the focus to get there)

::: toolkit/modules/PopupNotifications.jsm:229
(Diff revision 4)
> +  this._handleWindowKeyPress = aEvent => {
> +    if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE &&
> +      // If the chrome window has a focused element, let it handle the ESC key instead.
> +      (!this.window.document.activeElement ||
> +       this.window.document.activeElement == this.window.document.body ||
> +       this.window.document.activeElement == this.tabbrowser.selectedBrowser)) {

Please use a temporary variable to avoid calling the activeElement getter 3 times (with an early return when the key isn't Esc, to avoid calling the activeElement getter when we don't need it).

You may want an additionnal let doc = this.window.document.
Attachment #8817735 - Flags: review?(florian) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

a year ago
mozreview-review
Comment on attachment 8817735 [details]
Bug 1320719 - The Esc key should deny permission requests for persistent prompts.

https://reviewboard.mozilla.org/r/97946/#review102882

Works well now, thanks! :-)

::: toolkit/modules/PopupNotifications.jsm:234
(Diff revision 6)
> +      // Esc key cancels the topmost notification, if there is one.
> +      let notification = this.panel.firstChild;
> +      if (notification &&
> +          // If the chrome window has a focused element, let it handle the ESC key instead.
> +          (!activeElement ||
> +          activeElement == doc.body ||

nit: The ident is one space off here.

This would be more readable with an early return if !notification.

Also, the "let doc" and "let activeElement" lines should be after that early return.

While we are at it, another early return if (aEvent.keyCode != aEvent.DOM_VK_ESCAPE) could further reduce the indent level.

::: toolkit/modules/PopupNotifications.jsm:239
(Diff revision 6)
> +          activeElement == doc.body ||
> +          activeElement == this.tabbrowser.selectedBrowser ||
> +          // Ignore focused elements inside the notification.
> +          getNotificationFromElement(activeElement) == notification ||
> +          notification.contains(activeElement))) {
> +

nit: This empty line isn't helpful.
Attachment #8817735 - Flags: review?(florian) → review+
Comment hidden (mozreview-request)

Comment 21

a year ago
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46985f01d9ac
The Esc key should deny permission requests for persistent prompts. r=florian

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/46985f01d9ac
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Comment 23

a year ago
Build ID: 20170207030214
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1, Firefox Nightly 53.0a1 and Aurora 53.0a2.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
status-firefox54: --- → verified
You need to log in before you can comment on or make changes to this bug.