1238566 - Synced Tabs signed out state is not visually centered

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: screenshot.png

Reproducible on: Firefox 45.0a2 and Firefox 46.0a1 under Ubuntu 14.04 x64

STR:
1.Launch Firefox with a clean profile.
2.Click menu [≡] → select "Customize".
3.Move the "Synced tabs" icon to toolbar or tab bar.
4.Exit “Customize” mode.
5.Click on “Synced tabs” button.

Expected results:
The 'Synced Tabs' panel is correctly displayed.

Actual results:
1. The bottom part of the 'Synced Tabs' panel is too long (see the attached screenshot).
2. If the 'Synced Tabs' panel is opened from the Panel Menu, the content is not centered(see the 'not centered' screenshot).

Comment 1

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: not centered.png

Comment 2

[Chris Karlof [:ckarlof]]

Ryan is this design intentional or not?

Comment 3

[Chris Karlof [:ckarlof]]

Attachment: mac os signed out state in menu

Comment 4

[Chris Karlof [:ckarlof]]

Probably not. :) It looks worse in Mac OS X.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1238566-fix-centering-in-synced-tabs-UI-again-.-.patch

The problem is that while our vboxes flex, they often don't take the full width of the panelview. As a result these boxes are *close* to the width of the panel view, but a little bit narrower and left-aligned.

We can't change the alignment/packing of the parent box as that box typically holds things that should *not* be centered.

This patch adds 3 new hboxes that both flex and have pack=center. The child vbox is then centered. I tested on Windows and Linux - OSX should also be fixed.

Have I mentioned how much I hate XUL recently? :)

I'll attach a "diff -w" next to make the changes more obvious.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: Same patch but ignoring all whitespace

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8707286 [details] [diff] [review]
0001-Bug-1238566-fix-centering-in-synced-tabs-UI-again-.-.patch

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

::: browser/components/customizableui/content/panelUI.inc.xul
@@ +143,5 @@
>              <vbox id="PanelUI-remotetabs-fetching">
>                <label>&appMenuRemoteTabs.fetching.label;</label>
>              </vbox>
>              <!-- Sync has only 1 (ie, this) device connected -->
> +            <hbox iid="PanelUI-remotetabs-nodevicespane" pack="center" flex="1">

doh - "iid" should obviously be "id" - I've fixed that here.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8707286 [details] [diff] [review]
0001-Bug-1238566-fix-centering-in-synced-tabs-UI-again-.-.patch

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

This fixes the minor issue on Windows, but doesn't help on OSX.

It seems to me that the main issue is that the image here is fixed to 180px, but the width of the panel varies between OSes based on font-sizes (it's sized in em).

While the image does fit on OSX, it still gets too much left margin and too little right margin with this patch. It seems to me we should either fix the margins or change the width we use for the image (which depending on how the SVG is authored might require changes to the SVG).

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1238566-fix-centering-in-synced-tabs-UI-again-.-.patch

(In reply to :Gijs Kruitbosch from comment #8)
> While the image does fit on OSX, it still gets too much left margin and too
> little right margin with this patch. It seems to me we should either fix the
> margins or change the width we use for the image (which depending on how the
> SVG is authored might require changes to the SVG).

Thanks! This patch is the same as before, but also removes left and right padding on the PanelUI-remotetabs-instruction-box class - that padding was never necessary and now things look great on Windows, Linux and OSX (I actually tested them all ;)

Comment 10

[:Gijs (he/him)]

Comment on attachment 8710192 [details] [diff] [review]
0001-Bug-1238566-fix-centering-in-synced-tabs-UI-again-.-.patch

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

I'm sorry, I haven't been able to get to this today. Tomorrow, it'll be first on my list.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8710192 [details] [diff] [review]
0001-Bug-1238566-fix-centering-in-synced-tabs-UI-again-.-.patch

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

Tested on win8 and OSX, lgtm!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/be005c69fbb6

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/be005c69fbb6

Comment 14

[Vasilica Tamas, QA [:vtamas]]

Tested a bit around this bug and noticed the following:

- Ubuntu 14.04 32-bit: the button part of the 'Synced Tabs' panel is still too long: http://i.imgur.com/21arjFi.png

- Mac OS X 10.9.5: several Ui issues related to “Synced Tabs” panel: 
http://i.imgur.com/AIp7SMz.png
http://i.imgur.com/indeQjR.png 


Should I file new bugs for each platform separately?

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8710192 [details] [diff] [review]
0001-Bug-1238566-fix-centering-in-synced-tabs-UI-again-.-.patch

Approval Request Comment
[Feature/regressing bug #]: Synced Tabs Panel UI
[User impact if declined]: Visual glitch when offering to sign in to Sync
[Describe test coverage new/current, TreeHerder]: Existing tests pass
[Risks and why]: Low risk, layout and CSS only change. No code changes.
[String/UUID change made/needed]: None

Comment 16

[Mark Hammond [:markh] [:mhammond]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #14)
> Tested a bit around this bug and noticed the following:
> 
> - Ubuntu 14.04 32-bit: the button part of the 'Synced Tabs' panel is still
> too long: http://i.imgur.com/21arjFi.png

This is bug 1224412.

> - Mac OS X 10.9.5: several Ui issues related to “Synced Tabs” panel: 
> http://i.imgur.com/AIp7SMz.png
> http://i.imgur.com/indeQjR.png 

I guess bugs for these can't hurt.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8710192 [details] [diff] [review]
0001-Bug-1238566-fix-centering-in-synced-tabs-UI-again-.-.patch

Polish the interface, taking it.
Should be in 45 beta 2.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3d8a1493dc15

Comment 19

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 47.0a1 (2016-01-11) under Ubuntu 12.04 64-bit.  
Verified fixed on Firefox 47.0a1 (216-02-18), Firefox 46.0a2 (2016-02-18) and Firefox 45 beta 7 (20160218171844) under Ubuntu 12.04 64-bit, Mac OS X 10.9.5 and Windows 10 64-bit.

During my testing I encountered 2 already known issues: Bug 1224412 and Bug 1245844.

I am marking this bug as Verified since the other issues are tracked separately.