Last Comment Bug 1300755 - Notification anchors are not hidden when the user types in the location bar
: Notification anchors are not hidden when the user types in the location bar
Status: VERIFIED FIXED
[fxprivacy]
: regression
Product: Firefox
Classification: Client Software
Component: Site Identity and Permission Panels (show other bugs)
: Trunk
: Unspecified Unspecified
P2 normal (vote)
: Firefox 53
Assigned To: :Paolo Amadini
:
: Johann Hofmann [:johannh]
Mentors:
Depends on: 1231025 1328304
Blocks: 1239930 1267617
  Show dependency treegraph
 
Reported: 2016-09-06 05:16 PDT by Johann Hofmann [:johannh]
Modified: 2017-01-03 16:12 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 53.3 - Dec 26
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
wontfix
wontfix
wontfix
wontfix
verified

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Example (54.01 KB, image/gif)
2016-09-06 05:16 PDT, Johann Hofmann [:johannh]
no flags Details
Bug 1300755 - Hide notification anchors on invalid pageproxystate. (58 bytes, text/x-review-board-request)
2016-09-14 06:25 PDT, Johann Hofmann [:johannh]
no flags Details | Review
Bug 1300755 - Rename pageproxystate to editing and separate from blank page state. (58 bytes, text/x-review-board-request)
2016-10-05 12:09 PDT, Johann Hofmann [:johannh]
no flags Details | Review
Bug 1300755 - Notification anchors are not hidden when the user types in the location bar. (58 bytes, text/x-review-board-request)
2016-11-07 07:28 PST, :Paolo Amadini
past: review+
Details | Review

Description User image Johann Hofmann [:johannh] 2016-09-06 05:16:07 PDT
Created attachment 8788429 [details]
Example

See the appended GIF. All other icons are hidden, but the notification anchor stays. Not sure if we want this.
Comment 1 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2016-09-06 05:43:35 PDT
I'm not sure if we want to keep it or change it, but the current behavior makes sense to me. The icons that are hidden are tied to the location, which changes when the user types in the location bar. The other icons are tied to the page, which is still displayed until the user starts loading something else.
Comment 2 User image Johann Hofmann [:johannh] 2016-09-06 05:50:04 PDT
So I personally think we should hide them, it looks more consistent visually and doesn't make the user think about why certain icons were hidden and others not.

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> I'm not sure if we want to keep it or change it, but the current behavior
> makes sense to me. The icons that are hidden are tied to the location, which
> changes when the user types in the location bar. The other icons are tied to
> the page, which is still displayed until the user starts loading something
> else.

I don't understand the difference between tied to location and tied to page tbh. In my view the lock and the tracking protection icon have the same scope as the permission anchors.
Comment 3 User image Panos Astithas [:past] 2016-09-06 07:26:03 PDT
I think I agree with Johann on this and moreover we should also change the (i) icon to lose the dot when typing in a site with granted permissions for the same reasons.
Comment 4 User image Panos Astithas [:past] 2016-09-06 07:28:34 PDT
I'm on the fence about the animated sharing indicator though: consistency dictates that it should switch to the non-animated (i) for as long as the user is editing the location, but perhaps reminding about the privacy-invasive status should trump consistency in this case.
Comment 5 User image Erin Lancaster [:elan] 2016-09-06 09:10:34 PDT
Not a blocker for 49. NI UX for insight.
Comment 6 User image Panos Astithas [:past] 2016-09-06 12:34:37 PDT
To elaborate a bit on the request, I'm told that in older versions of Firefox with the "carrot" container for permission anchors, these icons are still present when the user starts typing a new URL. It's only the lock and the shield that are hidden. I don't know the reason for this behavior, as I presume we hide the other icons as they are no longer relevant in the new URL the user is typing. Perhaps it was deemed less important to hide the permission icons when the (i) is right next to the cursor? Or it may just be a bug, I don't know.

