1162750 - Reader View tour tool tip cannot be dismissed by clicking outside the door hanger

Status: RESOLVED WONTFIX

(Toolkit :: Reader Mode, defect)

Description

[Peter deHaan [:pdehaan]]

Attachment: Pocket__Pocket_Premium_and_Issues_·_Pocket_Firefox.png

## Steps to reproduce
1. Using the latest FF Nightly, browse The Glorious Internet until you find something that is reader-mode capable.
FWIW, I was on https://getpocket.com/premium?ep=2

## Actual results:
A Reader View door hanger appears.
You cannot click the door hanger to dismiss it.
You cannot click OUTSIDE the door hanger to dismiss it.
You cannot scroll the page to dismiss it.
Navigating to a different page dismisses door hanger.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: MozReview Request: Bug 1162750 - Reader View tour tool tip cannot be dismissed by clicking outside the door hanger. r?MattN,past

Review commit: https://reviewboard.mozilla.org/r/35405/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35405/

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8720634 [details]
MozReview Request: Bug 1162750 - Reader View tour tool tip cannot be dismissed by clicking outside the door hanger. r?MattN,past

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35405/diff/1-2/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Verdi, can you sign off on this change. The goal is to make these non-content driven UITours slightly less obtrusive.

Since the user didn't formally opt-in to these tours (they're more of a drive-by tour), we should make it a little easier to opt-out. For tours that are opt-in, like the Windows 10 tour that is initiated from content it is OK to keep those panels noautohide.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8720634 [details]
MozReview Request: Bug 1162750 - Reader View tour tool tip cannot be dismissed by clicking outside the door hanger. r?MattN,past

https://reviewboard.mozilla.org/r/35405/#review32223

Touching @noautohide, specifically changing it at run time can introduce many issues (that vary by platform unfortuntately). See the comment in UITour.recreatePopup https://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour.jsm#1821
IIRC that was mostly a problem with Linux so you should probably test there. For that same reason I think we need an automated test that alternates calling showInfo with autohide true and false and ensure that a synthesized click outside does the right thing after it's been toggled. If recreatePopup is still necessary then I think the test would fail on Linux. r- for the lack of test and likely required recreatePopup. Please use the add_UITour_task/add_task style of test instead of the custom UITour test harness with the `tests` array.

Please involve UX in this change as we may be biased into wanting to reduce the visility of this info panel due to seeing it often in testing profiles while regular users should only see it once in their only profile.

::: browser/components/uitour/UITour.jsm:1494
(Diff revision 1)
>     * @param {String}   [aOptions.targetCallback]
> +   * @param {Boolean}  [aOptions.autohide]
>     */

Nit: should it be camel case `autoHide`?

::: browser/components/uitour/UITour.jsm:1583
(Diff revision 1)
> +      tooltip.addEventListener("popuphidden", function tooltipHidden(event) {
> +        tooltip.removeEventListener("popuphidden", tooltipHidden);
> +        // noautohide attribute may have been removed by options.autohide
> +        tooltip.setAttribute("noautohide", "true");

Why did you decide to do this here instead of adding an else after `if (aOptions.autohide)`? I think that would be a bit cleaner keeping the logic together.

Note that I believe you will need a smart use of recreatePopup (for the attribute to take effect on some platforms) only if the attribute is actually changing. If you recreatePopup when it's not necessay then there will be a flicker in the cases where the info panel "moves" directly to a new position with (showInfo + showInfo) instead of a showInfo/hideInfo/showInfo (even when @noautohide is staying the same for both positions/anchors).

This will introduce a new defect wherein moving between anchors with a different `autohide` option will cause a flicker. This is probably fine in practice for current tours but I will CC agibson to make him aware of it.

Comment 6

[Matthew N. [:MattN]]

Note that your patch isn't actually exposing the autohide option to content as you didn't pass it along in UITour-lib.js.

I think that's fine if we want content tours to stay with the noautohide behaviour until we had a good use case to change that. Alex, just an FYI that we are going to remove @noautohide for tour panels not driven by webpages.

Comment 7

[Alex Gibson [:agibson]]

Just caught sight of my name here, thanks for the heads up Matt.

Just to be clear on what to expect here:

1.) UITour popups created by web content (e.g. www.mozilla.org) will still have the noautohide behavior
2.) There may be some flicker expected when moving between popups after this change, irrespective of noautohide.

