1161810 - Allow opening the Pocket panel

Status: VERIFIED FIXED

(Firefox :: Tours, defect, P2)

Description

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

This bug is created to create an API to allow the web to expand the Pocket panel in the chrome.

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: bz://1161810/MattN

/r/8193 - Bug 1161810 - UITour: Allow opening the Pocket panel via showMenu("pocket").

Pull down this commit:

hg pull -r 78426642c55b2a135511b95ed8c55ebfb719f06a https://reviewboard-hg.mozilla.org/gecko/

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8601817 [details]
MozReview Request: bz://1161810/MattN

/r/8193 - Bug 1161810 - UITour: Allow opening the Pocket panel via showMenu("pocket"). r=Gijs

Pull down this commit:

hg pull -r 4bd1339792c23ca60e536d31f1983745b11d8475 https://reviewboard-hg.mozilla.org/gecko/

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8601817 [details]
MozReview Request: bz://1161810/MattN

/r/8193 - Bug 1161810 - UITour: Allow opening the Pocket panel via showMenu("pocket"). r=Gijs

Pull down this commit:

hg pull -r 4bd1339792c23ca60e536d31f1983745b11d8475 https://reviewboard-hg.mozilla.org/gecko/

Comment 4

[:Gijs (out for now; he/him)]

https://reviewboard.mozilla.org/r/8193/#review7037

::: browser/components/uitour/UITour.jsm:1596
(Diff revision 2)
> +      })).catch(log.error);

The rejection here, in the case of an already-open panel, doesn't seem like it should log an error.

::: browser/components/uitour/UITour.jsm:1594
(Diff revision 2)
> +                                    widgetWrapper.anchor,

As best I can tell this won't work if the button is in a collapsed toolbar.

Also, have you tested if this works when the button is in the overflow of an overflowable toolbar? (I think it should, but it's been a while since I used this code)


All in all I would, I guess, feel happier if we had a utility function in CustomizableUI that took care of this. It seems to me that other consumers than UITour may be interested, and having the implementation in CustomizableUI will simplify some of the implementation, I think.

Comment 5

[Cory Price [:ckprice] (bugmail disabled, NI me!)]

MattN - initial request is to be able to highlight (bug 1160663) AND drop the panel in one go. Will this be possible with this implementation? Case of interest is if the icon exist in the menu panel.

Comment 6

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

Comment on attachment 8601817 [details]
MozReview Request: bz://1161810/MattN

https://reviewboard.mozilla.org/r/8191/#review7067

Ship It!

::: browser/components/uitour/test/browser_UITour_pocket.js:25
(Diff revision 2)
> +      return widgetPanel.state == "open";

It's possible that widgetPanel will be null here since the panel is created dynamically, but that's not an error case since it may exist on the next tick.

Can you check for null before dereferencing it?

> return widgetPanel && widgetPanel.state == "open";

Comment 8

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/8193/#review7125

> The rejection here, in the case of an already-open panel, doesn't seem like it should log an error.

I don't follow how we would log an error if the panel is already open since I call `resolve()` in that case? Do you mean the "A subview is already showing" case? If so, then I think it should be an error if that subview is not the expected one. I didn't know a good way to know which subview is open to have different behaviour in that case.

> As best I can tell this won't work if the button is in a collapsed toolbar.
> 
> Also, have you tested if this works when the button is in the overflow of an overflowable toolbar? (I think it should, but it's been a while since I used this code)
> 
> 
> All in all I would, I guess, feel happier if we had a utility function in CustomizableUI that took care of this. It seems to me that other consumers than UITour may be interested, and having the implementation in CustomizableUI will simplify some of the implementation, I think.

> As best I can tell this won't work if the button is in a collapsed toolbar.

The page is checking where the button is ahead of time and bug 988151 already covers this issue.

I did test that this works fine in overflow because I'm using widgetWrapper.anchor which is the overflow button in that case.

I filed bug 1163240 to have this code moved to CUI or PanelUI (I don't even really know where the line is between them or whether PanelUI is supposed to only be for the appMenu Panel or all widget-related panels) as I don't think this code handles enough edge cases for non-tour consumers to start using it and to keep the risk isolated to UITour since this will be uplifted to 38.0.5.

Comment 9

[Matthew N. [:MattN]]

(In reply to Cory Price [:ckprice] from comment #5)
> MattN - initial request is to be able to highlight (bug 1160663) AND drop
> the panel in one go. Will this be possible with this implementation? Case of
> interest is if the icon exist in the menu panel.

I wouldn't recommend highlighting while requesting the panel be opened or you will get: http://i.imgur.com/mRCe7Wg.png?1

Comment 10

[Pulsebot]

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

Comment 11

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3048969&repo=fx-team

Comment 12

[Pulsebot]

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

Comment 13

[Pulsebot]

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

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/3f934d7672b3

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8601817 [details]
MozReview Request: bz://1161810/MattN

Approval Request Comment
[Feature/regressing bug #]: Tour functionality for pocket (snippets or product page)
[User impact if declined]: Our web content can't programmatically open the pocket menu
[Describe test coverage new/current, TreeHerder]: Automated b-c tests
[Risks and why]: Low risk since we're not making the panel sticky (which can cause problems sometimes) and if we encounter issues, our web content can just stop using the API.
[String/UUID change made/needed]: None

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 8601817 [details]
MozReview Request: bz://1161810/MattN

a+ for all channels, required for Pocket launch in 38.0.5.

Comment 17

[Justin Dolske [:Dolske]]

(aurora currently closed)
https://hg.mozilla.org/releases/mozilla-beta/rev/885e50dd6ae3
https://hg.mozilla.org/releases/mozilla-release/rev/06499c7a81a9

Comment 18

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

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

Comment 19

[Andrei Vaida [:avaida]]

Confirmed fixed on RC 38.0.5 (build4: 20150525141253), using Windows 7 (x64), Ubuntu 14.04 (x64), Mac OS X 10.9.5.

Testing was performed on the demo tour page and covered the following locales on each platform: en-US, de, ja, es-ES, ru. A few issues were filed separately as a result: Bug 1169275, Bug 1169277 and Bug 1169283.

Comment 21

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1161810 - UITour: Allow opening the Pocket panel via showMenu("pocket"). r=Gijs