1328304 - Urlbar loses focus when typing after clicking on the icon of a permission doorhanger

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Johann Hofmann [:johannh]]

No idea how I discovered this edge case:

STR:

1. Open http://permission.site/
2. Click on a button to get a permission notification
3. Click on the doorhanger anchor icon
4. Type something in the urlbar

I can only type a single character before it loses focus.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=06429d283d5b465e5c484dc6a7447ad03b2589c0&tochange=1955f4507d13e7c834a90260fe6205f338a45a76

Regressed by: 1955f4507d13	Paolo Amadini — Bug 1300755 - Notification anchors are not hidden when the user types in the location bar. r=past

Comment 2

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

Suggesting P3 as it is an edge case and it only happens once (i.e. when you focus the location bar again you can continue typing).

Comment 3

[Johann Hofmann [:johannh]]

[Tracking Requested - why for this release]: This is a (slightly edge case) regression that needs to be fixed, but we might not get around to do it in this release.

Comment 4

[Johann Hofmann [:johannh]]

Panos explained to me on IRC that requesting tracking means we should probably prioritize this, so I'm moving to P1.

Comment 5

[:Paolo Amadini]

Uh, this bug is really bad. The second time around I don't have to follow step 3 from comment 0, meaning that the URL bar will always lose focus.

Related, but not necessarily the same bug, after following these steps it's possible that the awesomebar popup will appear under the notification popup, on Mac OS X at least. To fix this fully, likely we should hide all notifications while the awesomebar popup is showing. This is also very important to fix.

I'm wondering whether we should take a step back and hide all notifications, period, while the "pageproxystate" is "invalid". This would be entirely motivated by code simplicity and keeping the possible bug count low, because we know that from a user experience standpoint the drawback is that this will prevent users from answering any notification, if they accidentally typed something in the address bar and didn't realize it.

We don't provide a discoverable way to revert the editing in the address bar. The "reload" button is also hidden while typing. I and others have suggested replacing the "i" icon with an "X" icon while editing, which would be a possible future direction independent from this bug, but it would mitigate the concerns a little bit.

Comment 6

[Marcia Knous [:marcia]]

Tracking 53+ since it seems the URL bar always loses focus.

Comment 7

[:Paolo Amadini]

We just discussed this in a meeting with Florian, Gijs, Johann, myself, Peter, and Philipp.

The immediate solution to this bug, including the issue that the awesomebar popup may appear under the notification popup, is that we'll hide the notifications completely when the URL is being edited *and* the location bar has the keyboard focus. Focus in the location bar includes virtual focus inside the awesomebar popup, for example using up and down arrow navigation or mouse hover.

When the URL is being edited and the location bar does not have the keyboard focus, we'll display the popup notifications again, anchored to the "i" icon. This is the current behavior in Nightly already. Keeping this state solves the usability concerns with accidental typing, mentioned in comment 5.

We still have to be careful when transitioning between the two states above, because the notification popup should not steal focus from the new interface element that the user selected, whether it's in the web page or elsewhere in the location bar. We should have regression tests for this specifically.

We also mentioned some possible longer term changes to the URL bar behavior when focus is lost, but these are out of scope for this bug.

Comment 8

[:Paolo Amadini]

Attachment: Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

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

Comment 9

[:Paolo Amadini]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

This seems to work, I've tried it manually in various situations including switching tabs, but tell me if you find any edge case I'm missing. I'll make sure to include a test for it.

This also solves an intermittent issue with "noautofocus". It seems to me that the panel is still not autofocused when a permission panel is opened with user interaction, but I believe that's a different bug we discussed.

I'll have a test for tabbing away from the address bar anyways.

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

Sorry for the delay, I didn't realize this would be so straightforward :)

Seems good to me both in code and behavior. I have spent some time playing around with it and wasn't able to break anything yet. I haven't really pondered about edge cases yet so I might add another comment later but I think with some tests these edge cases are likely to reveal themselves anyway.

Thanks!

Comment 11

