1232663 - Hello panel remains visible when selecting Sign In option via Settings gear button

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P3)

Description

[Ada [:adalucinet]]

Reproducible with 43.0b9 (Build ID: 20151203163240), latest Developer Edition 44.0a2 and Nighty 45.0a1 (both from 2015-12-14)
Affected platforms: Windows 7 64-bit, Windows 10 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.11

Steps to reproduce:
1. Start Firefox with a clean profile.
2. Click Hello icon.
3. Click Get Started button.
4. Click Settings button via Hello panel and select Sign In option.
5. Repeat step 4.

Expected results: Hello panel is dismissed.

Actual results: Hello panel remains visible.

Additional notes:
1. Screen recording showing the issue: https://goo.gl/Mkcfgx
2. Also reproducible with Nightly from 2015-05-01 - will investigate further.
3. At step 4, the Hello panel is properly dismissed.

Comment 1

[Ada [:adalucinet]]

This issue is reproducible including with Nightly from 2014-10-17, therefore is not a regression.

Comment 2

[Vidhuran Harichandra Babu]

Attachment: rudimentary patch

I stumbled upon another bug which i was trying to investigate (Bug 1235560) and figured out there could be a common fix for both this and the linked bug.
I've now made a rudimentary patch that would fix this issue. I can make a proper one if the desired behaviour is that the panel should be closed.

Comment 3

[Mark Banner (:standard8)]

(In reply to Vidhuran Harichandra Babu from comment #2)
> I stumbled upon another bug which i was trying to investigate (Bug 1235560)
> and figured out there could be a common fix for both this and the linked bug.
> I've now made a rudimentary patch that would fix this issue. I can make a
> proper one if the desired behaviour is that the panel should be closed.

Yes, this is exactly the right thing to do for this bug. I think it would also be enough to fix your other bug as well.

Comment 4

[GitHub Autolander Bot]

Attachment: [loop] Vidhuran:bug-1232663-settings-menu-fix > mozilla:master

Comment 5

[Mark Banner (:standard8)]

Vidhuran: is this ready for review, if so please request it. I can look at it on Monday.

Comment 6

[Vidhuran Harichandra Babu]

Attachment: Link to Github pull-request: https://github.com/mozilla/loop/pull/37

Comment 7

[Vidhuran Harichandra Babu]

(In reply to Mark Banner (:standard8) from comment #5)
> Vidhuran: is this ready for review, if so please request it. I can look at
> it on Monday.

Yes, It is now ready for review. I've made a request for it.

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8703179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/37

(In reply to Mark Banner (:standard8) from comment #3)
> (In reply to Vidhuran Harichandra Babu from comment #2)
> > I stumbled upon another bug which i was trying to investigate (Bug 1235560)
> > and figured out there could be a common fix for both this and the linked bug.
> > I've now made a rudimentary patch that would fix this issue. I can make a
> > proper one if the desired behaviour is that the panel should be closed.
> 
> Yes, this is exactly the right thing to do for this bug. I think it would
> also be enough to fix your other bug as well.

Sorry about this, now I've played with the patch a bit, I think it would feel much better for the sign-out case just to close the settings menu. The panel gets reset correctly and next time the menu is opened it is updated. I think this will be much better for the users as it'll provide visual feedback that the sign-out has happened.

So in summary, could we make it:

- Sign-in: Hides the menu and closes the panel.
- Sign-out: hides the menu, leaves the panel open.

Comment 9

[Vidhuran Harichandra Babu]

(In reply to Mark Banner (:standard8) from comment #8)
> Comment on attachment 8703179 [details] [review]
> Link to Github pull-request: https://github.com/mozilla/loop/pull/37
> 
> (In reply to Mark Banner (:standard8) from comment #3)
> > (In reply to Vidhuran Harichandra Babu from comment #2)
> > > I stumbled upon another bug which i was trying to investigate (Bug 1235560)
> > > and figured out there could be a common fix for both this and the linked bug.
> > > I've now made a rudimentary patch that would fix this issue. I can make a
> > > proper one if the desired behaviour is that the panel should be closed.
> > 
> > Yes, this is exactly the right thing to do for this bug. I think it would
> > also be enough to fix your other bug as well.
> 
> Sorry about this, now I've played with the patch a bit, I think it would
> feel much better for the sign-out case just to close the settings menu. The
> panel gets reset correctly and next time the menu is opened it is updated. I
> think this will be much better for the users as it'll provide visual
> feedback that the sign-out has happened.
> 
> So in summary, could we make it:
> 
> - Sign-in: Hides the menu and closes the panel.
> - Sign-out: hides the menu, leaves the panel open.

I've made the changes as you had suggested.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8703179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/37

This looks great. I like how the new flow works - much better.

There's a couple of test improvements I'd like to land with this - I've commented on the PR. If you could address those and rebase on latest master, we can get it merged.

Comment 11

[Vidhuran Harichandra Babu]

Requesting review again as I've now made some changes based on the previous review comments

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8703179 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/37

That's great, thanks for the updates.

Comment 13

[Mark Banner (:standard8)]

https://github.com/mozilla/loop/commit/cd65b84d9241516d04f41e7a388ff069189ce870
https://github.com/mozilla/loop/commit/6bef30066e2f3df41a853d5501daf4ef52137fec

Comment 14

[Ada [:adalucinet]]

Verified fixed with latest 46.0a1 (from 2016-01-19), across platforms [1]:
* when Sign in is selected, both Hello panel and Settings menu are dismissed
* when Sign out is selected, Settings menu is dismissed and Hello panel remains visible

[1] Ubuntu 14.04 64-bit, Mac OS X 10.10.5 and Windows 7 64-bit