1207542 - The Control Center panel remains in focus after the user navigates to a previous page

Status: VERIFIED FIXED

(Firefox :: General, defect, P1)

Description

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced with:
* Firefox 42 beta 1
* latest 43.0a2 Aurora
* latest 44.0a1 Nightly

STR:
1. Open Firefox.
2. In a private window, navigate to http://cnn.com or any other page.
3. Click on the site identity panel.
4. Focus (press "Tab" or mouse click in) the panel.
5. Press backspace.

Expected Results:
The user is redirected to the previous page and the identity panel is closed.

Actual results:
The identity panel is still displayed, even if the user is returned to the previous page.

Screenshot:
http://i.imgur.com/jNMedf4.png

Comment 1

[Tanvi Vyas[:tanvi]]

This happens in regular mode and private browsing mode.

Comment 2

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned.

Comment 3

[Brian Grinstead [:bgrins]]

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

Comment 4

[Brian Grinstead [:bgrins]]

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

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1207542 - Hide the Control Center when the URL changes;r=paolo

Bug 1207542 - Hide the Control Center when the URL changes;r=paolo

Comment 6

[:Paolo Amadini]

Tim, do you remember if we used to close the panel on navigation and then changed the behavior? If we did that change, do you remember the use case?

The only thing that could happen here is that the page navigates away or changes the hash part of the URI, and you cannot see the details of the previous page anymore, but I don't know how often that happens, and thinking about that I don't see how this can be an issue, even if the panel included more advanced interactions like passwords.

Comment 7

[:Paolo Amadini]

Also, if the page is the same but its security properties change, for example following a subframe navigation, I wonder if at this point we should update the panel contents?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

We do not update the panel contents intentionally upon navigation, see my comment in bug 1208483. When the user decides to navigate away then we should probably indeed close the panel, but there are so many ways of doing that (probably) that this might turn into a whack-a-mole game.

Comment 9

[:Paolo Amadini]

