Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Notification anchors are not hidden when the user types in the location bar

VERIFIED FIXED in Firefox 53

Status

()

Firefox
Site Identity and Permission Panels
P2
normal
VERIFIED FIXED
11 months ago
4 months ago

People

(Reporter: johannh, Assigned: Paolo)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {regression})

Trunk
Firefox 53
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 unaffected, firefox49 wontfix, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox53 verified)

Details

(Whiteboard: [fxprivacy])

MozReview Requests

()

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 months ago
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.
(Reporter)

Updated

11 months ago
Summary: Notification anchors are not hidden when the searchbar is focused → Notification anchors are not hidden when the user types in the location bar
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.
(Reporter)

Comment 2

11 months ago
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.
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.
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

11 months ago
Not a blocker for 49. NI UX for insight.
Flags: needinfo?(philipp)
Priority: -- → P2
Whiteboard: [fxprivacy] [triage] → [fxprivacy][triage]
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").
Too late for 49, but we could still take changes for 50.
status-firefox49: affected → wontfix
(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.
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.
Flags: needinfo?(philipp)

Updated

10 months ago
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Comment hidden (mozreview-request)
(Reporter)

Updated

10 months ago
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
(Reporter)

Comment 11

10 months ago
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?
Flags: qe-verify+

Comment 12

10 months ago
mozreview-review
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.
Attachment #8791189 - Flags: review?(florian) → review-
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.
(Reporter)

Comment 14

10 months ago
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 hidden (mozreview-request)

Comment 16

10 months ago
mozreview-review
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)
Attachment #8791189 - Flags: review?(florian) → review+
(Reporter)

Comment 17

10 months ago
(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 hidden (mozreview-request)
(Reporter)

Comment 19

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bf25e32e6de
(Reporter)

Comment 20

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae959c911916

Updated

10 months ago
Iteration: --- → 52.1 - Oct 3
(Reporter)

Comment 21

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cac1a139b43
(Reporter)

Comment 22

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=76ad20aaacbc

Updated

10 months ago
Whiteboard: [fxprivacy] → [fxprivacy][triage]
(Assignee)

Comment 23

10 months ago
(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.)
(Assignee)

Comment 24

10 months ago
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.

Updated

10 months ago
Iteration: 52.1 - Oct 3 → ---
Whiteboard: [fxprivacy][triage] → [fxprivacy]
(Assignee)

Updated

10 months ago
Flags: needinfo?(philipp)
(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.
(Reporter)

Comment 26

10 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 29

10 months ago
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 :)
(Reporter)

Comment 30

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b2a0dc10df7
(Assignee)

Comment 31

10 months ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 32

10 months ago
(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?
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

10 months ago
Attachment #8791189 - Flags: review+
(Assignee)

Comment 33

10 months ago
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.
Attachment #8798167 - Flags: review?(paolo.mozmail)
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.
Flags: needinfo?(philipp)
Panos, given Johann's partially unresponsive status, is there someone else that could implement Philipp's suggestion in comment 34?
Flags: needinfo?(past)
Paolo, can you pick this one up since you have already reviewed it?
Flags: needinfo?(past) → needinfo?(paolo.mozmail)
(Assignee)

Updated

9 months ago
Assignee: jhofmann → paolo.mozmail
Flags: needinfo?(paolo.mozmail)

Updated

9 months ago
Iteration: --- → 52.3 - Nov 7

Comment 37

9 months ago
we are a week away from RC, too late, this is now a wontfix for 50.
status-firefox50: affected → wontfix
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8791189 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8798167 - Attachment is obsolete: true
(Assignee)

Comment 39

9 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 41

9 months ago
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

9 months ago
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.
Depends on: 1231025
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 43

8 months ago
I've replied in bug 1231025 comment 5. Thanks!
Flags: needinfo?(paolo.mozmail)

Updated

8 months ago
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
Comment hidden (mozreview-request)
(Assignee)

Comment 45

8 months ago
Tryserver build, including all the previous notifications work:

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

Comment 46

8 months ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Comment 48

8 months ago
(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
(Assignee)

Comment 49

8 months ago
There are still failures in the tests, likely due to the way we wait for browser load events.

Comment 50

8 months ago
mozreview-review
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.
Attachment #8808200 - Flags: review?(past) → review+

Updated

8 months ago
Iteration: 53.1 - Nov 28 → 53.2 - Dec 12
(Assignee)

Comment 51

7 months ago
(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 hidden (mozreview-request)
(Assignee)

Comment 53

7 months ago
Let's see if some other test changes actually solve the timeouts.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b655951fc5be97d783c0bdfd16b040949af771d8
(Assignee)

Comment 54

7 months ago
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.
(Assignee)

Comment 55

7 months ago
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.
Flags: needinfo?(past)
Comment hidden (mozreview-request)
(Assignee)

Comment 57

7 months ago
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.
Blocks: 1239930
(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?
Flags: needinfo?(past)
(Assignee)

Comment 59

7 months ago
(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!
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.
> In that case I agree that keeping that check is really necessary.

Oops, I meant to say "_isn't_ really necessary".

Comment 62

7 months ago
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
(Assignee)

Comment 63

7 months ago
Thanks, I landed the current version without the check.
https://hg.mozilla.org/mozilla-central/rev/1955f4507d13
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

7 months ago
Iteration: 53.2 - Dec 12 → 53.3 - Dec 26
Given the patch is huge and the issue has been there for a while, I'm going to won't fix this in Beta51.
status-firefox51: affected → wontfix

Updated

7 months ago
status-firefox52: --- → affected
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]
Thanks!
Status: RESOLVED → VERIFIED
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!
status-firefox53: fixed → verified
Flags: qe-verify+
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?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 70

7 months ago
(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.
Flags: needinfo?(paolo.mozmail)
(Assignee)

Updated

7 months ago
status-firefox52: affected → wontfix

Updated

7 months ago
Depends on: 1328304
(Assignee)

Updated

5 months ago
Depends on: 1340538
Depends on: 1347627
You need to log in before you can comment on or make changes to this bug.