Closed Bug 979987 Opened 7 years ago Closed 7 years ago

Subviews footers text isn't centered anymore

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- fixed
firefox30 --- verified
firefox31 --- verified

People

(Reporter: u428464, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3-])

Attachments

(5 files, 5 obsolete files)

Probably as a side effect of bug 972405 the subviews footers (History and Bookmarks) aren't centered anymore.
Summary: Subviews footers text isn't centered anymore → Subviews footers text aren't centered anymore
Whiteboard: [Australis:P3]
Please don't prio the bugs you file.
Whiteboard: [Australis:P3] → [Australis:P3-]
(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 ?
Attached patch Patch v1 (obsolete) — Splinter Review
Tested with the bookmarks and history panel on Mac, both in the menu panel and in the toolbar.
Assignee: nobody → philipp
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8387232 - Flags: review?(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 on attachment 8387232 [details] [diff] [review]
Patch v1

Works exactly as advertised. Thanks phlsa!
Attachment #8387232 - Flags: review?(mconley) → review+
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?
Attachment #8387232 - Flags: review+ → review-
Blake - the text-align:center and padding was added in bug 972405. Do you know why that's needed?
Flags: needinfo?(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.
Flags: needinfo?(bwinton)
Attached patch Patch v1.1 (obsolete) — Splinter Review
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).
Attachment #8387232 - Attachment is obsolete: true
Attachment #8387335 - Flags: review?(mconley)
Attachment #8387335 - Flags: feedback?(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…)
Summary: Subviews footers text aren't centered anymore → Subviews footers text isn't centered anymore
(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.)
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.
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.
Hm, still doesn't appear to be fully centered with this patch...
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.
Attachment #8387335 - Flags: review?(mconley) → review-
(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?
Flags: needinfo?(mconley)
Cleared up in meatspace.
Flags: needinfo?(mconley)
Duplicate of this bug: 981288
Attached patch Patch v1.2 (obsolete) — Splinter Review
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.
Attachment #8387335 - Attachment is obsolete: true
Attachment #8387335 - Flags: feedback?(bwinton)
Attachment #8388761 - Flags: review?(mconley)
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!)
(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.
(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.
(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 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
Attachment #8388761 - Flags: review?(mconley) → review-
(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.
(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).
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.
Assignee: philipp → mconley
(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…
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.
Attached patch Patch v1.3 (obsolete) — Splinter Review
Attachment #8388761 - Attachment is obsolete: true
Attached image Patch v1.3 on Windows 7
Here's the patch on Windows 7. I'll test OS X and Ubuntu next.
Attachment #8391393 - Attachment is patch: false
Attachment #8391393 - Attachment mime type: text/plain → image/png
Attached patch Patch v1.3 (obsolete) — Splinter Review
Whoops, forgot to qref.
Attachment #8391392 - Attachment is obsolete: true
Comment on attachment 8391395 [details] [diff] [review]
Patch v1.3

This seems to do the job.
Attachment #8391395 - Flags: review?(jaws)
(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 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
Attachment #8391395 - Flags: review?(jaws)
Attached patch Patch v1.4Splinter Review
Attachment #8391395 - Attachment is obsolete: true
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!
Attachment #8393610 - Flags: review?(jaws)
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.
Attachment #8393610 - Flags: review?(jaws) → review+
Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/cf4a39712334
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/cf4a39712334
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 31
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.
Attachment #8393610 - Flags: approval-mozilla-beta?
Attachment #8393610 - Flags: approval-mozilla-aurora?
Attachment #8393610 - Flags: approval-mozilla-beta?
Attachment #8393610 - Flags: approval-mozilla-beta+
Attachment #8393610 - Flags: approval-mozilla-aurora?
Attachment #8393610 - Flags: approval-mozilla-aurora+
QA Whiteboard: [good first verify]
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
QA Whiteboard: [good first verify] → [good first verify] [testday-20140530]
(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?
Flags: needinfo?(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
Flags: needinfo?(madamezou)
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.
You need to log in before you can comment on or make changes to this bug.