1320719 - The Esc key should deny permission requests for persistent prompts

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1320719 - The Esc key should deny permission requests for persistent prompts.

Review commit: https://reviewboard.mozilla.org/r/97946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97946/

Comment 2

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7c7c3ac17c8

Comment 3

[Florian Quèze [:florian]]

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

Comment 4

[Johann Hofmann [:johannh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97946/diff/1-2/

Comment 5

[Johann Hofmann [:johannh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97946/diff/2-3/

Comment 6

[Johann Hofmann [:johannh]]

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

Comment 7

[Johann Hofmann [:johannh]]

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.

Comment 8

[Florian Quèze [:florian]]

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

Comment 9

[Johann Hofmann [:johannh]]

(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

[Florian Quèze [:florian]]

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.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

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

Comment 12

[Johann Hofmann [:johannh]]

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

[Johann Hofmann [:johannh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97946/diff/3-4/

Comment 14

[Johann Hofmann [:johannh]]

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

[Florian Quèze [:florian]]

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.

Comment 16

[Johann Hofmann [:johannh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97946/diff/4-5/

Comment 17

[Johann Hofmann [:johannh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97946/diff/5-6/

Comment 18

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f14687a14fa

Comment 19

[Florian Quèze [:florian]]

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.

Comment 20

[Johann Hofmann [:johannh]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97946/diff/6-7/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46985f01d9ac
The Esc key should deny permission requests for persistent prompts. r=florian

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46985f01d9ac

Comment 23

[Hani Yacoub, Desktop QA]

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.