[Johann Hofmann [:johannh]]

> It seems to me that the panel is still not autofocused when a permission panel is opened with user interaction, but I believe that's a different bug we discussed.

Bug 1334496? :)

Comment 12

[Johann Hofmann [:johannh]]

Ah so here's a slightly contrived but not impossible way to get both the awesomebar autocomplete and the doorhanger to show:

1) run `setTimeout(() => navigator.geolocation.getCurrentPosition(() => {}), 5000)` in the page console
2) click inside the URL bar and type something

I'd say this is not an edge case we should ignore, since some sites spawn permission requests during usage (e.g. a desktop-notification prompt in a chat app once a message arrives). I wonder what would be best here. Remove focus from the URL bar or not show the permission prompt until the user clicks away?

Comment 13

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] from comment #12)
> not show the permission prompt until the user clicks away?

This, definitely. Looks like we should keep some state somewhere to prevent this.

Comment 14

[:Paolo Amadini]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

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

Comment 15

[:Paolo Amadini]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

(In reply to Johann Hofmann [:johannh] from comment #10)
> Sorry for the delay, I didn't realize this would be so straightforward :)

Not so straightforward once you need to maintain state...

So the issue here is that the module is lazily loaded, and the only method to ensure we do the right thing when other consumers invoke "show" is to use a callback that is also called on construction.

Comment 16

[:Paolo Amadini]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

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

Comment 17

[:Paolo Amadini]

Hopefully the isForLocationChange workaround is not necessary after the work Florian made to avoid multiple showing events for notifications that are already displayed. At least, tests didn't fail anymore.

Comment 18

[:Paolo Amadini]

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

Comment 19

[:Paolo Amadini]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

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

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

https://reviewboard.mozilla.org/r/109146/#review112326

This looks mostly good but I'm cancelling review for this version because I had some questions below :)

There are also some try failures so there would be more changes anyway.

I'm not... enthusiastic about the concept of shouldSupressFn and that we have to add more complexity to PopupNotifications.jsm but it seems there is no way around that.

Yesterday I discovered that there is a problem when switching tabs but it seems that you had resolved this already!

I would also like to note that I manually tested non-persistent notifications and they seem to behave as expected. Maybe you can come up with a test case for those but I wouldn't require it.

::: browser/base/content/browser.js:4586
(Diff revision 3)
>          this.reloadCommand.setAttribute("disabled", "true");
>        } else {
>          this.reloadCommand.removeAttribute("disabled");
>        }
>  
> -      URLBarSetURI(aLocationURI, { isForLocationChange: true });
> +      URLBarSetURI(aLocationURI);

Removing this option improves the code a lot IMO :)

::: browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js:158
(Diff revision 3)
>        yield shownLastTime;
>  
>        checkPopup(PopupNotifications.panel, this.notifyObj);
>  
> -      let hidden = new Promise(resolve => onPopupEvent("popuphidden", resolve));
> +      // Hide the persistent notification.
> +      let hiddenLastTime = waitForNotificationPanelHidden();

If you want to avoid calling the variables "hiddenAgain", "hiddenLastTime" and so on you could safely re-use the "hidden" variable.

::: browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js:162
(Diff revision 3)
> -      let hidden = new Promise(resolve => onPopupEvent("popuphidden", resolve));
> +      // Hide the persistent notification.
> +      let hiddenLastTime = waitForNotificationPanelHidden();
> +      this.notification.remove();
> +      gBrowser.removeTab(gBrowser.selectedTab);
> +      gBrowser.selectedTab = this.oldSelectedTab;
> +      yield hiddenLastTime;

I don't understand why you need to listen to this hidden event, has there been an intermittent associated with not doing that? Can't you simply replace lines 158-160 and 162 with

yield BTU.removeTab(gBrowser.selectedTab);

?

::: browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js:207
(Diff revision 3)
>  
>        goNext();
>      }
>    },
> +  // Test that persistent panels are still open after switching to another tab
> +  // and back, even while editing the URL in the new tab.

This was the problem I also discovered, good job including this as a test :)

::: browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js:217
(Diff revision 3)
> +      this.notifyObj = new BasicNotification(this.id);
> +      this.notifyObj.anchorID = "geo-notification-icon";
> +      this.notifyObj.addOptions({
> +        persistent: true,
> +      });
> +      this.notification = showNotification(this.notifyObj);

Shouldn't we also open a new tab for this test that we can clean up afterwards?

::: toolkit/modules/PopupNotifications.jsm:208
(Diff revision 3)
>   *        It is used as a fallback popup anchor if notifications specify
>   *        invalid or non-existent anchor IDs.
> + * @param options
> + *        An optional object with the following optional properties:
> + *        {
> + *          shouldSuppressFn:

I'm not a huge fan of calling a function SomethingFn, maybe rename this to only "shouldSuppress"?

::: toolkit/modules/PopupNotifications.jsm:1312
(Diff revision 3)
>    },
>  
>    _onPopupHidden: function PopupNotifications_onPopupHidden(event) {
> +    // Ensure that when the panel comes up without user interaction,
> +    // we don't autofocus it.
> +    this.panel.setAttribute("noautofocus", "true");

Why move this? :)

Comment 21

[Johann Hofmann [:johannh]]

Whoops, bad timing, I'll take another look at the new revision.

Comment 22

[:Paolo Amadini]

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

Comment 23

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] from comment #21)
> Whoops, bad timing, I'll take another look at the new revision.

That should only fix one test failure, I haven't see your review yet.

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

https://reviewboard.mozilla.org/r/109146/#review112344

::: browser/base/content/browser.js:2601
(Diff revision 4)
>      return;
>  
> +  let oldPageProxyState = gURLBar.getAttribute("pageproxystate");
> +  // The "browser_urlbar_stop_pending.js" test uses a MutationObserver to do
> +  // some verifications at this point, and it breaks if we don't write the
> +  // attribute, even if it hasn't changed (bug 1338115).

Ugh.

Comment 25

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] from comment #20)
> I'm not... enthusiastic about the concept of shouldSupressFn and that we
> have to add more complexity to PopupNotifications.jsm but it seems there is
> no way around that.

The upside is that we may be able to use the same system to hide the notifications when the window is minimized if we still have bugs in that area.

> I would also like to note that I manually tested non-persistent
> notifications and they seem to behave as expected. Maybe you can come up
> with a test case for those but I wouldn't require it.

Good idea, at least we should check they don't break and they are delayed as required.

> Shouldn't we also open a new tab for this test that we can clean up
> afterwards?

Actually, I realize now that we don't need a new tab for all the test where we don't navigate.

> I'm not a huge fan of calling a function SomethingFn, maybe rename this to
> only "shouldSuppress"?

I renamed this, but in general I still feel that, though inelegant, this convention avoids confusion on whether you should pass a boolean or a function returning a boolean, when you're building an options object or passing an argument. I know it sounds a lot like calling something "notificationNameString" instead of "notificationName", but it's different because in the latter case a string is already what I would expect for a notification name.

(Given that this is passed to the constructor of the service, a bit of reasoning tells that this must be a callback anyways.)

Comment 26

[:Paolo Amadini]

There may still be some intermittents, but posting the new version for review in the meantime.

Comment 27

[:Paolo Amadini]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

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

Comment 28

[:Paolo Amadini]

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

Comment 29

[:Paolo Amadini]

Sorry, missed that it's still better to check the target of the popup close event before resetting the "noautofocus" attribute, despite not doing that actually doesn't hurt.

Comment 30

[:Paolo Amadini]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

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

Comment 31

[:Paolo Amadini]

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

Comment 32

[Johann Hofmann [:johannh]]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

https://reviewboard.mozilla.org/r/109146/#review112652

Looks good now, thank you!

