979987 - Subviews footers text isn't centered anymore

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

Probably as a side effect of bug 972405 the subviews footers (History and Bookmarks) aren't centered anymore.

Comment 1

[:Gijs (he/him)]

Please don't prio the bugs you file.

Comment 2

[u428464]

(In reply to :Gijs Kruitbosch from comment #1)
> Please don't prio the bugs you file.

Ok so just marking australis-cust is now enough ?

Comment 3

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v1

Tested with the bookmarks and history panel on Mac, both in the menu panel and in the toolbar.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

(In reply to Guillaume C. [:ge3k0s] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Please don't prio the bugs you file.
> 
> Ok so just marking australis-cust is now enough ?

For now, yep - that'll be enough to get them on our radar. Thanks for filing. :)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8387232 [details] [diff] [review]
Patch v1

Works exactly as advertised. Thanks phlsa!

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8387232 [details] [diff] [review]
Patch v1

Withdrawing my r+ - on a second look (thanks Gijs!), we noticed that this text is not exactly centered due to the padding being added.

Why is this padding being added? Do we know?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Blake - the text-align:center and padding was added in bug 972405. Do you know why that's needed?

Comment 8

[Blake Winton (:bwinton) (:☕️)]

As I remember it…
The "-moz-padding-end" was added to provide some space between the item and the shortcut.
The "text-align: center" was removed because the box doesn't take up all the extra space anymore (due to the "-moz-box-flex: 0;"  (Instead, it's centered by the "-moz-box-pack: center;" on ".cui-widget-panelview .subviewbutton.panel-subview-footer".)
And I did it that way because we want both the item text and the shortcut key to be centered (together, with some space between them), so neither of them can have "-moz-box-flex: 1".

I suggest changing the centering selector to ".cui-widget-panelWithFooter .subviewbutton.panel-subview-footer" to fix this.

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v1.1

So after a lot of back and forth, this seems to fix the issue.
It works with the bookmarks and history menu both in the toolbar and in the panel (tested on mac).

Comment 10

[Blake Winton (:bwinton) (:☕️)]

As a question, what went wrong when you tried the ".cui-widget-panelWithFooter .subviewbutton.panel-subview-footer" selector?
(I'm a little hesitant to re-add the previously-unnecessary selector…)

Comment 11

[Blake Winton (:bwinton) (:☕️)]

(As a side note, what's going wrong is that the label is taking up the entire footer, instead of just the space it needs.  I'm not sure why, but Philipp is going to look into it a little.)

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

I played with it a little more, but I can't find a way to make it do the right thing.
Since every other approach I found seems equally or more hacky than the one in the last patch, I'll leave that here for review.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Yeah, ditto - after some poking, I think we're seeing some XUL layout funk. Blake explained how this is supposed to work (-moz-box-align: center _should_ be centering the label, since the label is the only child of the element with that rule).

However, it doesn't center it. I was able to trigger the correct behaviour by appending a sibling element to the label, and then removing it again.

Classic XUL layout.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Screenshot of Patch v1.1 on OS X.

Hm, still doesn't appear to be fully centered with this patch...

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8387335 [details] [diff] [review]
Patch v1.1

It's close, but not quite centered yet. I'm pretty sure it's the padding.

Comment 16

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Mike Conley (:mconley) from comment #15)
> Comment on attachment 8387335 [details] [diff] [review]
> Patch v1.1
> 
> It's close, but not quite centered yet. I'm pretty sure it's the padding.

Hm, it works for me:
http://cl.ly/image/3N002q0W061V
http://cl.ly/image/1M3C00073o3Z

Where specifically are you seeing an offset?

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Cleared up in meatspace.

Comment 19

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Attachment: Patch v1.2

OK, I think I finally got it. After a clean build, both the bookmarks footer and the history footer are perfectly centered on my OS X machine.

Comment 20

[:Gijs (he/him)]

Philipp, can you do me a favour and check what happens when you set your Windows font to something non-default after your patch? (see bug 980534)

(might not need to be fixed here... but it'd be nice if it was!)

Comment 21

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #20)
> Philipp, can you do me a favour and check what happens when you set your
> Windows font to something non-default after your patch? (see bug 980534)
> 
> (might not need to be fixed here... but it'd be nice if it was!)

I checked with a bunch of fonts. Footers without accelerator keys stay in place quite nicely. But especially with monospace fonts, the modifier keys are often too long to fit the panel (ctrl+shift+b is a lot of text). That wasn't introduced bu this patch though.

Comment 22

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #21)
> (In reply to :Gijs Kruitbosch from comment #20)
> > Philipp, can you do me a favour and check what happens when you set your
> > Windows font to something non-default after your patch? (see bug 980534)
> > 
> > (might not need to be fixed here... but it'd be nice if it was!)
> 
> I checked with a bunch of fonts. Footers without accelerator keys stay in
> place quite nicely. But especially with monospace fonts, the modifier keys
> are often too long to fit the panel (ctrl+shift+b is a lot of text). That
> wasn't introduced bu this patch though.

Sure, just vain hope that it'd be fixed. Anyhow, we can try to sort this out in bug 981548 once this has landed. One patch at a time.

Comment 23

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to Philipp Sackl [:phlsa] from comment #21)
> > (In reply to :Gijs Kruitbosch from comment #20)
> > > Philipp, can you do me a favour and check what happens when you set your
> > > Windows font to something non-default after your patch? (see bug 980534)
> > > 
> > > (might not need to be fixed here... but it'd be nice if it was!)
> > 
> > I checked with a bunch of fonts. Footers without accelerator keys stay in
> > place quite nicely. But especially with monospace fonts, the modifier keys
> > are often too long to fit the panel (ctrl+shift+b is a lot of text). That
> > wasn't introduced bu this patch though.
> 
> Sure, just vain hope that it'd be fixed. Anyhow, we can try to sort this out
> in bug 981548 once this has landed. One patch at a time.

Err, I meant bug 980534.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8388761 [details] [diff] [review]
Patch v1.2

phlsa:

Even with a clobber build, when I apply this patch, the labels are still not centered...

https://www.dropbox.com/s/gi1z64wfpby1uor/Screenshot%202014-03-12%2015.41.15.png

Comment 25

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Mike Conley (:mconley) from comment #24)
> Comment on attachment 8388761 [details] [diff] [review]
> Patch v1.2
> 
> phlsa:
> 
> Even with a clobber build, when I apply this patch, the labels are still not
> centered...
> 
> https://www.dropbox.com/s/gi1z64wfpby1uor/Screenshot%202014-03-12%2015.41.15.
> png

OK, maybe this needs a fresh pair of eyes ;)
I'm still wondering about the cause of the discrepancy between our two systems since font, resolution and just about everything else should be the same.

Comment 26

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #25)
> (In reply to Mike Conley (:mconley) from comment #24)
> > Comment on attachment 8388761 [details] [diff] [review]
> > Patch v1.2
> > 
> > phlsa:
> > 
> > Even with a clobber build, when I apply this patch, the labels are still not
> > centered...
> > 
> > https://www.dropbox.com/s/gi1z64wfpby1uor/Screenshot%202014-03-12%2015.41.15.
> > png
> 
> OK, maybe this needs a fresh pair of eyes ;)
> I'm still wondering about the cause of the discrepancy between our two
> systems since font, resolution and just about everything else should be the
> same.

I *think* it's because the screenshot is from the history panel which doesn't have a shortcut and so there's no menu-accel-container, so no right padding, so it looks uneven, because the toolbarbutton-text presumably gets padding-start from somewhere else (and no longer has it reset to 0 because you moved that selector).

Comment 27

[Mike Conley (:mconley) (:⚙️)]

I know how frustrating it is to work on something when you can't reproduce the problem. I'll see if I can figure this one out.

Comment 28

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #26)
> I *think* it's because the screenshot is from the history panel which
> doesn't have a shortcut and so there's no menu-accel-container, so no right
> padding, so it looks uneven, because the toolbarbutton-text presumably gets
> padding-start from somewhere else (and no longer has it reset to 0 because
> you moved that selector).

Hm, but yesterday when I last modified that patch I took this screenshot (also from the history panel) where the text is aligned correctly: http://cl.ly/image/2n0G1G1X1f16

That is not to say that the lack of a menu-accel-container has nothing to do with it though…

Comment 29

[Mike Conley (:mconley) (:⚙️)]

I have a simple explanation - my screenshot / testing has been on Windows 7. I _believe_ phlsa's screenshots are from OS X. That might explain the discrepancy?

Anyhow, I _think_ I've figured this out for Windows. Patch in a few minutes.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.3

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.3 on Windows 7

Here's the patch on Windows 7. I'll test OS X and Ubuntu next.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.3

Whoops, forgot to qref.

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8391395 [details] [diff] [review]
Patch v1.3

This seems to do the job.

Comment 34

[Tim Nguyen :ntim]

(In reply to Mike Conley (:mconley) from comment #15)
> Comment on attachment 8387335 [details] [diff] [review]
> Patch v1.1
> 
> It's close, but not quite centered yet. I'm pretty sure it's the padding.

It's nothing related to this bug, but I'm seeing in the screenshot, that buttons are bigger than they are supposed to be (consistent with other platforms)

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8391395 [details] [diff] [review]
Patch v1.3

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

The bookmarks footer is slightly off center still. Here is a screenshot showing it, http://screencast.com/t/jWtmsHhRa

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.4

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8393610 [details] [diff] [review]
Patch v1.4

Ok, this seems to fix the footer alignment for the Bookmarks menu and the History panel.

Thanks for your help over IRC, Jared!

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8393610 [details] [diff] [review]
Patch v1.4

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

Thanks, looks good. You may want to update the patch author though, up to you.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Thanks!

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

Comment 40

[Carsten Book [:Tomcat]]

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

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8393610 [details] [diff] [review]
Patch v1.4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis - specifically, Bug 969592, I believe.


User impact if declined: 

The footer links in the bottom of the History and Bookmarks menus will be off-center, which looks bad.


Testing completed (on m-c, etc.): 

Tested pretty thoroughly on Windows and OS X. A day to bake on m-c.


Risk to taking this patch (and alternatives if risky): 

Pretty low - it's all style, and very self-contained.


String or IDL/UUID changes made by this patch:

None.

Comment 42

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/589c1400785d

https://hg.mozilla.org/releases/mozilla-beta/rev/9c1be3177963

Comment 43

[Francesca Ciceri [:madamezou]]

Attachment: Bookmark subview still not centered in latest Nightly on Linux

Hi,

I was able to reproduce it on Windows 7 x86_64 with Nightly 30.0a1 2014-03-05, and I can confirm it's fixed in latest Nightly (32.0a1 2014-05-29), latest Aurora (31.0a2 2014-05-29) and latest Beta (30.0 2014-05-27) on the same platform.

I also reproduced it in Linux (Debian Sid) x86_64 with the same build as above (30.0a1 2014-03-05) but here the problem is still present for the bookmarks panel.
The History subview is fine in Linux (probably because it doesn't have shortcut and menu-accel-container?).
So to summarize, all the versions (Nightly, Aurora, Beta) are still affected in Linux in the Bookmarks panel. Comparing the two versions (the one from 2014-03-05 and the latest Nightly) it seems to me that the space between the "Show All Bookmarks" and the "Ctrl+Shift+O" has been increased in latest Nightly, (see comparison screenshot).

Let me know if you need more information or testing.

Cheers,
Francesca

Comment 44

[Francesca Ciceri [:madamezou]]

Attachment: Comparison between latest Nightly and one from 2014-03-05 on Linux

Comment 45

[:Gijs (he/him)]

(In reply to Francesca Ciceri [:madamezou] from comment #43)
> Created attachment 8431477 [details]
> Bookmark subview still not centered in latest Nightly on Linux
> 
> Hi,
> 
> I was able to reproduce it on Windows 7 x86_64 with Nightly 30.0a1
> 2014-03-05, and I can confirm it's fixed in latest Nightly (32.0a1
> 2014-05-29), latest Aurora (31.0a2 2014-05-29) and latest Beta (30.0
> 2014-05-27) on the same platform.
> 
> I also reproduced it in Linux (Debian Sid) x86_64 with the same build as
> above (30.0a1 2014-03-05) but here the problem is still present for the
> bookmarks panel.
> The History subview is fine in Linux (probably because it doesn't have
> shortcut and menu-accel-container?).
> So to summarize, all the versions (Nightly, Aurora, Beta) are still affected
> in Linux in the Bookmarks panel. Comparing the two versions (the one from
> 2014-03-05 and the latest Nightly) it seems to me that the space between the
> "Show All Bookmarks" and the "Ctrl+Shift+O" has been increased in latest
> Nightly, (see comparison screenshot).
> 
> Let me know if you need more information or testing.
> 
> Cheers,
> Francesca

Thanks for the detailed information. Would you mind filing a new bug for the linux case, and marking this one as verified?

Comment 46

[Francesca Ciceri [:madamezou]]

(In reply to :Gijs Kruitbosch from comment #45)
> (In reply to Francesca Ciceri [:madamezou] from comment #43)
> 
> Thanks for the detailed information. Would you mind filing a new bug for the
> linux case, and marking this one as verified?

Done: Bug 1018867 (and I've taken the liberty to cc you there).

As for this one: should I mark it as verified even if I haven't verified on a Mac? On the other hand, comment 19 seems to suggest that the patch was fine on Mac.


Cheers,
Francesca

Comment 47

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I've verified this on Mac OS X 10.9.2, and found similar results to the Linux case:
- History subview - is now fine, perfectly centered
- Bookmarks subview - was initially centered on Nightly 2014-03-05, but now is off by a few pixels - I've added this to Bug 1018867 created by Francesca

Verified on latest Nightly (BuildID: 20140603030220), latest Aurora (BuildID: 20140603004003), and latest Beta (BuildID: 20140529161749).

Based on comment 45, and Francesca's testing, I'm marking this as Verified for all versions.