Last Comment Bug 864034 - [Mac][personas] lightweight theming crunches window title bar
: [Mac][personas] lightweight theming crunches window title bar
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: Themes (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: seamonkey2.20
Assigned To: Stefan [:stefanh]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-20 09:19 PDT by Karsten Düsterloh
Modified: 2013-05-05 05:16 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
crunched window title (75.90 KB, image/png)
2013-04-20 09:19 PDT, Karsten Düsterloh
no flags Details
Fix title bar (5.78 KB, patch)
2013-04-21 13:07 PDT, Stefan [:stefanh]
mnyromyr: review+
mnyromyr: superreview+
Details | Diff | Splinter Review
Updated patch, r+moa=Mnyromyr (7.61 KB, patch)
2013-04-24 11:11 PDT, Stefan [:stefanh]
iann_bugzilla: approval‑comm‑aurora+
iann_bugzilla: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Karsten Düsterloh 2013-04-20 09:19:12 PDT
Created attachment 739980 [details]
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] 2013-04-20 09:33:19 PDT
I see this too... interesting... without digging it looks like the height calculation doesn't include the titlebar height.
Comment 2 Stefan [:stefanh] 2013-04-20 09:50:21 PDT
Doesn't happen in todays firefox nightly. Do you have a regression range?
Comment 3 Karsten Düsterloh 2013-04-20 10:33:12 PDT
> 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] 2013-04-20 11:26:54 PDT
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] 2013-04-20 12:42:40 PDT
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] 2013-04-20 12:50:25 PDT
(We probably need a titlebar vbox in our main windows)
Comment 7 Stefan [:stefanh] 2013-04-21 13:07:57 PDT
Created attachment 740113 [details] [diff] [review]
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] 2013-04-21 13:31:56 PDT
(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 2013-04-22 03:30:17 PDT
Does Addressbook need one too?
Comment 10 Stefan [:stefanh] 2013-04-22 09:58:01 PDT
(In reply to Philip Chee from comment #9)
> Does Addressbook need one too?

Yes, but it's in the patch?
Comment 11 Ian Neal 2013-04-23 13:29:07 PDT
Could it not just go into utilityOverlay.xul?
Comment 12 Karsten Düsterloh 2013-04-23 14:37:13 PDT
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] 2013-04-24 10:21:00 PDT
> I hope I got all windows now :-)

Turns out that I forgot messenger.xul and downloadmanager.xul...
Comment 14 Stefan [:stefanh] 2013-04-24 11:11:14 PDT
Created attachment 741438 [details] [diff] [review]
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] 2013-04-25 11:28:20 PDT
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] 2013-04-26 12:27:19 PDT
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) 2013-05-02 19:14:40 PDT
https://hg.mozilla.org/releases/comm-beta/rev/af57b5466466

Note You need to log in before you can comment on or make changes to this bug.