Synced Tabs signed out state is not visually centered

VERIFIED FIXED in Firefox 45

Status

()

Firefox
Sync
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: mboldan, Assigned: markh)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 46
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(firefox45 verified, firefox46 verified)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8706378 [details]
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).
(Reporter)

Comment 1

2 years ago
Created attachment 8706382 [details]
not centered.png
Ryan is this design intentional or not?
Flags: needinfo?(rfeeley)
Created attachment 8706471 [details]
mac os signed out state in menu
Probably not. :) It looks worse in Mac OS X.
Priority: -- → P3
Summary: [Ubuntu]'Sync Tabs' panel UI issues → Synced Tabs signed out state is not visually centered
Flags: firefox-backlog+
Blocks: 1239084
(Assignee)

Comment 5

2 years ago
Created attachment 8707286 [details] [diff] [review]
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.
Flags: needinfo?(rfeeley)
Attachment #8707286 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 6

2 years ago
Created attachment 8707287 [details] [diff] [review]
Same patch but ignoring all whitespace
Assignee: nobody → markh
Status: NEW → ASSIGNED
(Assignee)

Comment 7

2 years ago
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

2 years ago
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).
Attachment #8707286 - Flags: review?(gijskruitbosch+bugs) → feedback+
Priority: P3 → P1
(Assignee)

Comment 9

2 years ago
Created attachment 8710192 [details] [diff] [review]
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 ;)
Attachment #8707286 - Attachment is obsolete: true
Attachment #8707287 - Attachment is obsolete: true
Attachment #8710192 - Flags: review?(gijskruitbosch+bugs)

Comment 10

2 years ago
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

2 years ago
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!
Attachment #8710192 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 12

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

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be005c69fbb6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Blocks: 1241860
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?
Flags: needinfo?(markh)
(Assignee)

Comment 15

2 years ago
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
Attachment #8710192 - Flags: approval-mozilla-beta?
(Assignee)

Comment 16

2 years ago
(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.
Flags: needinfo?(markh)
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.
Attachment #8710192 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 18

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/3d8a1493dc15
status-firefox45: affected → fixed
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
status-firefox45: fixed → verified
status-firefox46: fixed → verified

Updated

2 years ago
Depends on: 1251842
(Assignee)

Updated

2 years ago
No longer depends on: 1251842
You need to log in before you can comment on or make changes to this bug.