Closed
Bug 1387077
Opened 7 years ago
Closed 7 years ago
Save to Pocket animation broken after Pocket button moved to URL bar
Categories
(Firefox :: Pocket, defect, P1)
Firefox
Pocket
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | fixed |
People
(Reporter: jaws, Assigned: jaws)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: [photon-animation])
Attachments
(1 file)
On a local build from fa1da3c0b200, clicking on the Pocket button doesn't show the Pocket icon increasing in size nor the Pocket icon dropping in to the Library button.
Assignee | ||
Updated•7 years ago
|
Keywords: regression
Updated•7 years ago
|
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-animation] → [reserve-photon-animation]
Assignee | ||
Updated•7 years ago
|
Blocks: 1367927
Whiteboard: [reserve-photon-animation] → [photon-animation]
Updated•7 years ago
|
Priority: P3 → P2
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Updated•7 years ago
|
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
I think you need to rebase, your patch context looks quite outdated.
Assignee | ||
Comment 4•7 years ago
|
||
Yeah, I had that on my todo list but forgot. Thanks for mentioning it.
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8895440 [details]
Bug 1387077 - Reimplement Pocket animation in the Page Action area.
https://reviewboard.mozilla.org/r/166628/#review171768
::: browser/extensions/pocket/skin/shared/pocket.css:55
(Diff revision 3)
> position: absolute;
> overflow: hidden;
> - top: calc(50% - 9px); /* 9px is half the height of the sprite */
> + top: calc(50% - 8px); /* 8px is half the height of the sprite */
> /* Since .toolbarbutton-icon uses a different width than the animatable box,
> we need to set a padding relative to the difference in widths. */
> margin-inline-start: calc((16px + 2 * var(--toolbarbutton-inner-padding) - 20px) / 2);
var(--toolbarbutton-inner-padding) doesn't make sense when the button is not a .toolbarbutton-1 anymore.
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895440 [details]
Bug 1387077 - Reimplement Pocket animation in the Page Action area.
https://reviewboard.mozilla.org/r/166628/#review171768
> var(--toolbarbutton-inner-padding) doesn't make sense when the button is not a .toolbarbutton-1 anymore.
That rule is overriden just a few lines later. Much of this can be cleaned up when bug 1385418 is fixed.
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8895440 [details]
Bug 1387077 - Reimplement Pocket animation in the Page Action area.
https://reviewboard.mozilla.org/r/166628/#review171932
Thanks Jared. Sorry again for breaking this. I feel like I should be doing a lot of the work here. I'm clearing the review for now with the comments below.
::: browser/base/content/browser-pageActions.js:73
(Diff revision 3)
> if (action.__isSeparator) {
> this._appendPanelSeparator(action);
> return;
> }
> + if (action._onBeforePlacedInWindow) {
> + action._onBeforePlacedInWindow(window);
Please add an onBeforePlacedInWindow to Action.prototype and call that on the action here, just like how onPlacedInPanel etc. work.
And I'm not sure the "before" is adding anything here... onPlacedInWindow sounds good by itself, don't you think?
::: browser/base/content/browser-pageActions.js:182
(Diff revision 3)
> _toggleTempPanelForAction(action) {
> let panelNodeID = this._tempPanelID;
> let panelNode = document.getElementById(panelNodeID);
> if (panelNode) {
> panelNode.hidePopup();
> - return;
> + return null;
See my comment below about opening the panel.
::: browser/base/content/browser-pageActions.js:221
(Diff revision 3)
> action.subview.onShowing(panelViewNode);
> }
>
> this.panelNode.hidePopup();
>
> - let urlbarNodeID = this._urlbarButtonNodeIDForActionID(action.id);
> + let urlbarNodeID = action.anchorIDOverride ||
I don't think you need this. The anchor will open on the urlbarIDOverride, the pocket-button-box, and that should be fine in this case, same as how it works for the star-button-box. See my comment below on the DOM setup, which I don't think is quite right and probably why you found this to be necessary.
::: browser/base/content/browser-pageActions.js:231
(Diff revision 3)
>
> if (iframeNode) {
> action.onIframeShown(iframeNode, panelNode);
> }
> +
> + return panelNode;
See my comment below about opening the panel.
::: browser/extensions/pocket/bootstrap.js:191
(Diff revision 3)
> + urlbarIDOverride: "pocket-button-box",
> + anchorIDOverride: "pageAction-urlbar-pocket",
> _insertBeforeActionID: PageActions.ACTION_ID_BOOKMARK_SEPARATOR,
> + _urlbarNodeInMarkup: true,
> + onBeforePlacedInWindow(window) {
> + let doc = window.document;
It would probably be a good idea to first check whether pocket-button-box exists (and bail if it does). Actions can be removed and then replaced in windows. I don't think that should happen for built-in actions since they can't really be removed, but that can definitely happen for extensions like pocket.
And actually now that I say that, you should probably remove pocket-button-box in PocketPageAction.shutdown. Once you do that, it shouldn't be possible for pocket-button-box to exist when onBeforePlacedInWindow is called, but it's still a good idea to check anyway.
::: browser/extensions/pocket/bootstrap.js:200
(Diff revision 3)
> + let animatableBox = doc.createElement("hbox");
> + animatableBox.id = "pocket-animatable-box";
> + let animatableImage = doc.createElement("image");
> + animatableImage.id = "pocket-animatable-image";
> + let pocketButton = doc.createElement("image");
> + pocketButton.id = "pageAction-urlbar-pocket";
I think this is why you found the anchorIDOverride to be necessary. Using this specific ID confuses the page actions stuff because this is the ID that would normally be automatically generated for this action's urlbar node, except for the fact that a urlbarIDOverride is also given. So it needs to be something else (if anything at all). I would use pocket-button so that this is similar to star-button-box and star-button.
::: browser/extensions/pocket/bootstrap.js:209
(Diff revision 3)
> + wrapper.appendChild(animatableBox);
> + animatableBox.appendChild(animatableImage);
> + let iconBox = doc.getElementById("urlbar-icons");
> + iconBox.appendChild(wrapper);
> + wrapper.hidden = true;
> + wrapper.addEventListener("click", PocketPageAction.onUrlbarNodeClicked);
This should probably be an arrow function just so that `this` isn't something unexpected in PocketPageAction.onUrlbarNodeClicked, even if it doesn't currently use `this`.
::: browser/extensions/pocket/bootstrap.js:239
(Diff revision 3)
> + BrowserUtils.setToolbarButtonHeightProperty(event.target);
> +
> + let win = event.target.ownerGlobal;
> + let browserPageActions = win.BrowserPageActions;
> + let action = PocketPageAction.pageAction;
> + let panel = browserPageActions._toggleTempPanelForAction(action);
I'm going to need to open the Pocket panel as part of bug 1385418 too. There should be a nicer way than this to do it -- a way to trigger a urlbar button's command. What I'd like is to add a method on BrowserPageActions, like doCommandForAction(action). It would do what the click listener is doing basically: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-pageActions.js#312 Except it wouldn't take an event so that anyone can call it, so your caller would still need to check event.button first.
Then, instead of doing everything you do below here in onUrlbarNodeClicked, you would do it in PocketPageAction.onIframeShown, which is called when the iframe's panel is opened. And that way no matter how the Pocket panel is opened, your animation will work.
Also, if you'd like, instead of adding your own popuphiding/hidden listeners, there could be an onIframeHiding/Hidden methods on the action, to go along with onIframeShown. But I don't think that's too important, so up to you.
Attachment #8895440 -
Flags: review?(adw)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8895440 [details]
Bug 1387077 - Reimplement Pocket animation in the Page Action area.
https://reviewboard.mozilla.org/r/166628/#review171932
> Please add an onBeforePlacedInWindow to Action.prototype and call that on the action here, just like how onPlacedInPanel etc. work.
>
> And I'm not sure the "before" is adding anything here... onPlacedInWindow sounds good by itself, don't you think?
onBeforePlacedInWindow makes more sense to me because at this point no DOM nodes have been created, and this function exists to allow creation of the nodes before other code would attempt to access them.
Changing the name to onPlacedInWindow would not have the same implication that this new function would run before onPlacedInUrlbar etc.
> I don't think you need this. The anchor will open on the urlbarIDOverride, the pocket-button-box, and that should be fine in this case, same as how it works for the star-button-box. See my comment below on the DOM setup, which I don't think is quite right and probably why you found this to be necessary.
Having the "pocket-button-box" as the anchor puts the panel slightly offset due to the margin-inline-start on the .urlbar-icon
The bookmark popup actually uses the "star-button" as its anchor, though that code is located in browser-places.js.
> I think this is why you found the anchorIDOverride to be necessary. Using this specific ID confuses the page actions stuff because this is the ID that would normally be automatically generated for this action's urlbar node, except for the fact that a urlbarIDOverride is also given. So it needs to be something else (if anything at all). I would use pocket-button so that this is similar to star-button-box and star-button.
I used that ID so I wouldn't have to change some of the CSS, but I agree that it is at least confusing while reading this code. I have changed this to "pocket-button" now.
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8895440 [details]
Bug 1387077 - Reimplement Pocket animation in the Page Action area.
https://reviewboard.mozilla.org/r/166628/#review172484
Thanks Jared.
::: browser/modules/PageActions.jsm:439
(Diff revision 4)
> * An options object suitable for passing to the Subview
> * constructor, if you'd like the action to have a subview. See
> * the subview constructor for info on this object's properties.
> * @param tooltip (string, optional)
> * The action's button tooltip text.
> + * @param anchorIDOverride (string, optional)
Nit: Could you please move this between title and iconURL? (I'm trying to keep these like: <required params ordered alphabetically>, <optional params ordered alphabetically>)
::: browser/modules/PageActions.jsm:467
(Diff revision 4)
> onPlacedInUrlbar: false,
> onShowingInPanel: false,
> shownInUrlbar: false,
> subview: false,
> tooltip: false,
> + anchorIDOverride: false,
Nite: Same here
Attachment #8895440 -
Flags: review?(adw) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d4cc2f1e5c48
Reimplement Pocket animation in the Page Action area. r=adw
I had to back this out for crashtest assertions like https://treeherder.mozilla.org/logviewer.html#?job_id=122391008&repo=autoland
https://hg.mozilla.org/integration/autoland/rev/65dd54117293fc9e0603d12b23c0029524b50f50
Flags: needinfo?(jaws)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jaws)
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9bc6af2793a
Reimplement Pocket animation in the Page Action area. r=adw
Comment 18•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev
Comment 19•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> Comment on attachment 8895440 [details]
> Bug 1387077 - Reimplement Pocket animation in the Page Action area.
>
> https://reviewboard.mozilla.org/r/166628/#review171768
>
> > var(--toolbarbutton-inner-padding) doesn't make sense when the button is not a .toolbarbutton-1 anymore.
>
> That rule is overriden just a few lines later. Much of this can be cleaned
> up when bug 1385418 is fixed.
Is there a bug filed on cleaning this up?
Flags: needinfo?(jaws)
Assignee | ||
Comment 20•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #19)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> > That rule is overriden just a few lines later. Much of this can be cleaned
> > up when bug 1385418 is fixed.
>
> Is there a bug filed on cleaning this up?
Bug 1393218.
Depends on: 1393218
Flags: needinfo?(jaws)
Comment 21•7 years ago
|
||
I have reproduced this issue with Nightly 57.0a1 (2017-08-08) on Windows 10 x64.
This bug fix is Verified with latest Nightly 57.0a1 !
Build ID 20170830100230
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•