967391 - UITour: When opening a sub-view, annotations in the panels initial view should be hidden

Status: VERIFIED FIXED

(Firefox :: General, defect, P2)

Description

[Matthew N. [:MattN]]

+++ This bug was initially created as a clone of Bug #949659 +++

STR:

1) Visit the Australis update page
2) Start the tour and enter the step where the menu panel shows, highlighting 
   add-ons.
3) Within the menu panel, click on 'History' or 'Help'

Expected results:
- The annotations in the panel should hide

Actual results:
- The annotation remains visible in the wrong location.

(Quoting Zhenshuo Fang (:fang) - Firefox UX Team from bug 949659 comment #2)
> When you click on an item in the menu that has a
> subview (say history or help), the highlight is still at the same location,
> we should hide it.

Is it fine that if a user closes the subview without choosing an item or closing the menu (e.g. clicks help again) that the annotation doesn't reappear?

Comment 1

[Zhenshuo Fang (:fang) - Firefox UX Team]

Ideally if the user clicks back into the main menu from subview, the highlight should reappear at the same location. We should aim for that and see if it's too much engineering efforts.

Comment 2

[Matthew N. [:MattN]]

Attachment: v.1 Reuse onAppMenuHiding (renamed to hidePanelAnnotations) for ViewShowing. Hide only.

Here is a patch to simply hide the annotations when a subview opens. It doesn't implement comment 1 as I don't think it's worth the effort at this time (for Aurora v.1) and I also wonder if the user would find it in the way while they're exploring as they intentionally opened a subview so probably already saw the highlight.

Tests coming up.

Comment 3

[Matthew N. [:MattN]]

Attachment: v.1 + Tests - Reuse onAppMenuHiding (renamed to hidePanelAnnotations) for ViewShowing

Now with tests!

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8371923 [details] [diff] [review]
v.1 + Tests -  Reuse onAppMenuHiding (renamed to hidePanelAnnotations) for ViewShowing

Review of attachment 8371923 [details] [diff] [review]:
-----------------------------------------------------------------

Well that ended up being much more elegant that I was expecting :)

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8371923 [details] [diff] [review]
v.1 + Tests -  Reuse onAppMenuHiding (renamed to hidePanelAnnotations) for ViewShowing

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis UITour
User impact if declined: When a panel menu item is highlighted during the tour, clicking an item with a subview (e.g. history, help, bookmarks, developer), would leave the highlight floating out of place in its original position.
Testing completed (on m-c, etc.): Automated mochitest locally
Risk to taking this patch (and alternatives if risky): Very simply change listening for an event new to Australis that is unlikely to conflict with things. Worst case is probably that a highlight would hide prematurely which the user may not even think is a bug.
String or IDL/UUID changes made by this patch: None

(In reply to Blair McBride [:Unfocused] from comment #4)
> Well that ended up being much more elegant that I was expecting :)

Same for me :)

Comment 6

[Matthew N. [:MattN]]

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

Comment 7

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ae9d9ec82186

Comment 8

[Matthew N. [:MattN]]

Follow-up test fixes:
https://hg.mozilla.org/integration/fx-team/rev/243fe57d77dd
https://hg.mozilla.org/releases/mozilla-aurora/rev/91a57ce52263

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Luckily this was test-only bustage since our Aurora nightlies ran on a busted cset. We have tree rules that explicitly state that patches are to be on mozilla-central before being uplifted to the release branches. If you're going to ignore that rule and push anyway, you should at least wait until tests are passing or have a green Try run (which clearly wasn't the case here).

This isn't a theoretical issue - we could have very easily shipped busted code to our users because of this. Please respect the rules we have in place.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c326f427d377
https://hg.mozilla.org/mozilla-central/rev/243fe57d77dd

Comment 11

[Matthew N. [:MattN]]

I was watching the tree so I wasn't worried about shipping busted code.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

You don't control when the nightlies start. And the point still stands, the tree rules are explicit about it.

Comment 13

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

User Agents:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0

Verified as fixed on latest Nightly (build ID: 20140302030203) latest Aurora (build ID: 20140303004002).