Is this accurate?

As a side note, I would welcome changing the behavior of all UITour popups in the long term. In the majority of use cases we end up writing a lot of boilerplate code in the web content to deal with closing popups (e.g. clicking on the document, minimizing or resizing the window etc). It's also a great source of edge-case bugs like this. 

I guess the one thing that's been a blocker for doing this is making sure all current / future tours have some way to re-trigger the popup, should the user accidentally close it and have no way to restart/continue the tour again. We should probably re-group with Holly on this when we have a good use-case to address it.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8720634 [details]
MozReview Request: Bug 1162750 - Reader View tour tool tip cannot be dismissed by clicking outside the door hanger. r?MattN,past

https://reviewboard.mozilla.org/r/35405/#review32267

Looks good to me, but I would defer to Matt anyway as he is more experienced with the UI tour code.

Comment 10

[Verdi [:verdi] Best to slack me]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Verdi, can you sign off on this change. The goal is to make these
> non-content driven UITours slightly less obtrusive.
> 
> Since the user didn't formally opt-in to these tours (they're more of a
> drive-by tour), we should make it a little easier to opt-out. For tours that
> are opt-in, like the Windows 10 tour that is initiated from content it is OK
> to keep those panels noautohide.

I'm not sure this is the way to go about making it easier to close the reader mode tour. It's a one-time notification and if you make it dismiss by clicking outside of it, it's likely that the user will either not see it or notice it too late with no way to re-trigger it. I'm also concerned about being to implement things like https://www.mozilla.org/en-US/firefox/47.0a1/tour/ - when we first launched that, it hinged on the user seeing the doorhanger and we explicitly made it so that you had to click "not now" or the "x" to get rid of it.

(In reply to Alex Gibson [:agibson] from comment #7)
> 
> I guess the one thing that's been a blocker for doing this is making sure
> all current / future tours have some way to re-trigger the popup, should the
> user accidentally close it and have no way to restart/continue the tour
> again. We should probably re-group with Holly on this when we have a good
> use-case to address it.

Maybe Alex is saying something similar here?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Verdi [:verdi] from comment #10)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> > Verdi, can you sign off on this change. The goal is to make these
> > non-content driven UITours slightly less obtrusive.
> > 
> > Since the user didn't formally opt-in to these tours (they're more of a
> > drive-by tour), we should make it a little easier to opt-out. For tours that
> > are opt-in, like the Windows 10 tour that is initiated from content it is OK
> > to keep those panels noautohide.
> 
> I'm not sure this is the way to go about making it easier to close the
> reader mode tour. It's a one-time notification and if you make it dismiss by
> clicking outside of it, it's likely that the user will either not see it or
> notice it too late with no way to re-trigger it. I'm also concerned about
> being to implement things like
> https://www.mozilla.org/en-US/firefox/47.0a1/tour/ - when we first launched
> that, it hinged on the user seeing the doorhanger and we explicitly made it
> so that you had to click "not now" or the "x" to get rid of it.

The tour you linked to is different because it comes from content. Yes, we open that tab during launch but it's also a "launch" activity. This is before somebody may be knee-deep in their browsing. These changes would not affect that tour.

Comment 12

[Matthew N. [:MattN]]

(In reply to Alex Gibson [:agibson] from comment #7)
> Just caught sight of my name here, thanks for the heads up Matt.
> 
> Just to be clear on what to expect here:
> 
> 1.) UITour popups created by web content (e.g. www.mozilla.org) will still
> have the noautohide behavior