The question is do we want to maintain that behavior now that the permission icons are between the (i) and the cursor, or do we want to hide everything? (See also comment 3 and comment 4 for an expanded definition of "everything").
Comment 7 User image Liz Henry (:lizzard) (needinfo? me) 2016-09-06 15:22:05 PDT
Too late for 49, but we could still take changes for 50.
Comment 8 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2016-09-07 01:24:42 PDT
(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> The other icons are tied to
> the page, which is still displayed until the user starts loading something
> else.

Actually, some other icons may be independent from the page: eg. e10s (hopefully gone soon if not already gone), add-on install.
Comment 9 User image (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo 2016-09-09 05:17:29 PDT
I agree that the icons should be hidden when the user starts editing. Changing the URL moves the focus from the site that the user is currently on to the site that the user wants to visit.

(In reply to Panos Astithas [:past] from comment #4)
> I'm on the fence about the animated sharing indicator though: consistency
> dictates that it should switch to the non-animated (i) for as long as the
> user is editing the location, but perhaps reminding about the
> privacy-invasive status should trump consistency in this case.

That's an interesting case... In some ways, the above argument about focus is even more relevant here, since the animated icon draws more attention. But really, if the user is already about to navigate away, the concern about privacy might be moot since it takes longer to click the icon and remove the pref than to just perform the navigation.

There's another thing to consider though: when the user enter something in the URL bar and then clicks out of it, the icons remain hidden. There is an argument to be made to re-surface the icons in that case, since the users' focus has shifted back to the page.
Comment 10 User image Johann Hofmann [:johannh] 2016-09-14 06:25:56 PDT Comment hidden (mozreview-request)
Comment 11 User image Johann Hofmann [:johannh] 2016-09-14 06:38:59 PDT
This patch fixes the regression that was originally described, since most people seem to agree this is not wanted. The related points brought up by Panos and Philipp are still to be discussed and I wanted to keep this fix separate, also so that we can land it soon.

We might want to speed it up to 50, what does everyone think?
Comment 12 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2016-09-14 07:08:12 PDT
Comment on attachment 8791189 [details]
Bug 1300755 - Hide notification anchors on invalid pageproxystate.

https://reviewboard.mozilla.org/r/78694/#review77288

Hiding the notification icons even when the urlbar isn't focused doesn't feel right to me.

Steps to reproduce the issue I see:
1. Load data:text/html,<iframe src="https://people.mozilla.org/~fqueze2/webrtc/" height=300></iframe><iframe src="https://people.mozilla.org/~fqueze2/webrtc/" height=300></iframe>
2. Click "Video" in the first iframe (do not answer the prompt).
3. Focus the location bar (Cmd+L) and either clear it (backspace) or start typing.
4. Click somewhere in the document.

At this point the notification icons (the prompt for the camera) is impossible to find.

5. Click the "Video" button in the second iframe.

See a new prompt hanging from nowhere.
Comment 13 User image Panos Astithas [:past] 2016-09-15 01:35:41 PDT
I think we already have consensus on hiding everything on edit and bringing it back on a blur event. That includes reverting the dot in the (i) and the animated sharing indicator.
Comment 14 User image Johann Hofmann [:johannh] 2016-09-15 03:27:18 PDT
Alright, I'm working on a) getting the identity block to show on urlbar blur b) getting the (i) icon situation working correctly, hiding everything on pageproxystate invalid and restoring to normal on valid.

We should rename pageproxystate.
Comment 15 User image Johann Hofmann [:johannh] 2016-09-15 05:37:50 PDT Comment hidden (mozreview-request)
Comment 16 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2016-09-16 06:29:02 PDT
Comment on attachment 8791189 [details]
Bug 1300755 - Hide notification anchors on invalid pageproxystate.

https://reviewboard.mozilla.org/r/78694/#review77596

r=me, but it's not a very confident r+. I haven't found any issue when looking at the code, and I tested it without spotting any undesirable behaviors, but I'm not very familiar with the code you are modifying, so my review may miss some issues.

::: browser/base/content/browser.js:2405
(Diff revision 2)
>  
> -    valid = !isBlankPageURL(uri.spec);
> +    valid = true;
>    }
>  
>    gURLBar.value = value;
>    gURLBar.valueIsTyped = !valid;

Looks like we don't need the 'valid' variable anymore, and you can just set this to !!value, and then use gURLBar.valueIsTyped for the SetPageProxyState call.

::: browser/themes/shared/identity-block/identity-block.inc.css:181
(Diff revision 2)
>  
> -#urlbar[pageproxystate="valid"] > #identity-box.verifiedDomain > #connection-icon,
> -#urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity > #connection-icon,
> -#urlbar[pageproxystate="valid"] > #identity-box.mixedActiveBlocked > #connection-icon {
> +#identity-box.verifiedDomain > #connection-icon,
> +#identity-box.verifiedIdentity > #connection-icon,
> +#identity-box.mixedActiveBlocked > #connection-icon {
>    list-style-image: url(chrome://browser/skin/identity-secure.svg);
>    visibility: visible;

Given that the visibility: collapse applies only when pageproxystate is "invalid", do we really need this visibility: visible rule here? (it's a real question, not a request for a change phrased as a question)
Comment 17 User image Johann Hofmann [:johannh] 2016-09-20 03:27:29 PDT
(In reply to Florian Quèze [:florian] [:flo] from comment #16) 
> ::: browser/themes/shared/identity-block/identity-block.inc.css:181
> (Diff revision 2)
> >  
> > -#urlbar[pageproxystate="valid"] > #identity-box.verifiedDomain > #connection-icon,
> > -#urlbar[pageproxystate="valid"] > #identity-box.verifiedIdentity > #connection-icon,
> > -#urlbar[pageproxystate="valid"] > #identity-box.mixedActiveBlocked > #connection-icon {
> > +#identity-box.verifiedDomain > #connection-icon,
> > +#identity-box.verifiedIdentity > #connection-icon,
> > +#identity-box.mixedActiveBlocked > #connection-icon {
> >    list-style-image: url(chrome://browser/skin/identity-secure.svg);
> >    visibility: visible;
> 
> Given that the visibility: collapse applies only when pageproxystate is
> "invalid", do we really need this visibility: visible rule here? (it's a
> real question, not a request for a change phrased as a question)

Yeah I initially thought the same thing but it seems that the icon should only be shown if these classes are applied and there are cases when there's none of these classes on the identity-box. So afaik we need it.
Comment 18 User image Johann Hofmann [:johannh] 2016-09-20 03:34:39 PDT Comment hidden (mozreview-request)
Comment 23 User image :Paolo Amadini 2016-10-04 07:44:20 PDT
(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #9)
> There's another thing to consider though: when the user enter something in
> the URL bar and then clicks out of it, the icons remain hidden. There is an
> argument to be made to re-surface the icons in that case, since the users'
> focus has shifted back to the page.

On the other hand, exactly because the focus of the user has moved away, the absence of security icons (or "invalid" page proxy state like it's called in the code) is relevant when the attention moves back to the location bar. It reminds the user that the address they see is not the address of the page that they are upon, and they have to press ESC to see the original address of the page.

So I'm not sure whether showing the security icons near a URL to which they don't apply is desirable.

(As a side note, maybe while editing an existing non-blank URL the dimmed "i" icon should be a dimmed "x" icon and clicking it could be used to revert the intent to navigate to a different URL. The ESC key is not very discoverable.)
Comment 24 User image :Paolo Amadini 2016-10-04 07:52:16 PDT
On the Elm branch, we may need a related test to ensure that persistent popup notifications are reopened after the page proxy state is back to valid.

In Nightly, we need to test that popup notifications are still displayed correctly if they are opened while the page proxy state is invalid, that is while the user is editing the URL to navigate. We may need to wait until the user has finished editing before we can open the popup notification panel, or anchor it somewhere else until the edit is reverted.
Comment 25 User image Florian Quèze [:florian] [:flo] (PTO until February 27) 2016-10-04 08:08:49 PDT
(In reply to :Paolo Amadini from comment #23)

> On the other hand, exactly because the focus of the user has moved away, the
> absence of security icons (or "invalid" page proxy state like it's called in
> the code) is relevant when the attention moves back to the location bar. It
> reminds the user that the address they see is not the address of the page
> that they are upon, and they have to press ESC to see the original address
> of the page.
> 
> So I'm not sure whether showing the security icons near a URL to which they
> don't apply is desirable.

Even if we don't show security icons when the url doesn't match the page actually loaded, I think we still need to show sharing (camera, etc...) indicators whenever the location bar isn't focused.
Comment 26 User image Johann Hofmann [:johannh] 2016-10-05 12:06:34 PDT
(In reply to :Paolo Amadini from comment #23)
> (In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from
> comment #9)
> > There's another thing to consider though: when the user enter something in
> > the URL bar and then clicks out of it, the icons remain hidden. There is an
> > argument to be made to re-surface the icons in that case, since the users'
> > focus has shifted back to the page.
> 
> On the other hand, exactly because the focus of the user has moved away, the
> absence of security icons (or "invalid" page proxy state like it's called in
> the code) is relevant when the attention moves back to the location bar. It
> reminds the user that the address they see is not the address of the page
> that they are upon, and they have to press ESC to see the original address
> of the page.

No, it doesn't remind the user to press ESC, because we have to assume that there are users (I actually believe the vast majority of users) that don't know about this way to get the security state back. It's even more complicated than just pressing ESC, they have to click inside the URL bar again and press ESC. I don't think I could get any of my relatives to bring the site info back after they clicked outside the URL bar. :)

> 
> So I'm not sure whether showing the security icons near a URL to which they
> don't apply is desirable.
> 

I don't see the problem with that. I don't think anyone except us thinks about pageproxystate when seeing their URL bar to determine if the URL is currently correct. I also can't make up an attack vector that involves the user typing a wrong address in their URL bar.

Not showing the security indicators on urlbar blur makes things more insecure, because we effectively remove them for any user who doesn't know about the click-again-and-press-ESC trick. An (x) button could do it but if this is really not a security risk (which I believe it isn't) why add an additional UI element?
Comment 27 User image Johann Hofmann [:johannh] 2016-10-05 12:09:12 PDT Comment hidden (mozreview-request)
Comment 28 User image Johann Hofmann [:johannh] 2016-10-05 12:09:12 PDT Comment hidden (mozreview-request)
Comment 29 User image Johann Hofmann [:johannh] 2016-10-05 12:15:57 PDT
This patch makes tests pass again (let's wait for another try, had to do a huge rebase) and gets rid of the "pageproxystate" concept, splitting it into an "editing" attribute, a .blankPage class and a field for the original URL. Pageproxystate doesn't really fit anymore, since with the proposed changes the URL bar could show an "invalid" URL without losing the security indicators. The first patch already did that.

Paolo, I wanted to make sure you're happy with the changes around this so I requested you for review. We might also wait for Philipps input.

Thanks :)
Comment 31 User image :Paolo Amadini 2016-10-06 05:30:05 PDT
(In reply to Johann Hofmann [:johannh] - partially unresponsive until 11/14 from comment #26)
> I don't see the problem with that. I don't think anyone except us thinks
> about pageproxystate when seeing their URL bar to determine if the URL is
> currently correct. I also can't make up an attack vector that involves the
> user typing a wrong address in their URL bar.

I actually agree with this analysis, I guess it's time to change how we treat this case.

(In reply to Johann Hofmann [:johannh] - partially unresponsive until 11/14 from comment #29)
> gets rid of the "pageproxystate" concept, splitting it into
> an "editing" attribute, a .blankPage class and a field for the original URL.

Even before I looked at the patch, this sounded great!

Gijs, can you just take a quick look at the patches on the bug and see if we are missing something obvious? While I'm familiar with the identity block logic I'm not familiar with the editing logic of the URL bar in browser.js. The changes look straightforward to me but it's a delicate part of the code.
Comment 32 User image :Gijs 2016-10-06 05:56:58 PDT
(In reply to Johann Hofmann [:johannh] - partially unresponsive until 11/14 from comment #26)
> I don't see the problem with that. I don't think anyone except us thinks
> about pageproxystate when seeing their URL bar to determine if the URL is
> currently correct. I also can't make up an attack vector that involves the
> user typing a wrong address in their URL bar.

It's not very difficult to convince a user to drag a URL to the location bar, and to change that URL and use that for spoofing.

I'm not thrilled about removing a clear indicator of whether the text in the location bar is what the user typed or a reflection of the current page contents, and Paolo's comment doesn't give me enough context to understand why this is necessary or how it relates to this bug - there seems to already be a separate patch to accomplish what we want without removing pageproxystate?
Comment 33 User image :Paolo Amadini 2016-10-11 05:10:36 PDT
Comment on attachment 8798167 [details]
Bug 1300755 - Rename pageproxystate to editing and separate from blank page state.

Following up to comment 32, we should probably split the behavior change about the case where the URL is being edited and the location bar is not focused to a different bug. This means reworking the first patch too.

Also we need to write the tests that I mentioned in comment 24, because we always have to handle the case where we show popup notifications while the URL bar is focused and the "pageproxystate" is "invalid". I guess what we want to do in that case is to delay those notifications until the anchors are visible, and hide any permanent notification when the anchors are hidden, analogously to what we do when the user switches tabs.
Comment 34 User image (Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo 2016-10-17 04:27:41 PDT
I'm afraid that we won't reach a perfect solution unless we start resetting the text in the URL bar when the user de-focuses it (and restoring the input value once the focus moves back). Such a change would require a wider discussion in a different bug since it undoubtedly will open up all kinds of other edge cases.

Until then, I'd prefer to proceed as suggested and hide the anchors while editing the URL.
Comment 35 User image Andrew Overholt [:overholt] 2016-10-19 11:36:20 PDT
Panos, given Johann's partially unresponsive status, is there someone else that could implement Philipp's suggestion in comment 34?
Comment 36 User image Panos Astithas [:past] 2016-10-20 05:50:59 PDT
Paolo, can you pick this one up since you have already reviewed it?
Comment 37 User image Ritu Kothari (:ritu) 2016-10-26 11:51:33 PDT
we are a week away from RC, too late, this is now a wontfix for 50.
Comment 38 User image :Paolo Amadini 2016-11-07 07:28:04 PST Comment hidden (mozreview-request)
Comment 39 User image :Paolo Amadini 2016-11-07 07:36:12 PST
This new version of the patch is limited to just changing the visibility of the anchors, while also moving the popup to the identity icon instead of the current tab in the tabstrip when no anchor is visible.

We could avoid hardcoding "identity-icon" by specifying the fallback anchor indirectly through an attribute on the browser, like we do for the default anchor which is a slightly different concept from the fallback anchor, but I'm not sure it's worth the complexity.

This still needs tests, and I still have to check existing failing tests to see if the change in meaning of _currentAnchorElement causes any regressions.
Comment 40 User image :Paolo Amadini 2016-11-10 06:43:01 PST Comment hidden (mozreview-request)
Comment 41 User image :Paolo Amadini 2016-11-10 06:46:53 PST
I had to change the code a bit to handle a few edge cases identified by regressions tests, for the rest most of the tests just had to be modified to use an actual page instead of about:blank.

I still need to write tests for invalid pageproxystate and the about:blank case.

I wonder if we should file a bug to also hide the notification if the awesomebar is open.
Comment 42 User image arni2033 2016-11-10 15:52:42 PST
What you are doing is breaking bug 1231025 even more.
Apparently, UX team haven't ever read that bug... Please read it.
I filed that bug for them to make a decision on what is the better way to remove the twitching, but instead I see more useless buttons being added to identity block, more twitching being implemented.
Comment 43 User image :Paolo Amadini 2016-11-14 05:15:51 PST
I've replied in bug 1231025 comment 5. Thanks!
Comment 44 User image :Paolo Amadini 2016-11-15 06:35:23 PST Comment hidden (mozreview-request)
Comment 45 User image :Paolo Amadini 2016-11-15 06:37:35 PST
Tryserver build, including all the previous notifications work:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12335468ae342b6847202295e3c9c98eab1543f7
Comment 46 User image Panos Astithas [:past] 2016-11-15 14:12:52 PST
Comment on attachment 8808200 [details]
Bug 1300755 - Notification anchors are not hidden when the user types in the location bar.

https://reviewboard.mozilla.org/r/91062/#review93270

::: browser/base/content/test/popupNotifications/browser.ini:19
(Diff revision 3)
>  skip-if = (os == "linux" && (debug || asan))
>  [browser_popupNotification_5.js]
>  skip-if = (os == "linux" && (debug || asan))
>  [browser_popupNotification_checkbox.js]
>  skip-if = (os == "linux" && (debug || asan))
> +[browser_popupNotification_no_anchors.js]

It looks that you forgot to add this file and the try builds failed.
Comment 47 User image :Paolo Amadini 2016-11-15 23:21:22 PST Comment hidden (mozreview-request)
Comment 48 User image :Paolo Amadini 2016-11-15 23:24:20 PST
(In reply to Panos Astithas [:past] from comment #46)
> It looks that you forgot to add this file and the try builds failed.

Also, not very useful for review :-)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6968d5b00442a2ce0920ed736e44dfef94e873c1
Comment 49 User image :Paolo Amadini 2016-11-16 02:44:31 PST
There are still failures in the tests, likely due to the way we wait for browser load events.
Comment 50 User image Panos Astithas [:past] 2016-11-17 09:33:07 PST
Comment on attachment 8808200 [details]
Bug 1300755 - Notification anchors are not hidden when the user types in the location bar.

https://reviewboard.mozilla.org/r/91062/#review93838

Works as advertised, thanks!

::: browser/base/content/browser.css
(Diff revision 4)
>  #urlbar[pageproxystate="invalid"] > #identity-box {
>    pointer-events: none;
>  }
>  
> -#urlbar[pageproxystate="invalid"] > #identity-box > #notification-popup-box {
> -  pointer-events: auto;

Isn't this necessary any more? pointer-events is an inherited property and we reset it in the rule above.

::: browser/base/content/browser.js:2387
(Diff revision 4)
>    // We didn't find a matching window, so open a new one.
>    return openDialog("chrome://browser/content/pageinfo/pageInfo.xul", "",
>                      "chrome,toolbar,dialog=no,resizable", args);
>  }
>  
> -function URLBarSetURI(aURI) {
> +function URLBarSetURI(aURI, aOptions = {}) {

This needs a comment to describe the available options. aURI is pretty obvious, but aOptions isn't.

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

Did you audit the tree for other call sites that need to be modified? I see about a dozen hits (mostly tests) and it's not clear from a quick glance which ones need to.
Comment 51 User image :Paolo Amadini 2016-12-13 03:42:01 PST
(In reply to Panos Astithas [:past] from comment #50)
> > -#urlbar[pageproxystate="invalid"] > #identity-box > #notification-popup-box {
> > -  pointer-events: auto;
> 
> Isn't this necessary any more? pointer-events is an inherited property and
> we reset it in the rule above.

I'm not sure what this comment means. This rule re-enabled the pointer events on the notification anchors, which is not necessary anymore now since these are now hidden.

> ::: browser/base/content/browser.js:4493
> > +      URLBarSetURI(aLocationURI, { isForLocationChange: true });
> 
> Did you audit the tree for other call sites that need to be modified? I see
> about a dozen hits (mostly tests) and it's not clear from a quick glance
> which ones need to.

Only this place actually sets the URI, but I'd rather have an explicit option than use the presence of aURI to determine whether this was called in response to a location change.
Comment 52 User image :Paolo Amadini 2016-12-13 03:43:28 PST Comment hidden (mozreview-request)
Comment 53 User image :Paolo Amadini 2016-12-13 03:44:42 PST
Let's see if some other test changes actually solve the timeouts.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b655951fc5be97d783c0bdfd16b040949af771d8
Comment 54 User image :Paolo Amadini 2016-12-13 07:29:47 PST
Looks like browser_popupNotification_4.js is now failing constantly on Mac and Windows, it might be related to the existing intermittent bug 1239930. The test case opens a new window, I suspect we might not be waiting enough for the window to be fully initialized.
Comment 55 User image :Paolo Amadini 2016-12-15 04:21:17 PST
This is wrong, right?

https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/browser/base/content/test/popupNotifications/browser_popupNotification_5.js#193

I'd expect the popup to be hidden in the first window while the second window is selected. I suspect this check only succeeds accidentally because of current timing.
Comment 56 User image :Paolo Amadini 2016-12-15 05:48:31 PST Comment hidden (mozreview-request)
Comment 57 User image :Paolo Amadini 2016-12-15 05:55:05 PST
The latest version has even more test refactoring to handle cases where we didn't wait enough when creating new windows or opening popup notifications. I'm not asking for review again since the only functional change is the removal of the check from comment 55, but the patch is now posted if you want to take a look.

Let's see if this solves the failures:

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

These changes might also solve the intermittent bug 1239930.
Comment 58 User image Panos Astithas [:past] 2016-12-15 08:27:34 PST
(In reply to :Paolo Amadini from comment #55)
> I'd expect the popup to be hidden in the first window while the second
> window is selected. I suspect this check only succeeds accidentally because
> of current timing.

Why would you expect that? A separate window doesn't reuse the URL bar like a separate tab does, which is what requires us to hide the notification in that case. Try opening two windows manually and observing a notification in the one in the background: it doesn't go away, because, honestly, why should it?
Comment 59 User image :Paolo Amadini 2016-12-15 09:35:02 PST
(In reply to Panos Astithas [:past] from comment #58)
> Why would you expect that? A separate window doesn't reuse the URL bar like
> a separate tab does, which is what requires us to hide the notification in
> that case. Try opening two windows manually and observing a notification in
> the one in the background: it doesn't go away, because, honestly, why should
> it?

Ah, I just assumed we hid panels on inactive windows because of possible z-order issues, but I see this isn't the case. What happens on Windows, maybe to work around these issues, is that the panel disappears momentarily when a new window is opened, to be displayed again soon.

This is perceivable when testing manually in the slower debug builds, and this is why the automated test fails. For some reason it stops running with the panel hidden in the original window, hence my wrong assumption.

I'll debug some more, in the worst case we may change what we check slightly, or skip the check only on Windows. The new try builds with this check removed have succeeded!
Comment 60 User image Panos Astithas [:past] 2016-12-15 10:21:27 PST
In that case I agree that keeping that check is really necessary. It verifies the current behavior, but from a UX POV I don't think we need to guarantee that the panel remains open.
Comment 61 User image Panos Astithas [:past] 2016-12-15 10:22:45 PST
> In that case I agree that keeping that check is really necessary.

Oops, I meant to say "_isn't_ really necessary".
Comment 62 User image Pulsebot 2016-12-17 04:49:39 PST
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1955f4507d13
Notification anchors are not hidden when the user types in the location bar. r=past
Comment 63 User image :Paolo Amadini 2016-12-17 04:50:00 PST
Thanks, I landed the current version without the check.
Comment 64 User image Phil Ringnalda (:philor) 2016-12-17 14:05:48 PST
https://hg.mozilla.org/mozilla-central/rev/1955f4507d13
Comment 65 User image Gerry Chang [:gchang] 2016-12-22 00:28:21 PST
Given the patch is huge and the issue has been there for a while, I'm going to won't fix this in Beta51.
Comment 66 User image Maruf Rahman[:mMARUF] 2016-12-22 07:15:34 PST
I have reproduced this bug with Nightly 51.0a1 (2016-09-06) on Windows 7, 64 Bit ! 

This bug's fix is verified with latest Nightly 

Build  ID  : 20161221030226
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
[bugday-20161221]
Comment 67 User image Panos Astithas [:past] 2016-12-23 07:01:00 PST
Thanks!
Comment 68 User image Cristian Comorasu [:CristiComo] 2016-12-27 07:01:11 PST
I reproduced this issue using Fx 51.0a1, build ID: 20160906030431, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 53.0a1, build ID:20161227030213, on Windows 10 x64, Ubuntu 14.04 LTS and Mac OS X 10.11.

Cheers!
Comment 69 User image Ryan VanderMeulen [:RyanVM] 2016-12-29 13:31:39 PST
Looks like a lot of the hugeness of this patch is just test changes. How do you feel about requesting Aurora approval on this, Paolo?
Comment 70 User image :Paolo Amadini 2016-12-30 23:51:03 PST
(In reply to Ryan VanderMeulen [:RyanVM] from comment #69)
> Looks like a lot of the hugeness of this patch is just test changes. How do
> you feel about requesting Aurora approval on this, Paolo?

The patch is not too complex except for the tests, but it's written on top of the changes we did early in Firefox 53, so it could require some non-trivial merging. Given that the state before the patch isn't too bad, I think we could wait one release and give this fix to users together with the Permission Notifications redesign.

Note You need to log in before you can comment on or make changes to this bug.