::: browser/base/content/test/popupNotifications/browser_popupNotification_no_anchors.js:111
(Diff revision 6)
> -  // editing the URL in the location bar, and restored to their anchors when the
> -  // URL is reverted.
> +  // location bar, anchored to the identity icon when the focus is moved away
> +  // from the location bar, and restored when the URL is reverted.
>    { id: "Test#4",
>      *run() {
> -      this.oldSelectedTab = gBrowser.selectedTab;
> -      yield BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.com/");
> +      for (let persistent of [false, true]) {
> +        // Show a persistent notification.

Nit: It's not necessarily persistent now, I'd just remove that comment.

::: toolkit/modules/PopupNotifications.jsm:1315
(Diff revision 6)
> -    if (event.target != this.panel || this._ignoreDismissal) {
> +    if (event.target != this.panel) {
> +      return;
> +    }
> +
> +    // We may have removed the "noautofocus" attribute before showing the panel
> +    // if it was opened with user iteraction. When the panel is closed, we have

typo: interaction

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/785314dbec57
Hide notifications when the address bar has focus and the URL is being edited. r=johannh

Comment 34

[:Paolo Amadini]

(In reply to Johann Hofmann [:johannh] from comment #32)
> Nit: It's not necessarily persistent now, I'd just remove that comment.

Good catch, removed a bunch of other comments but missed this one.

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/785314dbec57

Comment 36

[:Paolo Amadini]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1300755
[User impact if declined]: As the bug title says. Also, the bug was solved by improving on the current design, so the feature wouldn't be complete without this bug.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not tested manually yet
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Covered by regression tests
[String changes made/needed]: None

Comment 37

[Gerry Chang [:gchang]]

Hi Florin,
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!

Comment 38

[Andrei Vaida [:avaida]]

(In reply to Gerry Chang [:gchang] from comment #37)
> Hi Florin,
> could you help verify if this issue is fixed as expected on the latest
> Nightly build? Thanks!

Forwarding this to Brindusa for verification on Nightly.

Comment 39

[Adrian Florinescu [:aflorinescu]]

Verified as fixed on 54.0a1 / 20170214110212 on Mac OSX 10.12 and Windows10 x64, Windows 7 x64.

I noticed a small issue on Ubuntu 16.04:

STR:
1. Open permission.site
2. Click on anything that opens a permission prompt.
3. Click on the urlbar
4. Type a char.

AR: The cursor is placed at the end of the address, and the address is not autoselected. The char typed will be concatenated to the existent url.

ER: I would assume that the ER should be the same as on Win or OSX, meaning the address is selected and when I focus the urlbar.

Paolo, could you take a look? I'm not sure if the above is related to this bug.

Comment 40

[:Paolo Amadini]

I think you're observing bug 333714.

Comment 41

[Adrian Florinescu [:aflorinescu]]

Right, missed that one. Thanks Paolo.

Given comment 40/41, marking this bug as verified for 54.

Comment 42

[Gerry Chang [:gchang]]

Comment on attachment 8832894 [details]
Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.

Fix a regression related to URL bar and was verified. Aurora53+.

Comment 43

[Carsten Book [:Tomcat]]

needs rebasing for aurora

grafting 398162:785314dbec57 "Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited. r=johannh"
merging browser/base/content/browser.js
merging browser/base/content/test/popupNotifications/head.js
merging browser/base/content/urlbarBindings.xml
merging toolkit/modules/PopupNotifications.jsm
warning: conflicts while merging toolkit/modules/PopupNotifications.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Paulo did the rebase and push it to Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/8dadad0f570d

Comment 45

[:Paolo Amadini]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #44)
> Paulo did the rebase and push it to Aurora.
> https://hg.mozilla.org/releases/mozilla-aurora/rev/8dadad0f570d

Ah, I thought the URL would appear automatically in the bug like it happens for commits to mozilla-central. Thanks!

Comment 46

[Hani Yacoub, Desktop QA]

Build ID: 20170221004019
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

Verified as fixed on Aurora 53.0a2 (20170221004019) on Windows 10 x 64, Mac OS X 10.10 and Ubuntu 16.04 x64.