941862 - UITour: Fire an event to notify when the menu panel opens while the menu button is an info panel target

Status: RESOLVED FIXED

(Firefox :: General, defect, P3)

Description

[Matthew N. [:MattN]]

The page could start the tour when the menu panel is opened.

Comment 1

[Matthew N. [:MattN]]

Holly, is this something you'd like to use in Firefox 29 or can it wait?

Comment 2

[Matthew N. [:MattN]]

The following STR should be considered with this fix as it could lead to annotations appearing behind the menu panel on Linux (see bug 966072):

1) The user opens the menu panel on their own (noautohide doesn't get set and recreatePopup doesn't get called).
2) The page triggers a highlight on an item in the menu panel (e.g. based on the event fired from this bug fix).

Since the panel was already open, recreatePopup didn't get called but it might need to be in order to be positioned above the menu panel (at least on Linux).

We may want to always listen for popupshowing while the tour tab is selected and add @noautohide and call recreatePopup but we should check with UX. I can imagine tours that are unrelated to the menu so it may be weird to have it stay open when the user opens it. 

Otherwise, we could allow the flicker to happen this one time but remember that we already handled it so subsequent showMenu calls don't cause another flicker when the noautohide attribute is already correct. showMenu => showHighlight("help") => showMenu => showHighlight("help")
We can simply check if @noautohide already exists and decide whether to recreatePopup.

Comment 3

[Matthew N. [:MattN]]

It's been almost 2 weeks so I'm lowering the priority based on my judgement.

Comment 4

[Matthew N. [:MattN]]

habber/fang pointed out that we only need to do this when the menu button is an annotation target and so we wouldn't add noautohide if the user opens the menu in a tour unrelated to the menu button.

Also, I had an idea to avoid the flicker:
Prevent the menu from opening on the users click but then have UITour open it for them with the recreatePopup stuff. I don't like preventing the panel menu from opening as it seems a bit scary but I don't have a better solution.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

IMO, we're spending too much time on the flicker issue - considering it's a small issue, it's Linux-only (small user base, and I'd expect them to use the tour less - haven't checked the data), and it's not something people will see that often. So I think we need to just ignore it for now. We can potentially re-consider it in another cycle.

So, without that flicker issue, my understanding is that this bug should be a relatively simple fix.

Comment 6

[Matthew N. [:MattN]]

Attachment: v.1 Support a callback to notify when there is a mousedown on an info panel anchor

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8393314 [details] [diff] [review]
v.1 Support a callback to notify when there is a mousedown on an info panel anchor

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

::: browser/modules/UITour.jsm
@@ +945,5 @@
> +        };
> +        this.sendPageCallback(aContentDocument, aOptions.targetCallbackID, details);
> +      };
> +      if (aOptions.targetCallbackID) {
> +        aAnchor.node.addEventListener("mousedown", targetCallback);

Hm, this misses a few cases of user interaction. Such as keyboard-based focus navigation, and shortcut keys (especially ctrl-tab for the pinned tab). If the tour is going to be expecting to be able to detect that, and it *doesn't*, the person taking the tour is going to be stuck if they don't use the mouse.

If we were only doing the panel menu, I'd say we should just listen for the popupshown event - because that's the definite source of truth.

If we are expending it to all targets, we have four cases:
* Menu buttons: need to listen for popupshown events
* Text input widgets (urlbar, search bar): need to listen for focus events
* Pinned tab: need to listen for TabSelect event
* Everything else: pretty sure just listening for the command event should be enough

Comment 8

[Matthew N. [:MattN]]

Attachment: v.2 Let each target specify its own event listener for callbacks

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8396203 [details] [diff] [review]
v.2 Let each target specify its own event listener for callbacks

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

Ship it!

And I guess file a bug for eventually adding these to the other targets, if someone ever gets bored enough to want to fix em up?

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, and a reminder to submit a pull request on Github for mozilla-uitour.js - so easy to forget :\

Comment 11

[Matthew N. [:MattN]]

Thanks, I'll address comment 9 and 10 tomorrow after some sleep.

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

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8396203 [details] [diff] [review]
v.2 Let each target specify its own event listener for callbacks

[Approval Request Comment]
Bug caused by (feature/regressing bug #): UITour feature
User impact if declined: If the user opens the menu panel themselves since the tour is pointing to it, the tour won't start automatically.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Mostly an additive optional feature so low risk of breaking existing.
String or IDL/UUID changes made by this patch: None

Comment 14

[Matthew N. [:MattN]]

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

Comment 15

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5511e7bb7d4b

Comment 16

[Matthew N. [:MattN]]

(In reply to Blair McBride [:Unfocused] from comment #9)
> And I guess file a bug for eventually adding these to the other targets, if
> someone ever gets bored enough to want to fix em up?

On thinking about this more, I think we have enough bugs on file that are higher priority that we can wait to add events to other targets as needed.