1243806 - Pocket icon doesn't show up on Linux hidpi

Status: RESOLVED FIXED

(Firefox :: Pocket, defect)

Description

[Justin Wood (:Callek)]

Attachment: pocket_icon_missing.png

Pocket toolbar icon is missing on 46.0a2 for Linux. When customized, and move the Icon off the toolbar it does show up in the available-icons box.

The *button* is still there, and clicking it does still work as expected (for me, as a non-pocket user) just an empty space.

Attached find a screenshot of my environment.

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

Probably a recent regression.  Tracking for 46. 
Jesse, what flavor of Linux?
Andrei can you all check on 47 and 45 to see if they are definitely affected?

Comment 2

[Cornel Ionce [:noni] [Hubs QA]]

We cannot reproduce this issue on Ubuntu 12.04 x86 and 14.04 x86.
The button is correctly displayed in both menu/bar.
Tried with DevEdition/Default themes and did not see this on 45 beta 2, latest Nightly, latest Aurora and 2016-01-28 Aurora.

However, we ran into another issue where the Pocket button is completely removed when customizing and restoring default state. Adding Alexandra in the loop since she will make further investigation on this matter.

Comment 3

[Ada [:adalucinet]]

(In reply to Cornel Ionce [QA] from comment #2)
> However, we ran into another issue where the Pocket button is completely
> removed when customizing and restoring default state. Adding Alexandra in
> the loop since she will make further investigation on this matter.
This issue is tracked in bug 1245074 - apparently, a regression from 46.0a1 (2015-12-29).

Comment 4

[Justin Wood (:Callek)]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] [PTO 15 - 22 February] from comment #3)
> (In reply to Cornel Ionce [QA] from comment #2)
> > However, we ran into another issue where the Pocket button is completely
> > removed when customizing and restoring default state. Adding Alexandra in
> > the loop since she will make further investigation on this matter.
> This issue is tracked in bug 1245074 - apparently, a regression from 46.0a1
> (2015-12-29).

Ummm, afaict its not tracked in that bug. That is about a customize panel locking up. This is about the pocket icon being missing despite attempting to customize.

This is still an issue in todays aurora, so would affect beta users in just a few weeks.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Reproduces for me on Ubuntu 15.10 with mozregression. Caused by bug 1215694. Not sure if also related, but the button position in the toolbar also changed (from being next to the bookmarks menu dropdown to all the way on the end). Was that intentional?

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Justin Wood (:Callek) from comment #4)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] [PTO 15 - 22
> February] from comment #3)
> > (In reply to Cornel Ionce [QA] from comment #2)
> > > However, we ran into another issue where the Pocket button is completely
> > > removed when customizing and restoring default state. Adding Alexandra in
> > > the loop since she will make further investigation on this matter.
> > This issue is tracked in bug 1245074 - apparently, a regression from 46.0a1
> > (2015-12-29).
> 
> Ummm, afaict its not tracked in that bug. That is about a customize panel
> locking up. This is about the pocket icon being missing despite attempting
> to customize.
> 
> This is still an issue in todays aurora, so would affect beta users in just
> a few weeks.

That is actually bug 1247045.

Comment 7

[Justin Wood (:Callek)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> (In reply to Justin Wood (:Callek) from comment #4)
> > (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] [PTO 15 - 22
> > February] from comment #3)
> > > (In reply to Cornel Ionce [QA] from comment #2)
> > > > However, we ran into another issue where the Pocket button is completely
> > > > removed when customizing and restoring default state. Adding Alexandra in
> > > > the loop since she will make further investigation on this matter.
> > > This issue is tracked in bug 1245074 - apparently, a regression from 46.0a1
> > > (2015-12-29).
> > 
> > Ummm, afaict its not tracked in that bug. That is about a customize panel
> > locking up. This is about the pocket icon being missing despite attempting
> > to customize.
> > 
> > This is still an issue in todays aurora, so would affect beta users in just
> > a few weeks.
> 
> That is actually bug 1247045.

No its not -- its still in my toolbar, I can still interact with it -- there is just no icon "showing" just the blank-space button.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Justin Wood (:Callek) from comment #7)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> > (In reply to Justin Wood (:Callek) from comment #4)
> > > (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] [PTO 15 - 22
> > > February] from comment #3)
> > > > (In reply to Cornel Ionce [QA] from comment #2)
> > > > > However, we ran into another issue where the Pocket button is completely
> > > > > removed when customizing and restoring default state. Adding Alexandra in
> > > > > the loop since she will make further investigation on this matter.
> > > > This issue is tracked in bug 1245074 - apparently, a regression from 46.0a1
> > > > (2015-12-29).
> > > 
> > > Ummm, afaict its not tracked in that bug. That is about a customize panel
> > > locking up. This is about the pocket icon being missing despite attempting
> > > to customize.
> > > 
> > > This is still an issue in todays aurora, so would affect beta users in just
> > > a few weeks.
> > 
> > That is actually bug 1247045.
> 
> No its not -- its still in my toolbar, I can still interact with it -- there
> is just no icon "showing" just the blank-space button.

ok, that is a different issue, and this bug is conflating a couple things.  

My response referred to comment 3 which is related to a couple issues: bug 1245447 (waiting for uplift) which resulted in missing buttons after a customization restore.  There is bug 1247045 which is about the pocket icon moving into palette after a customization reset (deemed non-blocking).  Bug 1245074 referred to in comment 3 is caused by the same base issue as 1247045.

The icon missing from the button on ubuntu per comment 0 is new to me and I cannot reproduce with either a download of aurora or a nightly build.  Need to figure out what is different between ubuntu 14 and 15.

Comment 9

[Shane Caraveo (:mixedpuppy)]

this is likely a hidpi issue

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: linux hidpi fix

Comment 11

[Justin Wood (:Callek)]

Comment on attachment 8719974 [details] [diff] [review]
linux hidpi fix

I'm willing to test a try build of this patch, if it helps confidence levels (and if author and/or reviewer can't reproduce themselves)

Comment 12

[Shane Caraveo (:mixedpuppy)]

:Callek, RyanVM is doing that, but a second look would be great.  I don't have real hidpi to test with.

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2144bc9b303

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8719974 [details] [diff] [review]
linux hidpi fix

Looks good here!

Comment 15

[:Gijs (he/him)]

Comment on attachment 8719974 [details] [diff] [review]
linux hidpi fix

Review of attachment 8719974 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/pocket/skin/shared/pocket.css
@@ -44,5 @@
> -  }
> -
> -  #pocket-button[cui-areatype="menu-panel"],
> -  toolbarpaletteitem[place="palette"] > #pocket-button {
> -    list-style-image: url(chrome://pocket/skin/[e-mail]);

The menupanel code actually works everywhere, as there's a Linux copy of the hidpi menu panel file.

Please just move the toolbar .png list-style-image property assignments into the windows and osx files, don't create a new file, and leave the menupanel stuff here.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: linux hidpi fix

Comment 17

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecb8e109b9d3

Comment 18

[Justin Dolske [:Dolske]]

Who not fix this by shipping a @2x version of the Pocket icons for Linux?

Comment 19

[Shane Caraveo (:mixedpuppy)]

(In reply to Justin Dolske [:Dolske] from comment #18)
> Who not fix this by shipping a @2x version of the Pocket icons for Linux?

we dont ship any @2x toolbar icons on linux.

Comment 20

[Justin Wood (:Callek)]

Comment on attachment 8720347 [details] [diff] [review]
linux hidpi fix

This shows the pocket icon in the toolbar for me just fine. I suggest an aurora uplift.

Comment 21

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #18)
> Who not fix this by shipping a @2x version of the Pocket icons for Linux?

I did consider suggesting this instead, but IIRC I've gotten pushback from other reviewers on doing something like that in the past for Linux, and I don't know how well our CSS and the relevant width: specifications for buttons etc. would cope if we started doing this on the toolbar.

(We have CSS-d the crap out of the menu panel so it is expected to work there. The toolbar is harder in part because of add-on expectations.)

Given that this will need uplift to 46, keeping it simple seemed like the best idea.

Comment 22

[Justin Wood (:Callek)]

Attachment: pocket_patch_nightly.png

Shane asked me to check how it looks, so here is a screenshot of that try build on my system.

Comment 23

[Justin Wood (:Callek)]

Attachment: pocket_patch_nightly_customize.png

And here is the customize panel with Pocket in it.

Comment 24

[Justin Dolske [:Dolske]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> (In reply to Justin Dolske [:Dolske] from comment #18)
> > Who not fix this by shipping a @2x version of the Pocket icons for Linux?
> 
> we dont ship any @2x toolbar icons on linux.

Well. We don't have a 2x browser/ Toolbar.png on Linux. But the Hello system addon does (http://mxr.mozilla.org/mozilla-central/source/browser/extensions/loop/chrome/skin/linux/[e-mail]), and of course Linux picks up other hidpi assests as a result of themes/shared or from inheriting from Windows in /toolkit.

This patch isn't _wrong_, it's just that we're moving in the direction of having hidpi assets, and seems like it should be trivial for UX to create one here.

Comment 25

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/30dbdd0de3332d5fac0619d8a8ff11da5836e949
Bug 1243806 fix linux hidpi, r=gijs

Comment 26

[Shane Caraveo (:mixedpuppy)]

Bug 1249346 is a follow up to deal with adding new @2x icons.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/30dbdd0de333

Comment 28

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8720347 [details] [diff] [review]
linux hidpi fix

Approval Request Comment
[Feature/regressing bug #]:pocket
[User impact if declined]:button with no icon appears in toolbar for hidpi linux users
[Describe test coverage new/current, TreeHerder]:manual verification
[Risks and why]: low, css only change
[String/UUID change made/needed]:none

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8720347 [details] [diff] [review]
linux hidpi fix

Fix for minor pocket regression, please uplift to aurora

Comment 30

[Matthew N. [:MattN]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a695dee860e

Comment 31

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected