Pocket button does not stay red after URL pocketed and animation has finished

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jkerim, Assigned: adw)

Tracking

unspecified
Firefox 58
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed, firefox59 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8893484 [details]
Screenshot 2017-08-03 13.13.50.png

Browser Version: 57.0a1 (2017-08-03) (64-bit)
OS: OSX 10.12.4

Steps to reproduce:

Visit a URL, click the Pocket button in the URL bar

Expected result:

A drop down shows up saying the URL was successfully pocketed, and the button turns from grey to red.

Actual result:

The drop down appears, but the button never changes colour, it remains grey.

Comment 1

a year ago
Does this still reproduce? It seems to WFM on OS X, at least (on 57 and nightly).
Flags: needinfo?(jkerim)

Comment 2

a year ago
Chatted with :jkerim on IRC. Apparently in other browsers, the item stays red until you navigate or switch tabs. So it doesn't have state (so if you have a URL open twice, pocket one of them, check the other one, it will still be grey) but it continues to show you pocketed the page already, while you have it open. It seems like from a parity-with-chrome/edge/ie/safari/whatever perspective and so on, it might be helpful if we did that. Drew, do you think that's still feasible for 57? Not sure how easy it would be to deal with this because of the navigation/tabswitch change, given that the node is just always there. :-(
status-firefox57: --- → affected
status-firefox58: --- → affected
Flags: needinfo?(jkerim) → needinfo?(adw)
Priority: -- → P3
Summary: Pocket button does not change colour after URL pocketed → Pocket button does not stay red after URL pocketed and animation has finished

Updated

a year ago
See Also: → bug 1392142
(Assignee)

Comment 3

a year ago
It doesn't seem too hard, but I agree that tab switches and location changes are probably the hard part.  I'll take a look.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Flags: needinfo?(adw)

Updated

a year ago
See Also: bug 1392142
(Assignee)

Comment 4

a year ago
Another wrinkle is that we shouldn't (IMO) keep the icon red if the page wasn't actually pocketed, like if you don't have a Pocket account and only the intro panel was shown.
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
This adds an onLocationChange callback to actions.  I was thinking of adding that at some point anyway.  It's useful for cases like this where the action's state in a window depends on that window's location.  I can potentially use it in the WebExtensions reconciliation bug too since location changes are part of that API.  (But the WebExtensions page action implementation already handles that itself, so I might not use it there ultimately.)

I just reused the "animate" attribute for this, which means that you see the little pulsing animation when you switch back to a tab that you pocketed.  That seems OK to me especially since the animation is subtle, but maybe you disagree?

Note too that we color the button red also on the "open" attribute, and this patch doesn't change that.

Comment 8

a year ago
mozreview-review
Comment on attachment 8914974 [details]
Bug 1387141 - Pocket button does not stay red after URL pocketed and animation has finished.

https://reviewboard.mozilla.org/r/186234/#review191604

I think this is pretty close, but I'd prefer to disentangle this from the animations code a bit more, if that works for you. :-)

::: browser/extensions/pocket/bootstrap.js:203
(Diff revision 1)
> +
> +  // Sets or removes the "animate" attribute on the Pocket urlbar button as
> +  // necessary.
> +  updateUrlbarNodeState(browserWindow) {
> +    if (!this.pageAction ||
> +        !Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {

I don't think this should be gated on the animations pref.

::: browser/extensions/pocket/bootstrap.js:215
(Diff revision 1)
> +    if (pocketedInnerWindowID == browser.innerWindowID) {
> +      // The current window in this browser is pocketed.
> +      urlbarNode.setAttribute("animate", "true");
> +    } else {
> +      // The window isn't pocketed.
> +      urlbarNode.removeAttribute("animate");
> +    }

This retriggers the animation when switching between pocketed and unpocketed tabs. I think we should instead set/unset a separate attribute that only updates the context-fill colour of the "normal" SVG icon. That should be sufficient to keep the red colour but not trigger the animation. Plus we will then have a working pocket indicator for people who turn off cosmetic animations.

I'm also seeing a weird bug where if I follow these steps:

- pocket a page
- before the library button starts animating, switch to another tab
- (the library button now animates on the other tab)
- switch back to the pocketed tab
- this now doesn't have a red pocket icon.
Attachment #8914974 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
No problem, that's fair.

(In reply to :Gijs from comment #8)
> I'm also seeing a weird bug where if I follow these steps:

I can't reproduce following quite those steps (I imagine I'm not doing it quite the same way you are), but I can reproduce following these steps:

1. Start Firefox, open a pocketable page
2. Click Pocket urlbar button
3. After panel opens and page is pocketed, click outside the panel (e.g., blank spot in navbar) to close it and then quickly Command-Tab to another app
4. Command-Tab back

I can't reproduce with the new patch though, so I'm guessing it has to do with the complexity of the animation (multiple nodes, etc.)?

Comment 11

a year ago
mozreview-review
Comment on attachment 8914974 [details]
Bug 1387141 - Pocket button does not stay red after URL pocketed and animation has finished.

https://reviewboard.mozilla.org/r/186234/#review191974

Unfortunately I'm still seeing a race here.

STR:

1. open page A and B next to each other, both such that there's a visible pocket button (so not about: pages or new tab or whatever)
2. on page A, click the pocket button
3. after "saved to pocket" appears in the doorhanger, but before the doorhanger closes, switch to tab B with cmd-(shift-)tab

ER:
the tab on which you pocketed (page A) gets a red pocket button when checking both tabs

AR:
the tab which you didn't pocket (page B) gets a red pocket button when checking both tabs

I think this ends up being "the cure is worse than the disease", so clearing review for now. If this is difficult to solve (which it might be - hard for me to tell off-hand), perhaps we should punt on this for now. We've never supported this before (ie when pocket was a toolbar button). I hoped it would be relatively easy to change that, but if it's not I don't think we should waste too much time on this.
Attachment #8914974 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
Thanks Gijs.  The problem is that in that case, onIframeHiding is called after the tab switch.  So `browser` is the browser in the other tab.  This revision fixes that by saving the browser in onIframeShown, the same way `urlbarNode` is currently saved.  It saves the innerWindowID too just in case the browser's inner window somehow changes between the time the panel is shown and hidden.

I think this is relatively easy to fix, I just haven't been able to do it yet. :-/

Comment 14

a year ago
mozreview-review
Comment on attachment 8914974 [details]
Bug 1387141 - Pocket button does not stay red after URL pocketed and animation has finished.

https://reviewboard.mozilla.org/r/186234/#review192724

Looks good to me.
Attachment #8914974 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 15

a year ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5bcbac52b473
Pocket button does not stay red after URL pocketed and animation has finished. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/5bcbac52b473
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Probably too late for 57!
status-firefox57: affected → wontfix
QA Whiteboard: [good first verify]

Comment 18

a year ago
I have Reproduced this bug with Firefox Aurora Version 58.0b12 on Windows 7, (32-bit)

Actual Results:

When I clicked on the pocket button, a dropdown showsup "Saved to Pocket", add tags and the button turns from grey to red

Status: Fixed & Verified 

Useragent: Mozilla/5.0 (Windows NT 6.1; rv:58.0) Gecko/20100101 Firefox/58.0 
BuildID: 20171218174357

[testday-20171222]

Comment 19

a year ago
I have Reproduced this bug with Firefox Nightly 59.0a1 (2017-12-22) on Windows7(32-bit)

Actual Results:

When I clicked on the pocket button, a dropdown showsup "Saved to Pocket", add tags and the button turns from grey to red

Status: Fixed & Verified 

Useragent: Mozilla/5.0 (Windows NT 6.1; rv:59.0) Gecko/20100101 Firefox/59.0
BuildID: 20171222220251

[testday-20171222]

Updated

a year ago
status-firefox59: --- → verified
You need to log in before you can comment on or make changes to this bug.