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

NEW
Unassigned

Status

()

defect
--
trivial
11 years ago
3 years ago

People

(Reporter: BoxerBoi76, Unassigned)

Tracking

({polish, regression})

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [polish-easy][polish-visual][polish-p2])

Attachments

(4 attachments, 1 obsolete attachment)

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.
Blocks: 433109
No longer blocks: 433109
Depends on: 433109
Version: unspecified → Trunk
Flags: wanted1.9.1?
Keywords: polish, regression
No need to put the full URL, if anything at all to be honest :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Chevrons in bookmarks menu are no longer vertically aligned after landing of bug https://bugzilla.mozilla.org/show_bug.cgi?id=433109 → Chevrons in bookmarks menu are no longer vertically aligned after landing of bug 433109
Blocks: 436779
Blocks: 433109
No longer depends on: 433109
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.
Flags: blocking1.9.1?
Whiteboard: [polish-easy] [polish-visual]
Whiteboard: [polish-easy] [polish-visual] → [polish-easy][polish-visual][polish-high-visibility
this bug is eligible for bug 462081
Whiteboard: [polish-easy][polish-visual][polish-high-visibility → [polish-easy][polish-visual][polish-high-visibility]
Not blocking, but we should fix this. (And Faaborg will buy you a beer!)
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1?
Flags: blocking1.9.1-
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.
Posted patch Here comes the beer... ;-) (obsolete) — Splinter Review
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).
Attachment #345420 - Flags: review?(gavin.sharp)
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.
Kai: any idea why you went with min-width here as opposed to just width? (and can you address comment 8)?
Hey, don't suppose there's an update on this at all? Would be really nice to hit before 3.1 :).
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.
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...
Flags: wanted1.9.2?
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?
Assignee: nobody → highmind63
Attachment #362070 - Flags: review?(gavin.sharp)
Whiteboard: [polish-easy][polish-visual][polish-high-visibility] → [polish-easy][polish-visual][polish-high-visibility][needs review gavin]
Attachment #362070 - Flags: review?(gavin.sharp) → review?(mano)
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.
Whiteboard: [polish-easy][polish-visual][polish-high-visibility][needs review gavin] → [polish-easy][polish-visual][polish-high-visibility][needs review mano]
Summary: Chevrons in bookmarks menu are no longer vertically aligned after landing of bug 433109 → Arrows in bookmarks menu are no longer vertically aligned after landing of bug 433109
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?
Attachment #362070 - Flags: review?(mano) → review-
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.
Umm, attachment 362070 [details] [diff] [review] is about the bookmarks toolbar, and this bug is about the bookmarks menu, right?
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...
Assignee: highmind63 → nobody
Attachment #345420 - Attachment is obsolete: true
Attachment #345420 - Flags: review?(gavin.sharp)
Whiteboard: [polish-easy][polish-visual][polish-high-visibility][needs review mano] → [polish-easy][polish-visual][polish-high-visibility]
Component: XUL Widgets → Themes
QA Contact: xul.widgets → themes
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.
Whiteboard: [polish-easy][polish-visual][polish-high-visibility] → [polish-easy][polish-visual][polish-p2]
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
Flags: wanted1.9.2?
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)
Attachment #593213 - Flags: review?(dao)
I'd say this should be marked as fixed.
Attachment #593213 - Flags: review?(neil)
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.
Blocks: 313388
(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?
Attachment #593213 - Flags: review?(neil) → review-
Attachment #593213 - Flags: review?(dao)
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).
You need to log in before you can comment on or make changes to this bug.