Closed Bug 879162 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: u428464, Assigned: Gijs)

References

Details

(Whiteboard: [Australis:M6])

Attachments

(3 files, 2 obsolete files)

Attached image 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.
Whiteboard: [Australis:M?]
How do you enable the menubar "by default"? I looked for a pref and didn't find one. :-\
(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.
(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.
Depends on: 870849
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #757849 - Flags: review?(dao)
Blocks: 870849
No longer depends on: 870849
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?
Attached patch PatchSplinter Review
(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.
Attachment #757849 - Attachment is obsolete: true
Attachment #757849 - Flags: review?(dao)
Attachment #757856 - Flags: review?(dao)
Attachment #757856 - Flags: review?(dao) → review+
Pushed: https://hg.mozilla.org/projects/ux/rev/21906afe1080
Whiteboard: [Australis:M?] → [Australis:M6][fixed-in-ux]
I don't see this resolved on last tinderbox UX build that should contain the patch.
(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. :-\
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
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)
Attachment #757856 - Attachment is obsolete: true
Attachment #758488 - Flags: review?(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.
Attachment #758488 - Flags: review?(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.
Attachment #757856 - Attachment is obsolete: false
Attached patch Patch the CSSSplinter Review
OK! :-)
Attachment #758488 - Attachment is obsolete: true
Attachment #758519 - Flags: review?(dao)
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.
Attachment #757856 - Flags: checkin+
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 {
Attachment #758519 - Flags: review?(dao) → review+
Second time lucky, I hope...: https://hg.mozilla.org/projects/ux/rev/7b9226838784
Whiteboard: [Australis:M6] → [Australis:M6][fixed-in-ux]
Now it causes the tabstrip to go 1px over the nav bar when hitting Alt:

http://www.dropmocks.com/iBteYb
(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?
I don't see this on any builds at the moment (neither tinderbox nor latest UX).
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.
(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!
(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 ;)
https://hg.mozilla.org/mozilla-central/rev/21906afe1080
https://hg.mozilla.org/mozilla-central/rev/7b9226838784
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M6][fixed-in-ux] → [Australis:M6]
Target Milestone: --- → Firefox 28
Depends on: 966698
You need to log in before you can comment on or make changes to this bug.