864034 - [Mac][personas] lightweight theming crunches window title bar

Status: RESOLVED FIXED

(SeaMonkey :: Themes, defect)

Description

[Karsten Düsterloh]

Attachment: crunched window title

When using lightweight theming on Mac, the window title bar is crunched into the window content. See attached image for details.

Self built:
User agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:23.0) Gecko/20100101 Firefox/23.0 SeaMonkey/2.20a1
Build identifier: 20130416223447
Mac OS X 10.6.8

Comment 1

[Stefan [:stefanh]]

I see this too... interesting... without digging it looks like the height calculation doesn't include the titlebar height.

Comment 2

[Stefan [:stefanh]]

Doesn't happen in todays firefox nightly. Do you have a regression range?

Comment 3

[Karsten Düsterloh]

> Doesn't happen in todays firefox nightly.

*nod* FF and TB built from the same codebase aren't affected.

> Do you have a regression range?

No(t yet). I *think* my last build before (of 2013-01-14 :-() was okay, though.

Comment 4

[Stefan [:stefanh]]

Hmm, this might be caused by bug 647216. I haven't tested, so I'm guessing here :-)

LightweightThemeConsumer.jsm has this:

110 #ifdef XP_MACOSX
111     if (active)
112       root.setAttribute("drawintitlebar", "true");
113     else
114       root.removeAttribute("drawintitlebar");
115 #endif

And xul.css used to have this:
%ifdef XP_MACOSX
:root[drawintitlebar="true"] {
  padding-top: 22px;
}
%endif

But the above code in xul.css was removed in bug 647216:
https://hg.mozilla.org/mozilla-central/diff/2f54529528a9/toolkit/content/xul.css

Comment 5

[Stefan [:stefanh]]

If you do
+:root[drawintitlebar="true"] {
+  padding-top: 22px;
+}
+
in suite/themes/classic/mac/communicator.css the problem goes away. But that'll cause bug 806244, so we'll need a better approach here...

Comment 6

[Stefan [:stefanh]]

(We probably need a titlebar vbox in our main windows)

Comment 7

[Stefan [:stefanh]]

Attachment: Fix title bar

I hope I got all windows now :-)

Optionally, this should be handled in toolkit/...

This will also be needed in aurora. I wanted to remove the height of the #titlebar height in fullscreen mode with a persona applied (#main-window[drawintitlebar="true][sizemode="fullscreen"] > #titlebar), but there was a delay in the height adjustment, so I didn't thought it looked good enough.

(the sr? is a moa request).

Comment 8

[Stefan [:stefanh]]

(In reply to Stefan [:stefanh] from comment #7)

> This will also be needed in aurora.

And in beta too, since the m-c patch landed before February 19.

Comment 9

[Philip Chee]

Does Addressbook need one too?

Comment 10

[Stefan [:stefanh]]

(In reply to Philip Chee from comment #9)
> Does Addressbook need one too?

Yes, but it's in the patch?

Comment 11

[Ian Neal]

Could it not just go into utilityOverlay.xul?

Comment 12

[Karsten Düsterloh]

Comment on attachment 740113 [details] [diff] [review]
Fix title bar

>+  <hbox id="titlebar"/>

Looking at the way FF/TB use this pattern, this should be a <vbox>. (Just so that if someone ports the CAN_DRAW_IN_TITLEBAR stuff, he doesn't need to worry where and why the hbox came from.)

(In reply to Stefan [:stefanh] from comment #7)
> Optionally, this should be handled in toolkit/...

Is this likely? ;-)

(In reply to Ian Neal from comment #11)
> Could it not just go into utilityOverlay.xul?

How?! (Or maybe I misunderstand your proposal?)

Comment 13

[Stefan [:stefanh]]

> I hope I got all windows now :-)

Turns out that I forgot messenger.xul and downloadmanager.xul...

Comment 14

[Stefan [:stefanh]]

Attachment: Updated patch, r+moa=Mnyromyr

(In reply to Stefan [:stefanh] from comment #13)
> > I hope I got all windows now :-)
> 
> Turns out that I forgot messenger.xul and downloadmanager.xul...

Just putting the patch here... tree looks busted atm.

Comment 15

[Stefan [:stefanh]]

Comment on attachment 741438 [details] [diff] [review]
Updated patch, r+moa=Mnyromyr

Got a memo from Karsten, he is (naturally) OK with the updated patch

Comment 16

[Stefan [:stefanh]]

Comment on attachment 741438 [details] [diff] [review]
Updated patch, r+moa=Mnyromyr

Starting tomorrow, I'll be away until around May 5-6. The tree seems really busted atm, so I'll land this when I get back. But if someone wants to drive this in to the tree when I'm away, feel free to do so (I'm a bit unsure when the next uplift is)

[Approval Request Comment]
Regression caused by: Bug 647216
User impact if declined: Window title bar appears over toolbar in lightweight themes (on Mac)
Testing completed: nope, see above 
Risk to taking this patch: Low
String changes made by this patch: None

Comment 17

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/comm-beta/rev/af57b5466466

Comment 18

[Stefan [:stefanh]]

http://hg.mozilla.org/comm-central/rev/be604ac62ced
http://hg.mozilla.org/releases/comm-aurora/rev/329ad226ac9a