Correct

> 2.) There may be some flicker expected when moving between popups after this
> change, irrespective of noautohide.

No, the flicker is related to @noautohide. The flicker should only appear if a tour panel is already open then needs to change its @noautohide attribute before the next showInfo call. For example, a panel without @noautohide (e.g. TP tour step 1) and then the webpage opens step 2 with @noautohide (since it can't control this).

> As a side note, I would welcome changing the behavior of all UITour popups
> in the long term. In the majority of use cases we end up writing a lot of
> boilerplate code in the web content to deal with closing popups (e.g.
> clicking on the document, minimizing or resizing the window etc). It's also
> a great source of edge-case bugs like this. 

Well, we should really fix our popup code to handle a lot of these cases for us so you don't have to workaround them (bug 1207419). The popup/panel code really needs an owner. Perhaps some of that can be included in the quality of experience work?

> I guess the one thing that's been a blocker for doing this is making sure
> all current / future tours have some way to re-trigger the popup, should the
> user accidentally close it and have no way to restart/continue the tour
> again. We should probably re-group with Holly on this when we have a good
> use-case to address it.

I'm not sure what existing blocker there would be to re-triggering now. I thought that a new showInfo should re-open it if it was closed.

Comment 13

[Alex Gibson [:agibson]]

(In reply to Matthew N. [:MattN] (behind on bugmail) from comment #12)
> I'm not sure what existing blocker there would be to re-triggering now. I
> thought that a new showInfo should re-open it if it was closed.

I just meant we need to find the time/resources to do the UX & dev work to support this :)

Comment 14

[Verdi [:verdi] Best to slack me]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> The tour you linked to is different because it comes from content. Yes, we
> open that tab during launch but it's also a "launch" activity. This is
> before somebody may be knee-deep in their browsing. These changes would not
> affect that tour.

Ok cool. 

So still trying to understand the implications here. This change would hide the reader view tour if you click outside of it or interact with the page in some way? What other tours would this affect?

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

This would also affect the upcoming Tracking Protection tour, which is triggered when a page is visited that has elements blocked.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Verdi [:verdi] from comment #14)
> This change would hide
> the reader view tour if you click outside of it or interact with the page in
> some way?

That's correct.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

We're gonna skip on fixing this for the time being until we have a central place where users can see what notifications Firefox has shown them recently. Activity Stream will likely be that place, but it's not ready yet and likely still shifting around during development.

Comment 18

[Justin Dolske [:Dolske]]

FTR - I agree with some of the concerns here. It's a popup that interrupts the user in the middle of their task, and that's not great. But I also acknowledge that it's probably the best we have right now, given that it's unexpectedness means it can be easily accidentally dismissed if the user happens to click as part of whatever they were doing.

I do think this means we should avoid adding more of these, until we figure out a better mechanism for this kind of thing (e.g. by either introducing new features at a different time, or doing so in a way that better balances interrupting the user with persistence).

I also think it would be worthy to gather more data on how effective these tours are before doing more with them.

Comment 19

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Justin Dolske [:Dolske] from comment #18)
> FTR - I agree with some of the concerns here. It's a popup that interrupts
> the user in the middle of their task, and that's not great. But I also
> acknowledge that it's probably the best we have right now, given that it's
> unexpectedness means it can be easily accidentally dismissed if the user
> happens to click as part of whatever they were doing.
> 
> I do think this means we should avoid adding more of these, until we figure
> out a better mechanism for this kind of thing (e.g. by either introducing
> new features at a different time, or doing so in a way that better balances
> interrupting the user with persistence).
> 
> I also think it would be worthy to gather more data on how effective these
> tours are before doing more with them.

Without disagreeing with any of that, I'll just add one point: While it does aim to draw people's attention, it doesn't necessarily interrupt. We know that users are happily ignoring Chromes permission prompts (which have similar size and behavior) and just keep browsing.