574708 - Deformed back button when bookmarks button is on navigation bar

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dave Townsend [:mossop]]

Something is wrong with toolbar buttons and customisation. Opening toolbar customisation makes things look strange too but afterwards it all looks normal. Restarting makes the problem come back.

On startup: http://grab.by/58Go
Note the weird back/forward buttons, I also have a new bookmarks button to the right of the location bar.

During toolbar customisation: http://grab.by/58Gp
Note the normal back/forward buttons but offset and slightly hidden.

After toolbar customisation: http://grab.by/58Gq
Back/forward are now normal but now my bookmarks button has moved down to the bookmarks toolbar.

Comment 1

[Marco Bonardo [:mak]]

(In reply to comment #0)
> Note the weird back/forward buttons, I also have a new bookmarks button to the
> right of the location bar.

We got request to have the button on by default on Mac because window is detached from the menu (and looks like same will be in gnome 3.0). Actually if button would be removable this would be less than a issue.

> After toolbar customisation: http://grab.by/58Gq
> Back/forward are now normal but now my bookmarks button has moved down to the
> bookmarks toolbar.

this is expected, the button is on navbar but it's moved to bookmarks items if the latter is visible. The fact it moves during customization is just due to the fact it lives in navbar. sounds like a minor issue right now.

The fact back button gets vertically "compressed" instead is weird and pretty bad

Comment 3

[Marco Bonardo [:mak]]

actually the right thing to do here would probably be to use "menu-vertical" and not "menu", solving bug 354616.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I have to check if we need to get some styling from menu or menu-button, but trying it on windows and mac it works pretty well and has even the correct distance between icon and dropmarker (while the horizontal hack had not).
I'm unsure why this is called menu-vertical rather than menu-horizontal though.

Comment 5

[Dão Gottwald [:dao]]

(In reply to comment #3)
> actually the right thing to do here would probably be to use "menu-vertical"
> and not "menu", solving bug 354616.

How exactly is this related to this bug?

Comment 6

[Marco Bonardo [:mak]]

it's related because that was a binding made to implement this kind of menu button with the dropmarker on side rather than bottom.
Btw clearly if we could kill that unused binding and emulate through css would be even better. So both solution involve that bug (either using or killing the unused bindind).

I can't find the toolbarbutton-icon rule that screws up the toolbar (setting display:none on the .toolbarbutton-icon in the "menu" toolbarbutton solves the issue that is actually that toolbar height gets compressed to height of the button when we set -moz-box-horizontal on it).

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> I can't find the toolbarbutton-icon rule that screws up the toolbar (setting
> display:none on the .toolbarbutton-icon in the "menu" toolbarbutton solves the
> issue that is actually that toolbar height gets compressed to height of the
> button when we set -moz-box-horizontal on it).

I bet -moz-box-align:center from bug 559033 is the culprit, although XUL layout is clearly broken if the combination with -moz-box-orient:horizontal on some nested node has such an impact.

The menu-vertical binding has the downside of breaking child selectors trying to reach into the anonymous content. On top of that, I don't think we want to introduce another toolbarbutton type.

Comment 8

[Marco Bonardo [:mak]]

I'm talking with Gavin about the fact we have a moz-box-vertical on toolbarbutton-1 buttons, and since the binding defines children inline they are usually all vertical or all horizontal.
-vertical binding is the only one that correctly groups (icon text)(dropmarker).
I agree that if we can kill the binding it would be better. Testing touching that -mox-box-align: center now

Comment 9

[Marco Bonardo [:mak]]

hm no, removing that rule does not change a thing.

Comment 10

[Dão Gottwald [:dao]]

There must be something that makes this Mac-only, though.
It is Mac-only, isn't it?

Comment 11

[Marcia Knous [:marcia]]

Yes, as far as I can see it is Mac only.

(In reply to comment #10)
> There must be something that makes this Mac-only, though.
> It is Mac-only, isn't it?

Comment 12

[Marco Bonardo [:mak]]

I'm trying to check interesting changes that were made in bug 559033 to check if something changes.

Comment 13

[Marco Bonardo [:mak]]

interesting, see this comm-central definition

http://mxr.mozilla.org/comm-central/source/mail/themes/pinstripe/mail/primaryToolbar.css#67

Comment 14

[Markus Stange [:mstange]]

The max-height on the toolbarbutton icon contributes to the problem. Removing it fixes things.

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v2.0

Actually sounds like the headache is due to this max-height rule, not sure what's its purpose there.

Comment 16

[Marco Bonardo [:mak]]

uarg, if I'd read comment 14 first I'd have saved 20 minutes!

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 454196 [details] [diff] [review]
patch v2.0

asking review to markus to since it's quite late for dao. Review on a first-come-first-serve base.

I have no idea when b1 freeze will start, so the first we can fix this glitch, better is.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 454196 [details] [diff] [review]
patch v2.0

If you change max-height to height, you probably also want to change max-width (below) to width...

The purpose is to make extension icons fit.

Comment 20

[Marco Bonardo [:mak]]

Attachment: patch v2.1

makes sense

Comment 21

[Markus Stange [:mstange]]

(In reply to comment #17)
> (From update of attachment 454196 [details] [diff] [review])
> asking review to markus to since it's quite late for dao.

Dão and I are in the same timezone, as far as I know. :)

As for the patch: I think I hit exactly the same bug with max-width in icons/text mode, just on the horizontal axis instead of the vertical one. (That's what the comment above that second rule is trying to say.)
So after changing max-width to width, the width declaration can probably be moved up to the height rule. I haven't tested this yet, though, so the current patch is good for a quick beta landing.

Comment 22

[Marco Bonardo [:mak]]

Attachment: patch v2.2

I tested all modes and it feels like working pretty fine.

Comment 23

[Marco Bonardo [:mak]]

Attachment: horizontal suggestion

Actually the button looks really ugly in text or icons and text mode, so i suggest going horizontal on all modes.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 454266 [details] [diff] [review]
horizontal suggestion

>-toolbar[mode="icons"] #bookmarks-menu-button.toolbarbutton-1 {
>+toolbar #bookmarks-menu-button.toolbarbutton-1 {
>   -moz-box-orient: horizontal;
> }

remove "toolbar "

Comment 25

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/9525758dabf3
http://hg.mozilla.org/mozilla-central/rev/3cce4a13d6da