1177176 - Support opening the Tracking Protection panel via showMenu

Status: RESOLVED FIXED

(Firefox :: Tours, defect, P1)

Description

[Matthew N. [:MattN]]

We may want to anchor a tour panel on the Tracking Protection panel for the first run experience so we should be able to open the panel first.

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1177176 - Support opening the Control Center panel via UITour.showMenu. r=bgrins

Bug 1177176 - Support opening the Tracking Protection panel via UITour.showMenu.

Comment 2

[Matthew N. [:MattN]]

Hey agrigas,

Should the 3 tour info panels and the control center panel in step 3 be "sticky" (@noautohide) meaning that a click outside the panel won't automatically hide it and the user will have to use the [X] for the most part? The default of panels in Firefox is "no" but because past tours had the tour navigation (back/next step) in the page content, we made the panels sticky so that a click on the arrow in page content to go to the next step doesn't close a panel that may need to be immediately re-opened for the next step. With the current designs, the tour step navigation is in the panel itself so this may not be necessary.

Making a panel sticky is more annoying to users since they have to more explicitly close them and they're used to the non-sticky heaviour. Since "sticky" panels are rarely used outside of the tours, they're also more buggy (especially on Linux) and cause unusual operating system-specific bugs in some cases.

See the tour from the Firefox Help menu for an example of the sticky behaviour which differs from the behaviour of the control center panel by default.

We can try either option but I would lean towards non-sticky (default behaviour) unless you feel strongly about making them sticky.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8635102 [details]
MozReview Request: Bug 1177176 - Support opening the Control Center panel via UITour.showMenu. r=bgrins

Bug 1177176 - Support opening the Tracking Protection panel via UITour.showMenu. r=bgrins

Comment 4

[Ash Grigas]

(In reply to Matthew N. [:MattN] from comment #2)
> Hey agrigas,
> 
> Should the 3 tour info panels and the control center panel in step 3 be
> "sticky" (@noautohide) meaning that a click outside the panel won't
> automatically hide it and the user will have to use the [X] for the most
> part? The default of panels in Firefox is "no" but because past tours had
> the tour navigation (back/next step) in the page content, we made the panels
> sticky so that a click on the arrow in page content to go to the next step
> doesn't close a panel that may need to be immediately re-opened for the next
> step. With the current designs, the tour step navigation is in the panel
> itself so this may not be necessary.
> 
> Making a panel sticky is more annoying to users since they have to more
> explicitly close them and they're used to the non-sticky heaviour. Since
> "sticky" panels are rarely used outside of the tours, they're also more
> buggy (especially on Linux) and cause unusual operating system-specific bugs
> in some cases.
> 
> See the tour from the Firefox Help menu for an example of the sticky
> behaviour which differs from the behaviour of the control center panel by
> default.
> 
> We can try either option but I would lean towards non-sticky (default
> behaviour) unless you feel strongly about making them sticky.

I would prefer sticky as that is the behavior we have designed for the control panel notifications for 43. For example when asked to accept a permission, we're changing it from unsticky to having the user engage with the notification since the ctr is so low for notifications. I think especially given we want people to finish the tour, that having them explicitly dismissed via an X is important.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8635102 [details]
MozReview Request: Bug 1177176 - Support opening the Control Center panel via UITour.showMenu. r=bgrins

https://reviewboard.mozilla.org/r/13507/#review12175

Ship It!

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8635102 [details]
MozReview Request: Bug 1177176 - Support opening the Control Center panel via UITour.showMenu. r=bgrins

Bug 1177176 - Support opening the Control Center panel via UITour.showMenu. r=bgrins

Comment 7

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1177176 - Make the Control Center panel sticky when opened via UITour.showMenu. r=bgrins

Bug 1177176 - Make the Control Center panel sticky when opened via UITour.showMenu. r=bgrins

@noautohide makes the panel sticky like we do for the appMenu and loop.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8635609 [details]
MozReview Request: Bug 1177176 - Make the Control Center panel sticky when opened via UITour.showMenu. r=bgrins

https://reviewboard.mozilla.org/r/13573/#review12189

::: browser/components/uitour/UITour.jsm:1577
(Diff revision 1)
> +      } else {

Nit: no need for an else after the early return

::: browser/components/uitour/UITour.jsm:1570
(Diff revision 1)
> +      popup.setAttribute("noautohide", true);

The `this.availableTargetsCache.clear()` call should move up to right below the "noautohide" stuff and outside of the two conditionals

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8635609 [details]
MozReview Request: Bug 1177176 - Make the Control Center panel sticky when opened via UITour.showMenu. r=bgrins

https://reviewboard.mozilla.org/r/13573/#review12191

Ship It!

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8635102 [details]
MozReview Request: Bug 1177176 - Support opening the Control Center panel via UITour.showMenu. r=bgrins

https://reviewboard.mozilla.org/r/13507/#review12193

Ship It!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1077ce98bd44
https://hg.mozilla.org/integration/fx-team/rev/850d33233360

Comment 12

[:Paolo Amadini]

It seems the nsIUrlClassifierDBService.beginUpdate intermittent failure happens also on platforms other than Linux, maybe less frequently, see for example this try run that includes the changesets above:

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

Comment 13

[Matthew N. [:MattN]]

I think you meant that comment for bug 1177162 and I'm not surprised about that but couldn't reproduce on try with my tweaked patch including logging. I think we'll end up dealing with it somehow in a new bug for the intermittent.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1077ce98bd44
https://hg.mozilla.org/mozilla-central/rev/850d33233360