(In reply to Tim Taubert [:ttaubert] from comment #8)
> When the user decides to navigate away then we
> should probably indeed close the panel, but there are so many ways of doing
> that (probably) that this might turn into a whack-a-mole game.

Agreed for the desired behavior of navigations initiated by the user. The question is mostly what to do with navigations that are initiated by the page. Should we close the panel in that case, instead of keeping it displayed? Does this have any side effect? In the past, was there a time when we closed the panel, or instead we always just updated its contents (before we decided to keep the current contents)?

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

I'm quite sure we just updated it but didn't close it. If I want to look at a site's connection details and we automatically close the panel due to a redirect or similar that would quite annoy me and doesn't feel right, but I think that's maybe something where UX should weigh in too. OTOH, I think a page can detect when it loses focus and could then keep navigating until it regains focus to prevent the user from seeing connection details. Not sure if that's something we care about or if that's really possible to do, just thinking out loud.

Comment 11

[:Paolo Amadini]

Comment on attachment 8672914 [details]
MozReview Request: Bug 1207542 - Hide the Control Center when the URL changes;r=paolo

We should clarify the intended behavior before proceeding with code review.

Comment 12

[Brian Grinstead [:bgrins]]

Ash, there is some discussion going on in the bug here about the intended behavior for the Control Center's visibility/content when a page is navigated.  Generally the CC is hidden when navigating because the panel gets closed when the user clicks a link or submits a form in the page.  But there are cases where it's opened during navigation, like:

1) CC is focused and the user presses backspace (this bug)
2) CC is opened and the page does a navigation in the background with JavaScript
3) CC is opened during a page navigation (i.e. click a link and then quickly click on the ID block to open CC.

The question is, in these cases what should happen?  Having it remain open with the old information seems wrong (as evidenced by this bug and bug 1208483).  Other options would be to hide the CC (my current approach in this bug) or to go ahead and update the contents of the panel with the new data.

FYI it's going to be hard to distinguish between cases 1, 2, and 3 so we can hopefully have consistent behavior across all of those.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> The question is, in these cases what should happen?  Having it remain open
> with the old information seems wrong (as evidenced by this bug and bug
> 1208483).

I disagree, just because someone filed bugs doesn't mean it's wrong. When I click the identity area of the url bar then I'm interested in the *current* site's security state. Removing that information upon navigation is IMHO bad behavior. FWIW, Chrome does not update the information shown either, the last time I checked.

Comment 14

[Ash Grigas]

So I would say that if the user clicks elsewhere, theres pretty strong intent they are done viewing the control center. The behavior of the control center is fairly similar to a menu in the toolbar on an desktop OS in that if the user clicks somewhere else it closes. Same would be true if they navigate. 

I think its not very realistic to think case #3 will be very common. For case #2, if the user doesn't navigate and the page does for them, I would think they would expect the control center would stay open. 

How common is case #2?

Comment 15

[Brian Grinstead [:bgrins]]

There are also bugs when you get into a state like this.  For instance, when you click 'More Information' it ends up opening the Page Info dialog on the *new* page even though the security info in the CC is referring to the old one.  I imagine there will be similar bugs with permission controls and tracking protection controls.

Comment 16

[Ash Grigas]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> There are also bugs when you get into a state like this.  For instance, when
> you click 'More Information' it ends up opening the Page Info dialog on the
> *new* page even though the security info in the CC is referring to the old
> one.  I imagine there will be similar bugs with permission controls and
> tracking protection controls.

Closing the panel on user navigation should resolve those though, correct?

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to agrigas from comment #16)
> (In reply to Brian Grinstead [:bgrins] from comment #15)
> > There are also bugs when you get into a state like this.  For instance, when
> > you click 'More Information' it ends up opening the Page Info dialog on the
> > *new* page even though the security info in the CC is referring to the old
> > one.  I imagine there will be similar bugs with permission controls and
> > tracking protection controls.
> 
> Closing the panel on user navigation should resolve those though, correct?

Yes, although as I mentioned earlier I'm not sure if we can distinguish between user navigation and page navigation (case #2).  To answer your earlier question about how common that is, I'm not sure.  As an example, I know some news sites have a landing page that will hold you there for N seconds and then redirect to the article.

Comment 18

[Ash Grigas]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> (In reply to agrigas from comment #16)
> > (In reply to Brian Grinstead [:bgrins] from comment #15)
> > > There are also bugs when you get into a state like this.  For instance, when
> > > you click 'More Information' it ends up opening the Page Info dialog on the
> > > *new* page even though the security info in the CC is referring to the old
> > > one.  I imagine there will be similar bugs with permission controls and
> > > tracking protection controls.
> > 
> > Closing the panel on user navigation should resolve those though, correct?
> 
> Yes, although as I mentioned earlier I'm not sure if we can distinguish
> between user navigation and page navigation (case #2).  To answer your
> earlier question about how common that is, I'm not sure.  As an example, I
> know some news sites have a landing page that will hold you there for N
> seconds and then redirect to the article.

Ok, I would say we go forward then with closing on user navigation and breakout a seperate bug for if the page navigates itself to see if we can better detect that. I think that one case is odd and will be jarring to users if they intentionally opened the CC and then it closes without their interaction.

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8672914 [details]
MozReview Request: Bug 1207542 - Hide the Control Center when the URL changes;r=paolo

It's not ideal to be showing the non-current security info and fixing bugs related to that (with permission controls and the 'more info' button, for example).  So re-requesting review with the idea that we'll close the CC on a navigation as per Comment 18

Comment 20

[:Paolo Amadini]

Comment on attachment 8672914 [details]
MozReview Request: Bug 1207542 - Hide the Control Center when the URL changes;r=paolo

https://reviewboard.mozilla.org/r/21819/#review19899

::: browser/base/content/browser.js:7014
(Diff revision 1)
> +    let uriChanged = this._uri && (this._uri.spec != uri.spec);

You might be able to use nsIURI.equals, or maybe even nsIURI.equalsExceptRef.

Note that the first time this is called, uriChanged will be "false" although it did change from null to the initial value. Maybe we should name this variable differently?

::: browser/base/content/browser.js:7047
(Diff revision 1)
>  
>      // NOTE: We do NOT update the identity popup (the control center) when
>      // we receive a new security state. If the user opened the popup and looks
>      // at the provided information we don't want to suddenly change the panel
>      // contents.

This comment doesn't apply anymore.

::: browser/base/content/test/general/browser_identity_UI.js:96
(Diff revision 1)
> +    // If currently on about:blank or loading the same URL that is already
> +    // loaded, then  don't need to test any Control Center visibility (Bug 1207542).

I believe the comment should describe what we're testing first, and then maybe what we're not testing :-)

::: browser/base/content/test/general/browser_identity_UI.js:108
(Diff revision 1)
> +        // Show the subview and make sure it closes when the URL changes (Bug 1207542).

I'd clarify that the test is not specifically about the subview but about the panel. I guess you added this call just for the test to be more thorough.

Comment 21

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #20)
> >      // NOTE: We do NOT update the identity popup (the control center) when
> >      // we receive a new security state. If the user opened the popup and looks
> >      // at the provided information we don't want to suddenly change the panel
> >      // contents.
> 
> This comment doesn't apply anymore.

Sorry, it does apply when the security state changes for the currently loaded page (for example a subframe). Worth clarifying.

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #20)
> Comment on attachment 8672914 [details]
> MozReview Request: Bug 1207542 - Hide the Control Center when the URL
> changes;r=paolo
> 
> https://reviewboard.mozilla.org/r/21819/#review19899
> 
> ::: browser/base/content/browser.js:7014
> (Diff revision 1)
> > +    let uriChanged = this._uri && (this._uri.spec != uri.spec);
> 
> You might be able to use nsIURI.equals, or maybe even nsIURI.equalsExceptRef.

I copied this bit from onSecurityChange, which calls this function: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4534-4539, so would prefer to leave it as-is and maybe we can update both at the same time in a separate bug.

> 
> Note that the first time this is called, uriChanged will be "false" although
> it did change from null to the initial value. Maybe we should name this
> variable differently?

Good idea, switching to shouldHidePopup.  Also, updating all of the comments as you pointed out.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bfb10d470ef4

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bfb10d470ef4

Comment 25

[Paul Silaghi, QA [:pauly]]

Control Center dismisses automatically when opened and:
1. I press backspace or alt+right/left keys to navigate back/forward
2. the page redirects automatically to another page after a couple of seconds
Verified fixed FF 44.0a1 (2015-10-18) Win 7, Ubuntu 14.04, OS X 10.10.5

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8672914 [details]
MozReview Request: Bug 1207542 - Hide the Control Center when the URL changes;r=paolo

Approval Request Comment
[Feature/regressing bug #]: Control Center / 1175702
[User impact if declined]: The control center's information can get out of date with the current page - see Comment 0 / http://i.imgur.com/jNMedf4.png
[Describe test coverage new/current, TreeHerder]: There's new bc test coverage to make sure it is being hidden on location changes
[Risks and why]: We didn't used to hide the popup on location changes because we didn't want to interrupt what the user was reading, but decided to go with this behavior to prevent misleading info / triggering other bugs in the panel.
[String/UUID change made/needed]:

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking 42+ as affected.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8672914 [details]
MozReview Request: Bug 1207542 - Hide the Control Center when the URL changes;r=paolo

Includes browser chrome tests, small change for feature behavior. OK to uplift to aurora.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a63f9c56503

Comment 30

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 43b1 Win 7.