Allow opening the Pocket panel

VERIFIED FIXED in Firefox 38.0.5

Status

()

defect
P2
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: ckprice, Assigned: MattN)

Tracking

(Depends on 3 bugs)

36 Branch
Firefox 41
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox38.0.5 verified, firefox39 fixed, firefox40 fixed, firefox41 fixed)

Details

(Whiteboard: [engagement-fxGrowth-2015])

Attachments

(1 attachment, 1 obsolete attachment)

This bug is created to create an API to allow the web to expand the Pocket panel in the chrome.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 40.3 - 11 May
Points: --- → 5
Flags: qe-verify+
Flags: firefox-backlog+
/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/
Attachment #8601817 - Flags: review?(gijskruitbosch+bugs)
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 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/
Attachment #8601817 - Flags: review?(jaws)
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.
Attachment #8601817 - Flags: review?(gijskruitbosch+bugs)
QA Contact: andrei.vaida
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.
Flags: needinfo?(MattN+bmo)
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";
No longer blocks: 1162759
Priority: -- → P2
Blocks: 1162759
Duplicate of this bug: 1163033
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.
(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
Flags: needinfo?(MattN+bmo)
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3048969&repo=fx-team
Flags: needinfo?(MattN+bmo)
Iteration: 40.3 - 11 May → 41.1 - May 25
Flags: needinfo?(MattN+bmo)
https://hg.mozilla.org/mozilla-central/rev/3f934d7672b3
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
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
Attachment #8601817 - Flags: approval-mozilla-release?
Attachment #8601817 - Flags: approval-mozilla-beta?
Attachment #8601817 - Flags: approval-mozilla-aurora?
Comment on attachment 8601817 [details]
MozReview Request: bz://1161810/MattN

a+ for all channels, required for Pocket launch in 38.0.5.
Attachment #8601817 - Flags: approval-mozilla-release?
Attachment #8601817 - Flags: approval-mozilla-release+
Attachment #8601817 - Flags: approval-mozilla-beta?
Attachment #8601817 - Flags: approval-mozilla-beta+
Attachment #8601817 - Flags: approval-mozilla-aurora?
Attachment #8601817 - Flags: approval-mozilla-aurora+
Depends on: 1169277
Depends on: 1169283
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Attachment #8601817 - Attachment is obsolete: true
Attachment #8620234 - Flags: review+
You need to log in before you can comment on or make changes to this bug.