454338 - Arrows in bookmarks menu are no longer vertically aligned after landing of bug 433109

Status: NEW

(Toolkit :: Themes, defect)

Description

[Bryan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080908182316 Minefield/3.1b1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080908182316 Minefield/3.1b1pre ID:20080908182316

After patch https://bugzilla.mozilla.org/show_bug.cgi?id=433109, the chevrons in the bookmarks menu for Most Visited, Recent Tags and Recently Bookmarked are not aligned vertically with the user created folder chevrons.  They're one pixel to the right.  Also, the bookmarks toolbar chevron (I moved it there in my case) is also one pixel to the right.

Reproducible: Always

Steps to Reproduce:
1. Launch a recent build of Minefield
2. Click on the Bookmarks menu and observe that the chevrons for Most Visited, Recent Tags and Recently Bookmarked are not aligned vertically with the user created folder chevrons.
Actual Results:  
The chevrons for Most Visited, Recent Tags and Recently Bookmarked are not aligned vertically with the user created folder chevrons.

Expected Results:  
The chevrons for Most Visited, Recent Tags and Recently Bookmarked are not aligned vertically with the user created folder chevrons.

Comment 1

[Bryan]

Attachment: Screenshot of issue...

Comment 2

[Damian Shaw [Quan]]

No need to put the full URL, if anything at all to be honest :)

Comment 3

[Damian Shaw [Quan]]

This really should be blocking, it affects livebookmark folders as well and in a menu with lots of different folders mixed up it just makes things look untidy.

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

this bug is eligible for bug 462081

Comment 5

[(not currently active) Ted Mielczarek]

Not blocking, but we should fix this. (And Faaborg will buy you a beer!)

Comment 6

[Damian Shaw [Quan]]

Attachment: Screenshot without seperating horizontal lines

The original screenshot doesn't quite make the impact I see all the time. Here you can see what a mess it looks without horizontal lines separating the folders.

Comment 7

[Nochum Sossonko [:Natch]]

Attachment: Here comes the beer... ;-)

Dunno if this is the proper way to do it, but this fixes the problem for me although I have an icky feeling that it was done like that for a reason :(.

The problem is that the min-width doesn't restrict the arrow to a certain size, then my guess is that the icon differences (regular folder or query folder) affect the whole node resulting in this bad behavior (pure guess as that really is the only difference between the two menu items).

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Urgh, menu CSS :/

There's a long history of bugs with this code, and I'm very wary of making further changes to it without careful review that it doesn't regress any of them (CVS/Hg blame is useful here). Ideally we could write a test to ensure that the alignment is always right for the various menu types, but I'm not sure how much trouble that would be... It doesn't help that e.g. the bookmarks menu has its own styling.

Comment 9

[Nochum Sossonko [:Natch]]

Kai: any idea why you went with min-width here as opposed to just width? (and can you address comment 8)?

Comment 10

[Damian Shaw [Quan]]

Hey, don't suppose there's an update on this at all? Would be really nice to hit before 3.1 :).

Comment 11

[Nochum Sossonko [:Natch]]

Gavin: regarding the CVS Blame, I did a whole bunch of digging, it seems that originally there was an 8px width on .menu-right, which was later removed then modified to min-width 1.28em, which was then again modified to 1.45em, later to be reverted. However, and I am unsure if this is the actual problem, when the move to 1.45em was done .menu-iconic-left was affected as well, then when the 1.45em was reverted .menu-iconic-left was left with 1.45em. Somewhat confusing but following revision after revision (not to mention cvs->hg switches in the middle) was a helluva lot more mind-numbing ;) . The question basically is: is there anything else besides the arrow that is supposed to fit in .menu-right? If not, then this patch is fine as there should be no reason .menu-right needs more width.

That said, I've been using this in my local build and I haven't seen any problems so far with it. I also tested XP and Vista (no changes to Vista as this bug was never there) and it seemed fine. If anyone familiar with that code could point out exactly what the min-width is providing room for, please do say.

