Closed Bug 1328304 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Site Identity, defect, P1)

Product:
Firefox
Component:
Site Identity
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Iteration:
54.2 - Feb 20
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 + verified
firefox54 --- verified

People

(Reporter: johannh, Assigned: Paolo)

References

(Depends on 5 open bugs, Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [fxprivacy])

Attachments

(1 file)

Bug 1328304 - Hide notifications when the address bar has focus and the URL is being edited.
59 bytes, text/x-review-board-request
johannh
: review+
gchang
: approval-mozilla-aurora+
Details 
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.
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
Blocks: 1300755
status-firefox52: --- → unaffected
Keywords: regressionwindow-wanted
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).
Priority: -- → P3
[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.
tracking-firefox53: --- → ?
Panos explained to me on IRC that requesting tracking means we should probably prioritize this, so I'm moving to P1.
Priority: P3 → P1
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
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.
Whiteboard: [fxprivacy] → [fxprivacy][triage]
Flags: needinfo?(philipp)
Tracking 53+ since it seems the URL bar always loses focus.
tracking-firefox53: ? → +
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.
Flags: needinfo?(philipp)
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Assignee: nobody → paolo.mozmail
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
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.
Attachment #8832894 - Flags: feedback?(jhofmann)
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!
Attachment #8832894 - Flags: feedback?(jhofmann) → feedback+
> 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? :)
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?
(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 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.
Attachment #8832894 - Flags: feedback?(jhofmann)
Iteration: --- → 54.2 - Feb 20
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.
Blocks: 1338115
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? :)
Attachment #8832894 - Flags: review?(jhofmann)
Whoops, bad timing, I'll take another look at the new revision.
(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 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.
(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.)
There may still be some intermittents, but posting the new version for review in the meantime.
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 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
Attachment #8832894 - Flags: review?(jhofmann) → review+
Pushed by paolo.mozmail@amadzone.org:
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
(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.
https://hg.mozilla.org/mozilla-central/rev/785314dbec57
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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
Attachment #8832894 - Flags: approval-mozilla-aurora?
Hi Florin,
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(florin.mezei)
(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.
Flags: needinfo?(florin.mezei) → needinfo?(brindusa.tot)
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.
Flags: needinfo?(brindusa.tot) → needinfo?(paolo.mozmail)
I think you're observing bug 333714.
Flags: needinfo?(paolo.mozmail)
Right, missed that one. Thanks Paolo.

Given comment 40/41, marking this bug as verified for 54.
status-firefox54: fixed → verified
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+.
Attachment #8832894 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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')
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(paolo.mozmail)
(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!
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.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Depends on: 1341286
Depends on: 1347170
Depends on: 1352769
Depends on: 1352765
Depends on: 1352764
Depends on: 1349453
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: