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)

Product:
Firefox
Component:
Pocket
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.1 - Aug 15
Tracking Flags:
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)

Bug 1387077 - Reimplement Pocket animation in the Page Action area.
59 bytes, text/x-review-board-request
adw
: review+
Details 
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.
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-animation] → [reserve-photon-animation]
Blocks: 1367927
Whiteboard: [reserve-photon-animation] → [photon-animation]
Priority: P3 → P2
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
I think you need to rebase, your patch context looks quite outdated.
Yeah, I had that on my todo list but forgot. Thanks for mentioning it.
Blocks: 1373299
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.
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 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 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 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+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d4cc2f1e5c48
Reimplement Pocket animation in the Page Action area. r=adw
Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9bc6af2793a
Reimplement Pocket animation in the Page Action area. r=adw
https://hg.mozilla.org/mozilla-central/rev/c9bc6af2793a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1390264
Flags: qe-verify? → qe-verify+
QA Contact: stefan.georgiev
Depends on: 1391418
(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)
(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)
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1395743
Depends on: 1397583
Depends on: 1399517
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: