Closed Bug 813802 Opened 12 years ago Closed 11 years ago

[Win] Tabs and menubar in the titlebar for restored windows

Categories

(Firefox :: Theme, enhancement)

Product:
Firefox
Component:
Theme
Platform:
All
Windows XP
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 28

People

(Reporter: MattN, Assigned: mconley)

References

Details

(Keywords: addon-compat)

Attachments

(5 files, 31 obsolete files)

margin
17.46 KB, image/png
Details
First app tab a few pixels too much on the right ?
31.25 KB, image/png
Details
Patch v3.7 - v3.8 interdiff
1.83 KB, patch
Details | Diff | Splinter Review
Patch v3.8
20.73 KB, patch
MattN
: review+
Details | Diff | Splinter Review
screenshot
8.52 KB, image/png
Details
Attached patch WIP 1 - styling changes (obsolete) — — Splinter Review
On XP the titlebar needs to be behind the tabs when tabs are on top. This will require changing TabsInTitlebar (in browser.js) to work when sizemode != "restored" along with some theme changes. XP Designs: * Luna: http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windowsXP-lunaBlue-mainWindow.html * Silver: http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windowsXP-silver-mainWindow.html * Olive: http://people.mozilla.com/~shorlander/files/australis-designSpecs/australis-designSpecs-windowsXP-olive-mainWindow.html
I don't think we need this, since we don't hide the menu bar by default on XP -- which I don't is a can of worms we should open either. XP likely has its share of conservative users, but it's also declining, so this isn't something we need to actively solve. It would be unneeded user irritation.
With bug 813808, the menubar would be to the right of the app button and so the menubar shouldn't affect whether the titlebar appears behind the tabs. Even without the menubar change, I don't see why we wouldn't want the titlebar color behind the menubar. We'd also want the theme to look good when Classic users disable the menubar. This bug also applies to Windows Classic theme on Vista/7 as well.
I don't see how the things you mention are relevant for whether we put tabs in the title bar in restored windows on XP.
(In reply to Dão Gottwald [:dao] from comment #3) > I don't see how the things you mention are relevant for whether we put tabs > in the title bar in restored windows on XP. It seems like we're not understanding each other. Another way of looking at this bug is: Always draw our own titlebar on Windows. Drawing our own titlebar is necessary to put tabs there. I'm trying to achieve what the design spec shows and I'm not sure what exactly you are disagreeing with. Are you saying that getting the titlebar behind the tabs and menubar with the Windows Classic theme isn't worth the effort or do you think that the design will bother conservative users? (In reply to Dão Gottwald [:dao] from comment #1) > I don't think we need this, since we don't hide the menu bar by default on > XP -- which I don't is a can of worms we should open either. XP likely has > its share of conservative users, but it's also declining, so this isn't > something we need to actively solve. It would be unneeded user irritation. I wasn't planning on changing the default. This bug is about getting the Windows titlebar gradient displayed behind the tabstrip and menubar with tabs on top. If you think the titlebar colour behind the tabs will be irritating to some users then that should be discussed with Stephen and can be reflected in the design.
(In reply to Matthew N. [:MattN] from comment #4) > Are you saying that getting the titlebar behind the tabs and menubar with > the Windows Classic theme isn't worth the effort or do you think that the > design will bother conservative users? Both, potentially. I have however not seen any Windows Classic mockups. > I wasn't planning on changing the default. This bug is about getting the > Windows titlebar gradient displayed behind the tabstrip and menubar with > tabs on top. So then I haven't seen the relevant XP mockups either. Even with the menu bar displayed by default, it sounds like something that would visually be pretty alien on XP.
(In reply to Dão Gottwald [:dao] from comment #5) > (In reply to Matthew N. [:MattN] from comment #4) > > I wasn't planning on changing the default. This bug is about getting the > > Windows titlebar gradient displayed behind the tabstrip and menubar with > > tabs on top. > > So then I haven't seen the relevant XP mockups either. Even with the menu > bar displayed by default, it sounds like something that would visually be > pretty alien on XP. I think it has been planned for a long time though (bug 625307). There are mockups in the first comment (I may have misunderstood what this this bug is about though).
(In reply to Dão Gottwald [:dao] from comment #5) > (In reply to Matthew N. [:MattN] from comment #4) > > Are you saying that getting the titlebar behind the tabs and menubar with > > the Windows Classic theme isn't worth the effort or do you think that the > > design will bother conservative users? > > Both, potentially. I have however not seen any Windows Classic mockups. Sorry, where I said Classic in this bug, I was including XP themes. I also haven't seen Classic mockups but I don't care either way whether they follow the XP specs or not. > > I wasn't planning on changing the default. This bug is about getting the > > Windows titlebar gradient displayed behind the tabstrip and menubar with > > tabs on top. > > So then I haven't seen the relevant XP mockups either. Even with the menu > bar displayed by default, it sounds like something that would visually be > pretty alien on XP. The mockups in bug 625307 show the titlebar colours behind the menubar although those mockups also show the window title which I wasn't planning on showing. I guess this bug is mostly a dupe of bug 625307 (thanks Guillaume).
Blocks: 813808
I'll see if I can drive this one.
Assignee: nobody → mconley
Oops, I thought this was assigned this to me. I have an updated WIP patch which works visually in everything but fullscreen mode. I also need to have it recalculate when the sizemode changes. Dragging the titlebar also doesn't work in some cases yet. The WIP also covers bug 813802 at the moment since the menubar is used for positioning the tabs below it. I'll attach the patch ASAP.
Currently you have to toggle browser.tabs.drawInTitlebar off and on again to get it to recalculate but otherwise it should work visually in restored and maximized windows.
Attachment #683793 - Attachment is obsolete: true
Attached patch WIP Patch 3 (obsolete) — — Splinter Review
I think I'm making progress here. I'm still seeing some funky appearance when starting up in the restored state, but I'm getting close, I think.
Attachment #690962 - Attachment is obsolete: true
(In reply to Mike Conley (:mconley) from comment #11) > Created attachment 691496 [details] [diff] [review] > WIP Patch 3 > > I think I'm making progress here. I'm still seeing some funky appearance > when starting up in the restored state, but I'm getting close, I think. Just for clarity's sake, what I'm seeing is that there's not enough headroom for tabs or the menubar when starting FF in restored state and then maximizing. True for both Windows XP and Windows 7.
(In reply to Mike Conley (:mconley) from comment #12) > (In reply to Mike Conley (:mconley) from comment #11) > > Created attachment 691496 [details] [diff] [review] > > WIP Patch 3 > > > > I think I'm making progress here. I'm still seeing some funky appearance > > when starting up in the restored state, but I'm getting close, I think. > > Just for clarity's sake, what I'm seeing is that there's not enough headroom > for tabs or the menubar when starting FF in restored state and then > maximizing. True for both Windows XP and Windows 7. Yeah, the code is not recalculating at the right times as I mentioned in comment 9. Toggle the pref toggle browser.tabs.drawInTitlebar off and on again and you'll see the code calculates correctly.
Attached patch WIP Patch 4 (obsolete) — — Splinter Review
I don't think we can short-circuit anymore if enabled is already equal to allowed in update() - I think we're going to need to do the margin recalculations each time update() is called.
Attachment #691496 - Attachment is obsolete: true
Comment on attachment 691512 [details] [diff] [review] WIP Patch 4 This seems pretty close. Care to give this patch a spin and see what you think? What cases have I missed?
Attachment #691512 - Flags: feedback?(mnoorenberghe+bmo)
Attached image Screenshot of WIP Patch 4 - Luna Blue (obsolete) —
Attached image Screenshot of WIP Patch 4 - Luna Silver (obsolete) —
Hrm. The end of the silver texture seems to be visible in the titlebar when the window is maximized and the menu is opened. I'll modify the patch to increase the height of the titlebar.
(In reply to Mike Conley (:mconley) from comment #18) > Created attachment 691820 [details] > Screenshot of WIP Patch 4 - Luna Silver > > Hrm. The end of the silver texture seems to be visible in the titlebar when > the window is maximized and the menu is opened. I'll modify the patch to > increase the height of the titlebar. Actually, before I put too much elbow grease into that (since a static fudge-factor won't work, and we'd likely need to sample the height of the menu), I should run these by Stephen to see what he thinks. Stephen - is the end of the titlebar gradient when the menu is opened in Luna Silver a problem? (Note that the menus are still a tad fugly for now - hoping to make those prettier in a follow-up bug).
Flags: needinfo?(shorlander)
Comment on attachment 691512 [details] [diff] [review] WIP Patch 4 Matt - I'd also be interested in your feedback here. Is the gradient behind Luna Silver when the menu is opened on a maximized window undesirable? If so, the only solution I can think of is sampling the height of the toolbar-menubar, and adding that to the margin-bottom of the titlebar. That'd involve us doing a little bit of sleight of hand with the menubar's inactive state (since when inactive, the menu's height is set to 0), but it can be done. Worth doing?
Attached patch Patch v1 (obsolete) — — Splinter Review
Updated screenshots coming...
Attachment #691512 - Attachment is obsolete: true
Attachment #691512 - Flags: feedback?(mnoorenberghe+bmo)
(Just a note for that last patch - I opted to sample the menu height. Wasn't too bad.)
Attached image Screenshot of Patch 1 - Windows 7 - Aero (obsolete) —
I've manually edited these images by cutting out the middle of the maximized windows. All of the important stuff is there.
Comment on attachment 692416 [details] [diff] [review] Patch v1 Some notes from MattN over IRC: * Let's go with the original titlebar margin calculation, and not worry about sampling the menubar height. If Silver really is a problem, we'll fix in a follow-up. We'd rather reudce the amount of JS for calculating this stuff. * The Math.min call that was commented out in Matt's last WIP patch needs to be put back in order to handle the large-font case on Windows where the app button is larger than the tabs.
Comment on attachment 692419 [details] Screenshot of Patch 1 - Windows 7 - Aero I don't think we can seriously consider this an option as long as we have the Firefox button. Winning ~5px on the vertical axis vs. losing ~100px on the left side and another ~110px on the right side seems like a really bad tradeoff.
(In reply to Mike Conley (:mconley) from comment #23) > Created attachment 692419 [details] > Screenshot of Patch 1 - Windows 7 - Aero > > I've manually edited these images by cutting out the middle of the maximized > windows. All of the important stuff is there. How does it look like without the menu button ? A little more glow behnind the menus could be nice too (as seen on this mockup : http://people.mozilla.com/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-MainWindow-%28MenuBar%29.jpg )
(In reply to Dão Gottwald [:dao] from comment #25) > Comment on attachment 692419 [details] > Screenshot of Patch 1 - Windows 7 - Aero > > I don't think we can seriously consider this an option as long as we have > the Firefox button. Winning ~5px on the vertical axis vs. losing ~100px on > the left side and another ~110px on the right side seems like a really bad > tradeoff. Curvy tabs won't be landing on central without Unfocused's menu customization work, which involves moving the App Menu button to the toolbar.
(In reply to Guillaume C. [:ge3k0s] from comment #26) > (In reply to Mike Conley (:mconley) from comment #23) > > Created attachment 692419 [details] > > Screenshot of Patch 1 - Windows 7 - Aero > > > A little more glow behnind the menus could be nice too (as seen on this > mockup : > http://people.mozilla.com/~shorlander/files/australis-design-specs/images/ > Australis-i01-DesignSpec-MainWindow-%28MenuBar%29.jpg ) I agree.
(In reply to Mike Conley (:mconley) from comment #27) > (In reply to Dão Gottwald [:dao] from comment #25) > > Comment on attachment 692419 [details] > > Screenshot of Patch 1 - Windows 7 - Aero > > > > I don't think we can seriously consider this an option as long as we have > > the Firefox button. Winning ~5px on the vertical axis vs. losing ~100px on > > the left side and another ~110px on the right side seems like a really bad > > tradeoff. > > Curvy tabs won't be landing on central without Unfocused's menu > customization work, which involves moving the App Menu button to the toolbar. That's news to me. Bug 738491 is in the final review stages.
More notes to self: We *do* in some cases want to include the menubar in the titlebar-content's margin-bottom - for example, when the autohidden menubar is shown while in full-screen with large fonts. We can try to solve this by recalculating each time the autohidden menubar is shown and hidden (by listening for the DOMMenuBarActive and DOMMenuBarInactive events).
(In reply to Mike Conley (:mconley) from comment #27) > (In reply to Dão Gottwald [:dao] from comment #25) > > Comment on attachment 692419 [details] > > Screenshot of Patch 1 - Windows 7 - Aero > > > > I don't think we can seriously consider this an option as long as we have > > the Firefox button. Winning ~5px on the vertical axis vs. losing ~100px on > > the left side and another ~110px on the right side seems like a really bad > > tradeoff. > > Curvy tabs won't be landing on central without Unfocused's menu > customization work, which involves moving the App Menu button to the toolbar. I don't think that waiting on the new menu was the plan. UX feedback needed on this choice.
(In reply to Guillaume C. [:ge3k0s] from comment #31) > (In reply to Mike Conley (:mconley) from comment #27) > > (In reply to Dão Gottwald [:dao] from comment #25) > > > Comment on attachment 692419 [details] > > > Screenshot of Patch 1 - Windows 7 - Aero > > > > > > I don't think we can seriously consider this an option as long as we have > > > the Firefox button. Winning ~5px on the vertical axis vs. losing ~100px on > > > the left side and another ~110px on the right side seems like a really bad > > > tradeoff. > > > > Curvy tabs won't be landing on central without Unfocused's menu > > customization work, which involves moving the App Menu button to the toolbar. > > I don't think that waiting on the new menu was the plan. UX feedback needed > on this choice. The current plan is for things to live on the UX branch until we have a cohesive set of finalized and polished changes that we can ship at the same time. Instead of individual things slipping into separate releases.
Flags: needinfo?(shorlander)
(In reply to Stephen Horlander from comment #32) > (In reply to Guillaume C. [:ge3k0s] from comment #31) > > (In reply to Mike Conley (:mconley) from comment #27) > > > (In reply to Dão Gottwald [:dao] from comment #25) > > > > Comment on attachment 692419 [details] > > > > Screenshot of Patch 1 - Windows 7 - Aero > > > > > > > > I don't think we can seriously consider this an option as long as we have > > > > the Firefox button. Winning ~5px on the vertical axis vs. losing ~100px on > > > > the left side and another ~110px on the right side seems like a really bad > > > > tradeoff. > > > > > > Curvy tabs won't be landing on central without Unfocused's menu > > > customization work, which involves moving the App Menu button to the toolbar. > > > > I don't think that waiting on the new menu was the plan. UX feedback needed > > on this choice. > > The current plan is for things to live on the UX branch until we have a > cohesive set of finalized and polished changes that we can ship at the same > time. Instead of individual things slipping into separate releases. Ok, thanks for the clarification. Some minor visual changes will be however lauched alone I suppose.
Attached patch Patch v2 (obsolete) — — Splinter Review
We now do the recalculation when the menu is shown or hidden if autohide is set to true. We do this via the DOMMenuBarActive and DOMMenuBarInactive events. I'm also doing some better calculations to make sure we line up the bottom of the tab-strip with the titlebar - that way, large fonts don't make us look awful. I've also added a slight transparency as a background-color for the menu. Not sure if I've set it at the right opacity - might want some UX feedback on that eventually (I'll toss up some screenshots tomorrow).
Attachment #691817 - Attachment is obsolete: true
Attachment #691818 - Attachment is obsolete: true
Attachment #691820 - Attachment is obsolete: true
Attachment #692416 - Attachment is obsolete: true
Attachment #692419 - Attachment is obsolete: true
Attachment #693105 - Flags: feedback?(mnoorenberghe+bmo)
Attached image Patch v2 on Windows XP - Luna Blue (obsolete) —
Attached image Patch v2 on Windows XP - Luna Olive (obsolete) —
Attached image Patch v2 on Windows XP - Luna Silver (obsolete) —
Attached image Patch v2 on Windows 7 - Aero (obsolete) —
Hey Stephen - can you take a peek at those screenshots and see if I'm on the right track with the menu? I'm using a semi-transparent background, and it looks OK on Windows 7 Aero...not sure how I feel about it on XP's Luna.
Flags: needinfo?(shorlander)
Stephen: One other question - when the menu is not autohidden, what should it look like when the window is not focused? Currently, we do this on Windows 7: http://i.imgur.com/UOk4S.png Clearly that's suboptimal. How should it look instead? -Mike
Attached patch Patch v2.1 (obsolete) — — Splinter Review
Hey Dao - care to give this a look?
Attachment #693105 - Attachment is obsolete: true
Attachment #693105 - Flags: feedback?(mnoorenberghe+bmo)
Attachment #693924 - Flags: review?(dao)
Comment on attachment 693924 [details] [diff] [review] Patch v2.1 Redirecting to MattN to take some pressure off of Dao.
Attachment #693924 - Flags: review?(dao) → review?(mnoorenberghe+bmo)
(In reply to Mike Conley (:mconley) from comment #28) > (In reply to Guillaume C. [:ge3k0s] from comment #26) > > (In reply to Mike Conley (:mconley) from comment #23) > > > Created attachment 692419 [details] > > > Screenshot of Patch 1 - Windows 7 - Aero > > > > > A little more glow behnind the menus could be nice too (as seen on this > > mockup : > > http://people.mozilla.com/~shorlander/files/australis-design-specs/images/ > > Australis-i01-DesignSpec-MainWindow-%28MenuBar%29.jpg ) > > I agree. The Australis on-hover styling couldd be use here too as it should be used in the whole UI (e.g. bookmarks bar)
Comment on attachment 693924 [details] [diff] [review] Patch v2.1 Review of attachment 693924 [details] [diff] [review]: ----------------------------------------------------------------- The position of the tabs shift when toggling the menubar in restored windows which is something I wanted to be consistent so it's less of a distraction and it means it's easier to style. That is why I used visibility: hidden in WIP v.2. ::: browser/base/content/tabbrowser.xml @@ +3389,5 @@ > if (aEvent.target != window) > break; > > + TabsInTitlebar.allowedBy("sizemode", true); > + TabsInTitlebar._update(); Nit: it's a bit weird to call an underscore prefixed function here. Also, is the default value for the 1st argument ok in this case? ::: browser/themes/winstripe/browser.css @@ +35,5 @@ > } > > #main-menubar { > -moz-box-flex: 1; /* make menu items expand to fill toolbar height */ > + background-color: rgba(255, 255, 255, 0.2); Nit: remove the spaces between the rgba arguments. @@ +2003,5 @@ > > %ifndef WINSTRIPE_AERO > /* Use lighter colors of buttons and text in the titlebar on lune-blue */ > @media (-moz-windows-theme: luna-blue) { > + #main-window[tabsintitlebar] .tabbrowser-tab:not([selected]) > .tab-stack > .tab-content > .tab-text:not(:-moz-lwtheme), Note that this line was changed in bug 738491. ::: toolkit/content/xul.css @@ +288,5 @@ > } > > toolbar[type="menubar"][autohide="true"][inactive="true"]:not([customizing="true"]) { > min-height: 0 !important; > + height: 0; I would like Dão to take a look at this change because he's the one who added it. Enn is also supposed to take a look at this per the file header.
Attachment #693924 - Flags: review?(mnoorenberghe+bmo) → review-
(In reply to Matthew N. [:MattN] from comment #45) > Comment on attachment 693924 [details] [diff] [review] > Patch v2.1 > > Review of attachment 693924 [details] [diff] [review]: > ----------------------------------------------------------------- > > The position of the tabs shift when toggling the menubar in restored windows > which is something I wanted to be consistent so it's less of a distraction > and it means it's easier to style. That is why I used visibility: hidden in > WIP v.2. Hrm. So, just to be clear: what we actually want is a constant distance between the top of the tab strip and the top of the window, regardless of whether the menu is displayed or not? > > ::: browser/base/content/tabbrowser.xml > @@ +3389,5 @@ > > if (aEvent.target != window) > > break; > > > > + TabsInTitlebar.allowedBy("sizemode", true); > > + TabsInTitlebar._update(); > > Nit: it's a bit weird to call an underscore prefixed function here. > > Also, is the default value for the 1st argument ok in this case? > > ::: browser/themes/winstripe/browser.css > @@ +35,5 @@ > > } > > > > #main-menubar { > > -moz-box-flex: 1; /* make menu items expand to fill toolbar height */ > > + background-color: rgba(255, 255, 255, 0.2); > > Nit: remove the spaces between the rgba arguments. > > @@ +2003,5 @@ > > > > %ifndef WINSTRIPE_AERO > > /* Use lighter colors of buttons and text in the titlebar on lune-blue */ > > @media (-moz-windows-theme: luna-blue) { > > + #main-window[tabsintitlebar] .tabbrowser-tab:not([selected]) > .tab-stack > .tab-content > .tab-text:not(:-moz-lwtheme), > > Note that this line was changed in bug 738491. > > ::: toolkit/content/xul.css > @@ +288,5 @@ > > } > > > > toolbar[type="menubar"][autohide="true"][inactive="true"]:not([customizing="true"]) { > > min-height: 0 !important; > > + height: 0; > > I would like Dão to take a look at this change because he's the one who > added it. Enn is also supposed to take a look at this per the file header.
Flags: needinfo?(mnoorenberghe+bmo)
(In reply to Mike Conley (:mconley) from comment #46) > (In reply to Matthew N. [:MattN] from comment #45) > > Comment on attachment 693924 [details] [diff] [review] > > Patch v2.1 > > > > Review of attachment 693924 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > The position of the tabs shift when toggling the menubar in restored windows > > which is something I wanted to be consistent so it's less of a distraction > > and it means it's easier to style. That is why I used visibility: hidden in > > WIP v.2. > > Hrm. So, just to be clear: what we actually want is a constant distance > between the top of the tab strip and the top of the window, regardless of > whether the menu is displayed or not? Yes, that's what I meant (at least for the default font-size)
Flags: needinfo?(mnoorenberghe+bmo)
Attached patch Patch v3.0 (obsolete) — — Splinter Review
Thanks for the feedback, Matt! (In reply to Matthew N. [:MattN] from comment #45) > Comment on attachment 693924 [details] [diff] [review] > Patch v2.1 > > Review of attachment 693924 [details] [diff] [review]: > ----------------------------------------------------------------- > > The position of the tabs shift when toggling the menubar in restored windows > which is something I wanted to be consistent so it's less of a distraction > and it means it's easier to style. That is why I used visibility: hidden in > WIP v.2. Ok, my mistake. In this patch, we don't push the tab strip down for restored windows. > > ::: browser/base/content/tabbrowser.xml > @@ +3389,5 @@ > > if (aEvent.target != window) > > break; > > > > + TabsInTitlebar.allowedBy("sizemode", true); > > + TabsInTitlebar._update(); > > Nit: it's a bit weird to call an underscore prefixed function here. > Agreed. To keep the patch simple, I've added a public update function that forwards to _update iff CAN_DRAW_IN_TITLEBAR is true. > Also, is the default value for the 1st argument ok in this case? > Yes - the default argument should only be overridden if we're handling a DOMMenuBarActive event. > ::: browser/themes/winstripe/browser.css > @@ +35,5 @@ > > } > > > > #main-menubar { > > -moz-box-flex: 1; /* make menu items expand to fill toolbar height */ > > + background-color: rgba(255, 255, 255, 0.2); > > Nit: remove the spaces between the rgba arguments. Done. > > ::: toolkit/content/xul.css > @@ +288,5 @@ > > } > > > > toolbar[type="menubar"][autohide="true"][inactive="true"]:not([customizing="true"]) { > > min-height: 0 !important; > > + height: 0; > > I would like Dão to take a look at this change because he's the one who > added it. Enn is also supposed to take a look at this per the file header. Alright, I'll request reviews on those changes from Dao and Enn.
Attachment #693415 - Attachment is obsolete: true
Attachment #693416 - Attachment is obsolete: true
Attachment #693417 - Attachment is obsolete: true
Attachment #693418 - Attachment is obsolete: true
Attachment #693419 - Attachment is obsolete: true
Attachment #693924 - Attachment is obsolete: true
Comment on attachment 699866 [details] [diff] [review] Patch v3.0 Dao / Neil: I've made a change to toolkit/content/xul.css in this patch, so I think I'd like one or both of you to make sure it's OK. Basically, for restored windows on winstripe, we want the autohidden, inactive menubars to still retain their height, and then just remove visibility:hidden when they become active. This makes the menu more appear/disappear-y rather than expand/collapse-y. Is it alright? -Mike
Attachment #699866 - Flags: review?(enndeakin)
Attachment #699866 - Flags: review?(dao)
Comment on attachment 699866 [details] [diff] [review] Patch v3.0 >+ /** >+ * Exposes update functionality to outside callers iff CAN_DRAW_IN_TITLEBAR >+ * is true. If not, this function is a no-op. >+ */ >+ update: function update() { >+#ifdef CAN_DRAW_IN_TITLEBAR >+ this._update(); >+#endif >+ }, Remove wordy comment. The dependency on CAN_DRAW_IN_TITLEBAR applies to all public methods of this object. What would be more helpful is giving this method a more self-explaining name. I.e. what exactly being updated here? >+ let deck = $("tab-view-deck"); >+ // Align the bottom of the tabs toolbar with the bottom of the titlebar. >+ deck.style.marginTop = "-" + (tabAndMenuHeight + baseHeight) + "px"; This whole approach makes me cringe. Can you avoid messing with tab-view-deck? >--- a/browser/base/content/tabbrowser.xml >+++ b/browser/base/content/tabbrowser.xml >@@ -3402,19 +3402,18 @@ > switch (aEvent.type) { > case "load": > this.updateVisibility(); > break; > case "resize": > if (aEvent.target != window) > break; > >- let sizemode = document.documentElement.getAttribute("sizemode"); >- TabsInTitlebar.allowedBy("sizemode", >- sizemode == "maximized" || sizemode == "fullscreen"); >+ TabsInTitlebar.allowedBy("sizemode", true); >+ TabsInTitlebar.update(); What's the point of keeping the sizemode criterion in the first place if it's always true? >+#main-window[sizemode="normal"][tabsintitlebar] #toolbar-menubar[autohide="true"][inactive="true"]:not([customizing="true"]) { >+ height: inherit; inherit from where? None of the parents have a height specified. >+ #navigator-toolbox > #toolbar-menubar { >+ background-color: transparent; >+ } What's the point of "#navigator-toolbox > " here? >+ #main-window[tabsintitlebar]:not([sizemode="normal"]):not([inFullscreen]) #toolbar-menubar[inactive] ~ #TabsToolbar:not(:-moz-lwtheme) { > background: -moz-linear-gradient(bottom, @toolbarShadowColor@ 1px, transparent 1px), > -moz-linear-gradient(rgba(50%,50%,50%,0), ActiveCaption 85%); > color: CaptionText; > } Use [sizemode=maximized]?
Attachment #699866 - Flags: review?(dao) → review-
Attached patch Patch v3.1 (obsolete) — — Splinter Review
Thanks for the review Dao! I really appreciate the constructive criticism. (In reply to Dão Gottwald [:dao] from comment #50) > Comment on attachment 699866 [details] [diff] [review] > Patch v3.0 > > >+ /** > >+ * Exposes update functionality to outside callers iff CAN_DRAW_IN_TITLEBAR > >+ * is true. If not, this function is a no-op. > >+ */ > >+ update: function update() { > >+#ifdef CAN_DRAW_IN_TITLEBAR > >+ this._update(); > >+#endif > >+ }, > > Remove wordy comment. The dependency on CAN_DRAW_IN_TITLEBAR applies to all > public methods of this object. > > What would be more helpful is giving this method a more self-explaining > name. I.e. what exactly being updated here? > You're absolutely right - my comment wasn't really providing anything that wasn't immediately obvious. I've removed it, and changed "update" to "updateAppearance", which should clear things up. > >+ let deck = $("tab-view-deck"); > > >+ // Align the bottom of the tabs toolbar with the bottom of the titlebar. > >+ deck.style.marginTop = "-" + (tabAndMenuHeight + baseHeight) + "px"; > > This whole approach makes me cringe. Can you avoid messing with > tab-view-deck? > I wasn't aware that messing with the deck's margin-top was any less desirable than messing with titlebar's margin-bottom. Regardless, I've altered the code to leave deck alone, and apply the changes solely upon titlebar. > >--- a/browser/base/content/tabbrowser.xml > >+++ b/browser/base/content/tabbrowser.xml > >@@ -3402,19 +3402,18 @@ > > switch (aEvent.type) { > > case "load": > > this.updateVisibility(); > > break; > > case "resize": > > if (aEvent.target != window) > > break; > > > >- let sizemode = document.documentElement.getAttribute("sizemode"); > >- TabsInTitlebar.allowedBy("sizemode", > >- sizemode == "maximized" || sizemode == "fullscreen"); > >+ TabsInTitlebar.allowedBy("sizemode", true); > >+ TabsInTitlebar.update(); > > What's the point of keeping the sizemode criterion in the first place if > it's always true? > Good point. Removed. > >+#main-window[sizemode="normal"][tabsintitlebar] #toolbar-menubar[autohide="true"][inactive="true"]:not([customizing="true"]) { > >+ height: inherit; > > inherit from where? None of the parents have a height specified. > Perhaps this and the next point reveal some of the many gaps in my CSS knowledge. Essentially, if I remove height: inherit, then the height gets set to 0 from toolkit's xul.css. What would you suggest I do instead? > >+ #navigator-toolbox > #toolbar-menubar { > >+ background-color: transparent; > >+ } > > What's the point of "#navigator-toolbox > " here? > As above, this one is a little confusing - if I remove #navigator-toolbox, then background-color: transparent; doesn't seem to get applied. Instead (at least on Windows XP), I get a jarring gray background. What would you suggest I do instead? It's not too clear to me what I'm doing wrong, or how better to solve this problem. > >+ #main-window[tabsintitlebar]:not([sizemode="normal"]):not([inFullscreen]) #toolbar-menubar[inactive] ~ #TabsToolbar:not(:-moz-lwtheme) { > > background: -moz-linear-gradient(bottom, @toolbarShadowColor@ 1px, transparent 1px), > > -moz-linear-gradient(rgba(50%,50%,50%,0), ActiveCaption 85%); > > color: CaptionText; > > } > > Use [sizemode=maximized]? Good idea. Done.
Attachment #699866 - Attachment is obsolete: true
Attachment #699866 - Flags: review?(enndeakin)
Attachment #700031 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #50) > >+ #navigator-toolbox > #toolbar-menubar { > >+ background-color: transparent; > >+ } > > What's the point of "#navigator-toolbox > " here? > Ok, I think I've figured this one out, anyway; I need #navigator-toolbox to increase the specificity of my CSS rule. Otherwise, it's beaten out by this rule: http://hg.mozilla.org/projects/ux/file/7cdcbd9433d9/browser/themes/winstripe/browser.css#l58 which, among other things, sets background-color to -moz-Dialog. If my use of #navigator-toolbox is not the preferred method to increase specificity, I'd be happy to hear about alternatives.
This is almost never the preferred method to increase specificity. Stylesheets should be ordered such that overriding rules come after overridden ones or -- if the former is infeasible or insufficient -- you should use !important.
Attached patch Patch v3.2 (obsolete) — — Splinter Review
Ah, thanks for clearing that up. Good to know. I've opted to use !important.
Attachment #700031 - Attachment is obsolete: true
Attachment #700031 - Flags: review?(dao)
Attachment #700451 - Flags: review?(dao)
Attachment #700451 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 700451 [details] [diff] [review] Patch v3.2 Review of attachment 700451 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. I like the allowedBy("sizemode"...) cleanup and the new comments. On Windows 8 with 100% (default) sizes, the tab bar was still shifted in a restored window when I press alt. It's late so I'll look more closely at browser.js/.css tomorrow. r- for now because of resize perf. and tab shifting. ::: browser/base/content/browser.js @@ +4964,5 @@ > + * The element to measure. > + * @return > + * The height as an integer. > + */ > + _outerHeight: function _outerHeight (ele) { Nit: The need to name the function to show in stack traces has been fixed (see http://javascript-reverse.tumblr.com/post/34328529526/javascript-function-name-inference-aka-stop-function ) and it's more consistent here without it. ::: browser/base/content/tabbrowser.xml @@ +3406,5 @@ > case "resize": > if (aEvent.target != window) > break; > > + TabsInTitlebar.updateAppearance(); I think I mentioned this before but don't remember exactly what I said: I don't think we actually need to recalculate the margins everytime the window resizes a bit as that will make resizing slower. Can't we bail from the calculations if some attribute such as the sizemode haven't changed since the last _update?
Attachment #700451 - Flags: review?(mnoorenberghe+bmo) → review-
Attachment #700451 - Flags: review?(dao)
Attached patch Patch v3.3 (obsolete) — — Splinter Review
(In reply to Matthew N. [:MattN] from comment #55) > Comment on attachment 700451 [details] [diff] [review] > Patch v3.2 > > Review of attachment 700451 [details] [diff] [review]: > ----------------------------------------------------------------- > > > On Windows 8 with 100% (default) sizes, the tab bar was still shifted in a > restored window when I press alt. Hrm - I think this was caused by the min-height and height hacks I was doing before for the menubar. I've got a potential solution which involves using visibility: collapse instead of the min-height/height values and overflow:hidden in xul.css to hide menubars...this eliminates the problem, but I might be missing a use-case here if we never used visibility: collapse in the first place. I'll needinfo? dao. > > It's late so I'll look more closely at browser.js/.css tomorrow. r- for now > because of resize perf. and tab shifting. > > ::: browser/base/content/browser.js > @@ +4964,5 @@ > > + * The element to measure. > > + * @return > > + * The height as an integer. > > + */ > > + _outerHeight: function _outerHeight (ele) { > > Nit: The need to name the function to show in stack traces has been fixed > (see > http://javascript-reverse.tumblr.com/post/34328529526/javascript-function- > name-inference-aka-stop-function ) and it's more consistent here without it. > Cool - didn't know we'd gotten that. Removed. > ::: browser/base/content/tabbrowser.xml > @@ +3406,5 @@ > > case "resize": > > if (aEvent.target != window) > > break; > > > > + TabsInTitlebar.updateAppearance(); > > I think I mentioned this before but don't remember exactly what I said: > I don't think we actually need to recalculate the margins everytime the > window resizes a bit as that will make resizing slower. Can't we bail from > the calculations if some attribute such as the sizemode haven't changed > since the last _update? You're right - we don't seem to need it on resize; on the sizemodechange event is fine.
Attachment #700451 - Attachment is obsolete: true
Dao: In patch v3.2, MattN noticed a problem on Windows 8 where in restored windows, showing the autohidden menubar via Alt cause the <browser> element to jump up a single pixel. It took a little while, but I think it had to do with the min-height and height override hacking I was doing in that patch. This patch swaps out xul.css's approach to hiding menubars and uses visibility: collapse instead. For what reason did we use min-height/height etc in the first place? Is there a good reason not to use visibility: collapse? The comments in bug 477256 didn't really explain. Thanks, -Mike
Flags: needinfo?(dao)
(In reply to Mike Conley (:mconley) from comment #57) > Dao: > > In patch v3.2, MattN noticed a problem on Windows 8 where in restored > windows, showing the autohidden menubar via Alt cause the <browser> element > to jump up a single pixel. > > It took a little while, but I think it had to do with the min-height and > height override hacking I was doing in that patch. > > This patch swaps out xul.css's approach to hiding menubars and uses > visibility: collapse instead. > > For what reason did we use min-height/height etc in the first place? Is > there a good reason not to use visibility: collapse? The comments in bug > 477256 didn't really explain. See bug 127244.
Flags: needinfo?(dao)
(In reply to Dão Gottwald [:dao] from comment #58) > See bug 127244. Ah, I see. Well - good news; it looks like the problem reported in bug 127244 doesn't re-appear with this patch. Steps I tried: 1) Start a new profile 2) Press F11 to go to full-screen mode 3) Press Alt-f, Esc, Alt-e, Esc, Alt-v, Esc, Alt-s, Esc, Alt-b, Esc, Alt-t, Esc, Alt-h, Esc 4) Press F11 to exit full-screen mode 5) Press Alt-f, Esc, Alt-e, Esc, Alt-v, Esc, Alt-s, Esc, Alt-b, Esc, Alt-t, Esc, Alt-h, Esc All menus appear just fine following these steps. Also, repeating 1-4 and then using Alt and mouse-clicks to access menus works correctly as well.
Note that I only tried those steps on Windows 7 and 8. I'll try XP and Ubuntu next.
Cannot reproduce bug 127244 with this patch on Windows XP or Ubuntu either. Unless I've misunderstood that bug, I think I'm in the clear.
Comment on attachment 705100 [details] [diff] [review] Patch v3.3 The issues you brought up in your last review have been fixed. Thanks!
Attachment #705100 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 705100 [details] [diff] [review] Patch v3.3 Review of attachment 705100 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately I found some issues in my testing. Testing on XP (Olive) with "Extra Large Fonts" I found 3 bugs: 1) The space above #main-menubar increases when going from autohide to always visible. STR: 1) Hide menu bar 2) Press alt to bring up auto-hidden menu bar 3) View > Toolbars > Menu bar to keep the menu bar visible. Notice the menu bar move down. 2) On click of the <menu> when the menubar is autohidden, there is a vertical jump in position. 3) There is too much space on top of the menubar. Bugs found on XP Olive with normal font size: 4) Position of menubar differs depending on whether is calculated while the window is restored or maximized. I tested by toggling the drawInTitlebar pref twice in each sizemode. You will notice that the menubar is too high in some cases. If you think some of these large font cases above are edge-cases then I'd be fine with handling them in follow-ups. ::: browser/base/content/tabbrowser.xml @@ -3419,5 @@ > if (aEvent.target != window) > break; > > - let sizemode = document.documentElement.getAttribute("sizemode"); > - TabsInTitlebar.allowedBy("sizemode", I thought I saw somewhere that there's a possibility we'll do our calculations too early before the window has the correct size. Waiting for the first resize(?) event before calculating was a workaround. I can't remember where I read this. This may help with some of the bugs I found. ::: browser/themes/winstripe/browser.css @@ +34,5 @@ > } > > #main-menubar { > -moz-box-flex: 1; /* make menu items expand to fill toolbar height */ > + background-color: rgba(255,255,255,0.2); Is this background-color necessary without LWT? I much prefer the menubar without it as seen in the mockups in bug 625307 comment 0. @@ +43,5 @@ > +} > + > +#main-window[sizemode="normal"][tabsintitlebar] #toolbar-menubar, > +#main-window[tabsintitlebar] #toolbar-menubar:not([inactive]) { > + margin-bottom: 4px; Is this supposed to be the difference in height between the appmenu button and the menubar? If so, that would be why this breaks with larger fonts since the appmenu button height increases. ::: toolkit/content/xul.css @@ -290,5 @@ > toolbar[type="menubar"][autohide="true"][inactive="true"]:not([customizing="true"]) { > - min-height: 0 !important; > - height: 0 !important; > - -moz-appearance: none !important; > - border-style: none !important; I'll add the addon-compat keyword in case some addons were relying on the !important.
Attachment #705100 - Flags: review?(mnoorenberghe+bmo) → review-
Keywords: addon-compat
Attached patch Patch v3.4 (obsolete) — — Splinter Review
Hey Matt - would you mind giving this a spin? I think I've covered all cases - I've been working with Luna (Olive, Silver, Blue) and Windows 7 (Aero), in normal and large fonts. I haven't been worrying about the classic themes yet. Is this close to what we want?
Attachment #705100 - Attachment is obsolete: true
Attachment #712547 - Flags: ui-review?(mnoorenberghe+bmo)
It would be nice to have this patch pushed to UX branch for testing.
(In reply to Mike Conley (:mconley) from comment #64) > Created attachment 712547 [details] [diff] [review] > Patch v3.4 > > Hey Matt - would you mind giving this a spin? This doesn't apply cleanly on tip or Feb. 9th. Could you either let me know which patches it depends upon (e.g. australis patches) and which changeset it was built on or upload a new patch?
Status: NEW → ASSIGNED
Ignore that, I got it to apply on Feb. 7th with only a small hunk of conflict instead of all of the browser.js changes.
Comment on attachment 712547 [details] [diff] [review] Patch v3.4 Review of attachment 712547 [details] [diff] [review]: ----------------------------------------------------------------- This is working much better now. It's really close visually IMO. I think the menubar background colour on glass should be subtler but I guess that's for another bug since you didn't change that. There is one issue that I found (other than Windows Classic) for Extra Large Fonts (on Olive): 1) Turn on the menubar 2) Maximize the window 3) Restart the browser 4) Hide the menubar Actual result: The app and caption buttons overlap the nav-bar. ::: browser/themes/winstripe/browser.css @@ +40,5 @@ > +#main-window[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive="true"]:not([customizing="true"]) { > + visibility: hidden; > +} > + > +#main-window[sizemode="normal"] #TabsToolbar, The first rule should include [tabsintitlebar] otherwise it creates extra space with tabs on bottom and browser.tabs.drawInTitlebar = false. I don't know if drawInTitlebar = false on Windows is supported though. It seems like it's something that adds extra complexity with little benefit but I see quite a few add-ons that disable it. @@ +41,5 @@ > + visibility: hidden; > +} > + > +#main-window[sizemode="normal"] #TabsToolbar, > +#toolbar-menubar:-moz-any(:not([autohide="true"]), :not([inactive="true"])) ~ #TabsToolbar { Can you add a comment stating what the second rule is doing? @@ +145,5 @@ > } > > %ifndef WINSTRIPE_AERO > @media (-moz-windows-default-theme) { > + #main-window[sizemode="normal"] #toolbar-menubar { [tabsintitlebar] here too? @@ +462,5 @@ > } > > @media (-moz-windows-classic) { > + #main-window[sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container, > + #main-window[sizemode="normal"] > #tab-view-deck > #browser-panel > #navigator-toolbox > #toolbar-menubar { …and here?
Attachment #712547 - Flags: ui-review?(mnoorenberghe+bmo) → ui-review-
Attached patch Patch v3.5 (obsolete) — — Splinter Review
(In reply to Matthew N. [:MattN] from comment #68) > Comment on attachment 712547 [details] [diff] [review] > Patch v3.4 > > Review of attachment 712547 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is working much better now. It's really close visually IMO. > Excellent! Sorry this is taking so long. > I think the menubar background colour on glass should be subtler but I guess > that's for another bug since you didn't change that. > *nods* > There is one issue that I found (other than Windows Classic) for Extra Large > Fonts (on Olive): > 1) Turn on the menubar > 2) Maximize the window > 3) Restart the browser > 4) Hide the menubar > Actual result: The app and caption buttons overlap the nav-bar. > Good catch - I wasn't properly resetting the bottom margin of the menubar before recalculating. Fixed now. > ::: browser/themes/winstripe/browser.css > @@ +40,5 @@ > > +#main-window[sizemode="normal"] #toolbar-menubar[autohide="true"][inactive="true"]:not([customizing="true"]) { > > + visibility: hidden; > > +} > > + > > +#main-window[sizemode="normal"] #TabsToolbar, > > The first rule should include [tabsintitlebar] otherwise it creates extra > space with tabs on bottom and browser.tabs.drawInTitlebar = false. I don't > know if drawInTitlebar = false on Windows is supported though. It seems > like it's something that adds extra complexity with little benefit but I see > quite a few add-ons that disable it. > Ok, done. > @@ +41,5 @@ > > + visibility: hidden; > > +} > > + > > +#main-window[sizemode="normal"] #TabsToolbar, > > +#toolbar-menubar:-moz-any(:not([autohide="true"]), :not([inactive="true"])) ~ #TabsToolbar { > > Can you add a comment stating what the second rule is doing? > Done. > @@ +145,5 @@ > > } > > > > %ifndef WINSTRIPE_AERO > > @media (-moz-windows-default-theme) { > > + #main-window[sizemode="normal"] #toolbar-menubar { > > [tabsintitlebar] here too? > Done. > @@ +462,5 @@ > > } > > > > @media (-moz-windows-classic) { > > + #main-window[sizemode="normal"] > #titlebar > #titlebar-content > #appmenu-button-container, > > + #main-window[sizemode="normal"] > #tab-view-deck > #browser-panel > #navigator-toolbox > #toolbar-menubar { > > …and here? Done.
Attachment #712547 - Attachment is obsolete: true
Attachment #713496 - Flags: ui-review?(mnoorenberghe+bmo)
This latest patch was rebased on the latest UX tip (34d5265eaa3e - Feb 12th, 14:54).
Attached patch Patch v3.6 (obsolete) — — Splinter Review
So, I've cleaned up this patch some, added some documentation where I felt we could use it. I also checked to see how this behaved on Windows Classic theme, and it actually seems to work alright - it stretches the titlebar down the right amount. Works in large and small fonts. The text in the menu is basically unreadable though - but you can say the same thing for the background tabs. I think that's a fix-it-later. So, I think I'm done messing with this until I hear from you. Once I've got your ui-r+ on it, I think I'm ready for review.
Attachment #713496 - Attachment is obsolete: true
Attachment #713496 - Flags: ui-review?(mnoorenberghe+bmo)
Attachment #714532 - Flags: ui-review?(mnoorenberghe+bmo)
Comment on attachment 714532 [details] [diff] [review] Patch v3.6 Review of attachment 714532 [details] [diff] [review]: ----------------------------------------------------------------- I didn't find problems other than the ones you already mentioned. The menubar text color for Classic is the main issue. Hiding and showing the menubar from the menu seems to differ in behaviour now IIRC but this is probably OK since the app button will be gone when this lands. I didn't look closely at the code since interdiff gives an error but I'll do that on the review pass.
Attachment #714532 - Flags: ui-review?(mnoorenberghe+bmo) → ui-review+
Comment on attachment 714532 [details] [diff] [review] Patch v3.6 Ok, I think I'm all set for a review here.
Attachment #714532 - Flags: review?(mnoorenberghe+bmo)
It looks very nice on UX. I spotted some bugs though. -The private window indicator doesn't appear anymore on the right side of the screen. -First tab is a few pixels too much on the right with the menu bar enabled -known bug with both menu button and menu in titlebar : in restored mode the tabs are a lot too much on the right (problem will be solved with new menu panel). -Menu bar in unselected windows isn't very readable. It's also a bit hard to click on the different menu items since the clickable area doesn't go up to the top of the window in maximized mode. Using the Australis hover styling could also be nice for the menu bar.
And I forgot to add that the first letter of all menu item shouldn't be underlined according to the mockup.
Middle mouse click to open new tab, doesnt work properly.
Attached image margin —
If the menu bar is enabled and you use the full screen mode. At the top of window is 4px margin. I do not know if it is related to this bug.
Another little detail : the menubar should cover the window even on the left side (see http://people.mozilla.com/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-MainWindow-%28MenuBar%29.jpg )
Comment on attachment 714532 [details] [diff] [review] Patch v3.6 Review of attachment 714532 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Thanks for the thorough code comments. r- for the pointer-event issue mostly because I don't know how big of a change will be required. ::: browser/base/content/browser.css @@ -117,5 @@ > -moz-binding: url("chrome://global/content/bindings/general.xml#windowdragbox"); > } > > -#titlebar-spacer { > - pointer-events: none; Removing this breaks toolbar buttons on the menubar. Only the bottom few pixels of buttons are clickable because the pointer-events over #titlebar-spacer go to it instead. ::: browser/themes/winstripe/browser.css @@ +44,5 @@ > +#main-window[sizemode="normal"] #TabsToolbar, > +/* We want a 4px gap between the TabsToolbar and the toolbar-menubar when the > + toolbar-menu is displayed. The menu is displayed with autohide is false, or > + when inactive is false. */ > +#toolbar-menubar:-moz-any(:not([autohide="true"]), :not([inactive="true"])) ~ #TabsToolbar { Yay comments! [autohide=false] is more clear than :not([autohide="true']). Did you find a case where the autohide attribute wasn't set but you wanted this selector to match? @@ +1970,1 @@ > color: white; Note to self: deselected tab colour also needs to be set to white on Windows Classic.
Attachment #714532 - Flags: review?(mnoorenberghe+bmo) → review-
Comment on attachment 714532 [details] [diff] [review] Patch v3.6 Could you also address comment 77?
Also, double-click to maximize doesn't work above the tabstrip on XP in a restored window. Replies to some comments below. I'll let Mike investigate the other comments. (In reply to Guillaume C. [:ge3k0s] from comment #74) > -known bug with both menu button and menu in titlebar : in restored mode the > tabs are a lot too much on the right (problem will be solved with new menu > panel). Yes, this can be handled in the bug which removes the app button. (In reply to Guillaume C. [:ge3k0s] from comment #75) > And I forgot to add that the first letter of all menu item shouldn't be > underlined according to the mockup. Please file a new bug (if there isn't one filed already) since it will require some discussion and isn't directly related to this bug. I suspect we would still want to show the underlines in the auto-hide case. (In reply to UK92 from comment #76) > Middle mouse click to open new tab, doesnt work properly. Were you middle-clicking on either the top, left or right of the tabstrip? The spacers under the app and caption buttons currently don't support this but perhaps we should consider it. We should also consider this for the area above the tabs since it's no longer clear where the tabstrip boundary is. Please file a new bug which blocks this one. (In reply to Guillaume C. [:ge3k0s] from comment #79) > Another little detail : the menubar should cover the window even on the left > side This can also be handled in the bug which removes the app button.
Depends on: 852368
(In reply to Matthew N. [:MattN] from comment #82) > Also, double-click to maximize doesn't work above the tabstrip on XP in a > restored window. I'm not sure what was going on because this works now. There is the (4px?) gap between the tabstrip and titlebar area which doesn't act as either of the two but I think it didn't work anywhere above the tabs. I suspect this was a bug related to switching XP themes while Firefox is running and may not be caused by this patch.
Depends on: 852462
(In reply to Matthew N. [:MattN] from comment #82) > Replies to some comments below. I'll let Mike investigate the other comments. > (In reply to Guillaume C. [:ge3k0s] from comment #75) > > And I forgot to add that the first letter of all menu item shouldn't be > > underlined according to the mockup. > > Please file a new bug (if there isn't one filed already) since it will > require some discussion and isn't directly related to this bug. I suspect > we would still want to show the underlines in the auto-hide case. Filed bug 852462. Concerning the private indicator what is the plan ? I know Australis should change it, but for now I think it should appear on the right as previously.
(In reply to Guillaume C. [:ge3k0s] from comment #84) > Filed bug 852462. That was a dupe from bug 25894.
What 3.7 fixes: 1) Puts pointer-events: none; back on the titlebar-spacer 2) Due to #1, I had to do a little bit of work to make it possible to drag the window by the hidden menubar. I do this by introducing a new binding "toolbar-menubar-autohide-draggable" that inherits from "toolbar-menubar-autohide". I also had to set the z-index of both the appmenu button and the window buttons to 1 for all themes so that the draggable menubar wouldn't swallow click and hover events for those items. 3) Also fixed the 4px space when in full-screen mode with menubar enabled 4) Simplified the CSS as per MattN's suggestion (using [autohide=false] as opposed to :not([autohide="true"]), which is indeed much easier to read). Patch v3.6 got updated to use the new theme directories, but that patch never landed in this bug. I extracted the patch as it exists in the UX branch to make these changes. This is the interdiff between the extracted patch and the fixes I made.
Attachment #714532 - Attachment is obsolete: true
Whoops - forgot the new binding.
Attachment #727232 - Attachment is obsolete: true
Attached patch Patch v3.7 (obsolete) — — Splinter Review
(In reply to Mike Conley (:mconley) from comment #86) > 2) Due to #1, I had to do a little bit of work to make it possible to drag > the window by the hidden menubar. The only purpose of temporarily showing the toolbar is to open menus, no? Why should it be a drag handle? > Yay comments! [autohide=false] is more clear than :not([autohide="true']). > Did you find a case where the autohide attribute wasn't set but you wanted > this selector to match? The autohide attribute isn't set by default on XP.
(In reply to Dão Gottwald [:dao] from comment #89) > (In reply to Mike Conley (:mconley) from comment #86) > > 2) Due to #1, I had to do a little bit of work to make it possible to drag > > the window by the hidden menubar. > > The only purpose of temporarily showing the toolbar is to open menus, no? > Why should it be a drag handle? When the menubar is hidden, is still consumes space (as per spec) - it's just got an opacity of 0. When it is in this state, the hidden menubar should be a window drag target. Otherwise, the titlebar is mostly undraggable in the restored mode. > > > Yay comments! [autohide=false] is more clear than :not([autohide="true']). > > Did you find a case where the autohide attribute wasn't set but you wanted > > this selector to match? > > The autohide attribute isn't set by default on XP. Ah, so you're right. http://mxr.mozilla.org/mozilla-central/source/browser/base/content/win6BrowserOverlay.xul#11 adds it for osversion>=6. I'll regenerate the patch with the old rule.
(In reply to Mike Conley (:mconley) from comment #90) > When the menubar is hidden, is still consumes space (as per spec) - it's > just got an opacity of 0. When it is in this state, the hidden menubar > should be a window drag target. Otherwise, the titlebar is mostly > undraggable in the restored mode. Why are you using opacity:0 rather than, say, visibility:hidden?
(In reply to Dão Gottwald [:dao] from comment #91) > (In reply to Mike Conley (:mconley) from comment #90) > > When the menubar is hidden, is still consumes space (as per spec) - it's > > just got an opacity of 0. When it is in this state, the hidden menubar > > should be a window drag target. Otherwise, the titlebar is mostly > > undraggable in the restored mode. > > Why are you using opacity:0 rather than, say, visibility:hidden? With visibility: hidden, the toolbar-menubar does not emit click events.
(In reply to Mike Conley (:mconley) from comment #92) > (In reply to Dão Gottwald [:dao] from comment #91) > > (In reply to Mike Conley (:mconley) from comment #90) > > > When the menubar is hidden, is still consumes space (as per spec) - it's > > > just got an opacity of 0. When it is in this state, the hidden menubar > > > should be a window drag target. Otherwise, the titlebar is mostly > > > undraggable in the restored mode. > > > > Why are you using opacity:0 rather than, say, visibility:hidden? > > With visibility: hidden, the toolbar-menubar does not emit click events. But we don't want it to consume click events. We just want that area to act as a drag handle. So maybe the element that gets exposed when the toolbar is hidden (the toolbox?) should just be a drag handle -- preferably without any XBL.
Putting back the changed CSS rule, since Dao pointed out that autohide is not set on the toolbar-menubar for Windows versions < 6.
Attachment #727233 - Attachment is obsolete: true
(In reply to Dão Gottwald [:dao] from comment #93) > (In reply to Mike Conley (:mconley) from comment #92) > > (In reply to Dão Gottwald [:dao] from comment #91) > > > (In reply to Mike Conley (:mconley) from comment #90) > > > > When the menubar is hidden, is still consumes space (as per spec) - it's > > > > just got an opacity of 0. When it is in this state, the hidden menubar > > > > should be a window drag target. Otherwise, the titlebar is mostly > > > > undraggable in the restored mode. > > > > > > Why are you using opacity:0 rather than, say, visibility:hidden? > > > > With visibility: hidden, the toolbar-menubar does not emit click events. > > But we don't want it to consume click events. We just want that area to act > as a drag handle. So maybe the element that gets exposed when the toolbar is > hidden (the toolbox?) should just be a drag handle -- preferably without any > XBL. Yes, the navigator-toolbox is what is exposed. How can we make the navigator-toolbox a draggable? We can't just attach the windowdragbox binding, since we still need the toolbox to act like a toolbox. We could extend toolbox with a toolbox-draggable - but that'd be using XBL again, and isn't a whole lot different than my current solution. Do you have any recommendations, Dao?
Hey ge3k0s, (In reply to Guillaume C. [:ge3k0s] from comment #74) > It looks very nice on UX. I spotted some bugs though. > Thanks! > -The private window indicator doesn't appear anymore on the right side of > the screen. The private window indicator never appeared on the right side of the screen for Windows - it always appeared on the AppMenu button. > -First tab is a few pixels too much on the right with the menu bar enabled Can you post a screenshot? I'm not seeing this. > -Menu bar in unselected windows isn't very readable. This is true, and I'd like to deal with that in a follow-up bug. Hey fx4waldi, (In reply to fx4waldi from comment #77) > Created attachment 723595 [details] > margin > > If the menu bar is enabled and you use the full screen mode. At the top of > window is 4px margin. > I do not know if it is related to this bug. Good catch, and it was indeed related to this bug. That's been fixed in my latest iteration.
(In reply to Mike Conley (:mconley) from comment #96) > (In reply to Guillaume C. [:ge3k0s] from comment #74) > > -The private window indicator doesn't appear anymore on the right side of > > the screen. > > The private window indicator never appeared on the right side of the screen > for Windows - it always appeared on the AppMenu button. No with the app menu hidden the private icon should appear on the right side : https://bug749394.bugzilla.mozilla.org/attachment.cgi?id=694075 > > -First tab is a few pixels too much on the right with the menu bar enabled > > Can you post a screenshot? I'm not seeing this. It may have been an impression, but the first app tab seems to be a bit too much on the right (compare to this mockup : http://people.mozilla.com/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-MainWindow-%28AppTab%29.jpg )
Depends on: 853083
(In reply to Mike Conley (:mconley) from comment #95) > How can we make the navigator-toolbox a draggable? We can't just attach the > windowdragbox binding, since we still need the toolbox to act like a > toolbox. We could extend toolbox with a toolbox-draggable - but that'd be > using XBL again, and isn't a whole lot different than my current solution. WindowDraggingUtils.jsm can be used like this: http://hg.mozilla.org/mozilla-central/annotate/1d6fe70c79c5/browser/base/content/browser.js#l5118 Presumably this could just be applied unconditionally -- there aren't any theme / OS / sizemode / customization combinations where it would hurt, are there?
Attached patch Patch v3.6 - v3.7 interdiff (obsolete) — — Splinter Review
Attachment #727271 - Attachment is obsolete: true
Comment on attachment 727819 [details] [diff] [review] Patch v3.6 - v3.7 interdiff Ok, thanks Dao. I've switched to using WindowDraggingUtils.jsm directly instead of the XBL. How do you feel about this approach?
Attachment #727819 - Flags: feedback?(dao)
(In reply to Mike Conley (:mconley) from comment #100) > Comment on attachment 727819 [details] [diff] [review] > Patch v3.6 - v3.7 interdiff > > Ok, thanks Dao. I've switched to using WindowDraggingUtils.jsm directly > instead of the XBL. How do you feel about this approach? I like it better.
Comment on attachment 727819 [details] [diff] [review] Patch v3.6 - v3.7 interdiff >- if (!this._draghandle) { >+ if (!this._draghandlers) { >+ this._draghandlers = {}; s/_draghandlers/_draghandles/
Attachment #727819 - Flags: feedback?(dao)
Attached patch Patch v3.7 (obsolete) — — Splinter Review
Thanks Dao. I've updated the aggregated patch with the changes in the interdiff.
Attachment #727235 - Attachment is obsolete: true
Comment on attachment 727879 [details] [diff] [review] Patch v3.7 Ok, I believe I've addressed all of the issues you brought up in your review.
Attachment #727879 - Flags: review?(mnoorenberghe+bmo)
Attached patch Patch v3.6 - v3.7 interdiff (obsolete) — — Splinter Review
During a self-review, noticed that I hadn't switched to _draghandles from _draghandlers as Dao had suggested. Fixed that now.
Attachment #727819 - Attachment is obsolete: true
Attached patch Patch v3.7 (obsolete) — — Splinter Review
Updating the unified diff.
Attachment #727879 - Attachment is obsolete: true
Attachment #727879 - Flags: review?(mnoorenberghe+bmo)
Attachment #728965 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 728965 [details] [diff] [review] Patch v3.7 Review of attachment 728965 [details] [diff] [review]: ----------------------------------------------------------------- I found new bugs :( 1) Buttons on the menubar now have the proper hover area but are not clickable. Luckily that's an old bug (bug 541844) so no need to fix it here. 2) When there is tab overflow and the menubar is hidden, pressing ALT causes the selected tab to scroll to a certain position and causes shifts in the y-coordinate and state/color/opacity of the scrollbox arrows. Doesn't seem to happen on m-c. 3) When the menubar is showing with Extra Large Fonts, there is a lot of unnecessary space between the tabs and menubar. I think it's still taking the height of the app & caption buttons (titlebar-content) into account but now that we don't put tabs under the app/caption buttons that doesn't seem right. I'm wondering if I'm taking a step backwards here since the calculation has been discussed many times and I didn't note this before. This could move to a follow-up although it seems to hint that there are fundamental issues with the calculation. ::: browser/themes/windows/browser-aero.css @@ -334,5 @@ > -moz-appearance: none; > background-color: #556; > } > > - /* Glass Fog */ Leave this section heading here as it's also for the rulesets below the deleted one.
Attachment #728965 - Flags: review?(mnoorenberghe+bmo) → feedback+
Blocks: 574928
No longer blocks: 813808
No longer depends on: 852462
Summary: [Win] Tabs in the titlebar for restored windows → [Win] Tabs and menubar in the titlebar for restored windows
(In reply to Matthew N. [:MattN] from comment #107) > Comment on attachment 728965 [details] [diff] [review] > Patch v3.7 > > Review of attachment 728965 [details] [diff] [review]: > ----------------------------------------------------------------- > > I found new bugs :( > > 1) Buttons on the menubar now have the proper hover area but are not > clickable. Luckily that's an old bug (bug 541844) so no need to fix it here. Ok, I'll leave it. > 2) When there is tab overflow and the menubar is hidden, pressing ALT causes > the selected tab to scroll to a certain position and causes shifts in the > y-coordinate and state/color/opacity of the scrollbox arrows. Doesn't seem > to happen on m-c. I think I've fixed this one, though I'm not entirely certain why it was broken in the first place...instead of putting a margin-top of 4px on the TabsToolbar when showing the toolbar-menubar, I put a margin-bottom on the menubar. This seems to sidestep the problem, anyhow. > 3) When the menubar is showing with Extra Large Fonts, there is a lot of > unnecessary space between the tabs and menubar. I think it's still taking > the height of the app & caption buttons (titlebar-content) into account but > now that we don't put tabs under the app/caption buttons that doesn't seem > right. I'm wondering if I'm taking a step backwards here since the > calculation has been discussed many times and I didn't note this before. > This could move to a follow-up although it seems to hint that there are > fundamental issues with the calculation. > Let's deal with that in a follow-up. > ::: browser/themes/windows/browser-aero.css > @@ -334,5 @@ > > -moz-appearance: none; > > background-color: #556; > > } > > > > - /* Glass Fog */ > > Leave this section heading here as it's also for the rulesets below the > deleted one. Done.
Attachment #728964 - Attachment is obsolete: true
Attachment #728965 - Attachment is obsolete: true
Attached patch Patch v3.8 — — Splinter Review
Attachment #730674 - Flags: review?(mnoorenberghe+bmo)
Attached image screenshot —
1.Open a maximized window (with app button) 2.Open print preview 3.Close print preview 4.Tabs are too much on the left
(In reply to fx4waldi from comment #110) > Created attachment 730934 [details] > screenshot > > 1.Open a maximized window (with app button) > 2.Open print preview > 3.Close print preview > 4.Tabs are too much on the left Thanks fx4waldi. This seems to occur independent from this patch. I've filed bug 856590 for you.
(In reply to Mike Conley (:mconley) from comment #109) > Created attachment 730674 [details] [diff] [review] > Patch v3.8 This patch doesn't seem to apply directly on top of bug 738491. I tried to review this on Friday but the conflicts slowed me down. The interdiff seems to apply on UX so I'll test this once my Windows build of the ux branch finishes in an hour or so.
Comment on attachment 730674 [details] [diff] [review] Patch v3.8 r=me. Please file the follow-up from comment 108.
Attachment #730674 - Flags: review?(mnoorenberghe+bmo) → review+
Flags: needinfo?(shorlander)
Whiteboard: [fixed in ux]
(In reply to Matthew N. [:MattN] from comment #113) > Comment on attachment 730674 [details] [diff] [review] > Patch v3.8 > > r=me. > > Please file the follow-up from comment 108. Thanks - filed bug 857088.
In the latest UX build (04.16.13) the old menn button doesn't hide any more when menubar is shown by default.
(In reply to Guillaume C. [:ge3k0s] from comment #115) > In the latest UX build (04.16.13) the old menn button doesn't hide any more > when menubar is shown by default. That's a known issue but since we won't ship this patch with the old button it's okay (see comment 27). Customization patches will eventually remove the old app button.
Depends on: 862903
Depends on: 863862
Depends on: 863863
Blocks: 850924
Blocks: 873449
No longer blocks: 873449
Depends on: 873449
Depends on: 879581
Depends on: 879590
Depends on: 879592
Depends on: 879598
Depends on: 885062
https://hg.mozilla.org/mozilla-central/rev/9c493366a3da
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in ux]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: