879162 - The gap between the menu bar and the tabs isn't the same when you press alt

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[u428464]

Attachment: Menu bar different gap

In the latest UX builds the gap between the tabs and the menu bar has been reduced has expected, but I've encounter a bug. When the menu bar is enabled the menu bar seem too close to the tabs, especially if you consider the same gap when making the menu bar appears with the alt-shortcut. I don't know which should be the default (although I prefer the the bigger one) but both should be the same.

Comment 1

[:Gijs (he/him)]

How do you enable the menubar "by default"? I looked for a pref and didn't find one. :-\

Comment 2

[u428464]

(In reply to :Gijs Kruitbosch from comment #1)
> How do you enable the menubar "by default"? I looked for a pref and didn't
> find one. :-\

Sorry I wasn't clear. I meant make it visible all the time via the tab strip context menu.

Comment 3

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > How do you enable the menubar "by default"? I looked for a pref and didn't
> > find one. :-\
> 
> Sorry I wasn't clear. I meant make it visible all the time via the tab strip
> context menu.

OK. I can reproduce this. As a workaround, right click the tabstrip again, hide the menubar, hit alt, and use the view > toolbars menu to unhide the menubar.

Comment 4

[:Gijs (he/him)]

Attachment: Patch

Dao asked me in the original bug if having :not([autohide="true"]) in the selector was really necessary. I thought, like him, that it ought not to be.

This fixes it so that it really isn't. There are other selectors in browser.css that may be simplified if this lands, so ultimately I think this is a better fix than putting the attribute back in the selector.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 757849 [details] [diff] [review]
Patch

Seems like the toolbar-menubar-autohide binding should call this._setActive in its destructor. Would that fix this bug?

Comment 6

[:Gijs (he/him)]

Attachment: Patch

(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 757849 [details] [diff] [review]
> Patch
> 
> Seems like the toolbar-menubar-autohide binding should call this._setActive
> in its destructor. Would that fix this bug?

Sorry, I hadn't looked that far. You're right, that fixes this as well, and is cleaner. I've updated both toolkit and browser, because it seemed to me that keeping them in sync is probably the right thing to do here.

Comment 7

[:Gijs (he/him)]

Pushed: https://hg.mozilla.org/projects/ux/rev/21906afe1080

Comment 8

[u428464]

I don't see this resolved on last tinderbox UX build that should contain the patch.

Comment 9

[:Gijs (he/him)]

(In reply to Guillaume C. [:ge3k0s] from comment #8)
> I don't see this resolved on last tinderbox UX build that should contain the
> patch.

You're right. I did test this, but clearly I did not test it well enough. :-\

Comment 10

[:Gijs (he/him)]

Attachment: Patch to workaround XBL destructors sucking

I could also revert the previous change in addition to this one.

Or I could go with my original solution.

Or we could update the CSS.

I don't like any of these solutions particularly, but I like the CSS fix least because I think our markup should just be consistent rather than us (and theme authors) having to hack around its inconsistencies.

Dao, what do you think?

(root cause: XBL destructors... "might" fire. Sometimes. Usually not, though)

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 758488 [details] [diff] [review]
Patch to workaround XBL destructors sucking

I'm fine with letting the CSS deal with this, but we shouldn't use :-moz-any there like your first patch did.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 757856 [details] [diff] [review]
Patch

Not sure why this was marked obsolete. Are you going to back it out? I think it should stay.

Comment 13

[:Gijs (he/him)]

Attachment: Patch the CSS

OK! :-)

Comment 14

[:Gijs (he/him)]

Comment on attachment 757856 [details] [diff] [review]
Patch

I obsoleted this because I was thinking it may need to be backed out. If you think it should stay, let's have it stay.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 758519 [details] [diff] [review]
Patch the CSS

>+#toolbar-menubar:not([autohide="true"]) ~ #TabsToolbar,
> #toolbar-menubar:not([inactive]) ~ #TabsToolbar {

add [autohide="true"] to the second line for clarity:

#toolbar-menubar[autohide="true"]:not([inactive]) ~ #TabsToolbar {

Comment 16

[:Gijs (he/him)]

Second time lucky, I hope...: https://hg.mozilla.org/projects/ux/rev/7b9226838784

Comment 17

[Alejandro Rodriguez [:Alopepeo]]

Now it causes the tabstrip to go 1px over the nav bar when hitting Alt:

http://www.dropmocks.com/iBteYb

Comment 18

[:Gijs (he/him)]

(In reply to Alejandro Rodriguez [:Alopepeo] from comment #17)
> Now it causes the tabstrip to go 1px over the nav bar when hitting Alt:
> 
> http://www.dropmocks.com/iBteYb

Which build exactly is this, and does a build from http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/ux-win32/1370435175/ have this issue?

Comment 19

[u428464]

I don't see this on any builds at the moment (neither tinderbox nor latest UX).

Comment 20

[Alejandro Rodriguez [:Alopepeo]]

I´m seeing the problem on both builds. Here you can see my latest ux build troubleshooting information. 

http://www.mytextfile.com/public?s=5KEWSrT0cpKUKRiVDr3AhlQMJSNFx1DZ9oH2tlLJ

If anyone need more information, please request it here and I'll answer as soon as I can.

Comment 21

[:Gijs (he/him)]

(In reply to Alejandro Rodriguez [:Alopepeo] from comment #20)
> I´m seeing the problem on both builds. Here you can see my latest ux build
> troubleshooting information. 
> 
> http://www.mytextfile.com/public?s=5KEWSrT0cpKUKRiVDr3AhlQMJSNFx1DZ9oH2tlLJ
> 
> If anyone need more information, please request it here and I'll answer as
> soon as I can.

Can you file a new bug for this, assign to me, and also include any add-ons you use, and which Windows theme you use? Thanks!

Comment 22

[Alejandro Rodriguez [:Alopepeo]]

(In reply to :Gijs Kruitbosch from comment #21)
> Can you file a new bug for this, assign to me, and also include any add-ons
> you use, and which Windows theme you use? Thanks!

I've filed the new bug. Unfortunately, I doesn't have the "editbugs" or "canconfirm" permissions, and therefore I can't assign the issue to you. 

Many thanks for your support! Keep up the good work ;)

Comment 23

[Alejandro Rodriguez [:Alopepeo]]

Sorry, here it is: https://bugzilla.mozilla.org/show_bug.cgi?id=880924

Comment 24

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/21906afe1080
https://hg.mozilla.org/mozilla-central/rev/7b9226838784