Finally, the min-width was added in bug 313388, of course there .menu-iconic-left was also part of the story. Ems are used so that the width adjusts with the font-size, I think this patch is very safe, although I still wasn't able to fully track down the bookmarks menu specificity of this.

Comment 12

[Nochum Sossonko [:Natch]]

Gavin: can we try this out on 1.9.2 now so that if it regresses anything it'll be pulled by the time 1.9.2 is released? Can't really find anymore info...

Comment 13

[Nochum Sossonko [:Natch]]

Attachment: Ok, remove the winstripe bookmark stuff

I think the previous patch should be taken regardless, but here's one for you guys to try (don't have XP atm). All this bookmark specificity seems like crap and most of it was landed with "land winstripe on trunk" as the comments, so I can't really understand why it's there.

Some of it was added in bug 221824, not sure why. Also of interest might be this: http://mxr.mozilla.org/mozilla-central/search?find=%2F&string=bookmark-item-microsummarized it's a defined style for microsummaries but it seems linke it's never used, huh?

Comment 14

[Nochum Sossonko [:Natch]]

Comment on attachment 362070 [details] [diff] [review]
Ok, remove the winstripe bookmark stuff

Actually, I'll forward review to mano since he put some of this code in.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 362070 [details] [diff] [review]
Ok, remove the winstripe bookmark stuff

This looks like it should stay (see bug 363130), although I'd say it belongs to browser/base/content/browser.css:

.bookmark-item > .toolbarbutton-icon {
  width: 16px;
  height: 16px;
}

In any case, what does that patch have to do with this bug?

Comment 16

[Nochum Sossonko [:Natch]]

Well that can stay regardless, I guess I was getting a bit over zealous. The rest changes the way the root bookmark submenu items are displayed and are probably the cause of this bug. Notice, it doesn't happen with any other menu and is a discrepancy between the built-in root bookmark submenus and regular bookmark folders.

Note that even for the code in the previous comment http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/winstripe/global/menu.css#117 would be sufficient if only the bookmark menu actually followed the regular menu conventions.

Comment 17

[Dão Gottwald [:dao]]

Umm, attachment 362070 [details] [diff] [review] is about the bookmarks toolbar, and this bug is about the bookmarks menu, right?

Comment 18

[Nochum Sossonko [:Natch]]

huh, I meant to nuke some of the cruft below that about the menus... I'll have to try to look at this another time maybe, it's getting too annoying for me :)

I still don't understand the difference between vista->xp probably one of the -aero.css files...

Comment 19

[Alex Faaborg [:faaborg] (Firefox UX)]

This bug's priority relative to the set of other polish bugs is:
P2 - Polish issue that is in a secondary interface, occasionally encountered, and is easily identifiable.

Comment 20

[Bryan]

This appears to still be an issue with: Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100922 Firefox/4.0b7pre ID:20100922111040

Any chance in resolution?

~B

Comment 21

[Ian Moody [:Kwan] (UTC+1)]

Attachment: =Increase menu-right min-width

Here's a one-line fix for the bug.  Originally tried fiddling with the -moz-image-region property, since for some reason a value of auto displays the bug while a rect() with the image dimensions doesn't. However that would have required a change for each menu variant, whereas this is simpler and seems to address the problem more directly. (P.S. if this goes horrbily wrong I apologise, first time using bzexport)

Comment 22

[toe_head2001]

I'd say this should be marked as fixed.

Comment 23

[[e-mail]]

So this is actually bug 313388's fault.

Basically it removed .menu-right { -moz-image-region: auto; } from menu.css

Without that, the -moz-image-region inherits from the parent menu(item).

And this forces the .menu-right to increase its size to that of the region.

Comment 24

[[e-mail]]

(In reply to Ian Moody from comment #21)
> Originally tried fiddling with the -moz-image-region property
Maybe you fiddled with it in the wrong place?

Comment 25

[Liviu Cirdei]

User Agent 	Mozilla/5.0 (Windows NT 5.1; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID 	20160516030211

Still reproducible on latest Nightly (49.0a1).