869660 - Pop-up windows on OSX are looking pretty strange

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Attachment: Demonstration of problem

This was noticed using a recent build of UX after bug 865374. We probably shouldn't be drawing in the titlebar for these windows. See screenshot.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: What it should look like

A reference screenshot to work off of.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

This seems to take care of the problem.

Dao, are there other variations I should care about besides chromehidden~="toolbar"?

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 747088 [details] [diff] [review]
Patch v1

>--- a/browser/components/customizableui/content/toolbar.xml	Tue May 07 10:33:08 2013 -0400
>+++ b/browser/components/customizableui/content/toolbar.xml	Wed May 08 15:52:35 2013 -0400

>   <binding id="toolbar">
>+    <resources>
>+      <stylesheet src="chrome://global/skin/toolbar.css"/>
>+    </resources>

What does this have to do with this bug?

>--- a/browser/themes/osx/browser.css	Tue May 07 10:33:08 2013 -0400
>+++ b/browser/themes/osx/browser.css	Wed May 08 15:52:35 2013 -0400

>+#main-window[chromehidden~="toolbar"] > #titlebar {
>+  height: 22px;
>+}
>+
>+#main-window:not([chromehidden~="toolbar"]) > #titlebar {
>   padding-top: 9px;
> }

Can't you use padding-top in both cases?

> Dao, are there other variations I should care about besides
> chromehidden~="toolbar"?

No, this should be sufficient as of bug 855370.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 747088 [details] [diff] [review]
> Patch v1
> 
> >--- a/browser/components/customizableui/content/toolbar.xml	Tue May 07 10:33:08 2013 -0400
> >+++ b/browser/components/customizableui/content/toolbar.xml	Wed May 08 15:52:35 2013 -0400
> 
> >   <binding id="toolbar">
> >+    <resources>
> >+      <stylesheet src="chrome://global/skin/toolbar.css"/>
> >+    </resources>
> 
> What does this have to do with this bug?
> 

We need this rule: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/toolbar.css#7

Otherwise the popup "toolbar" that contains the urlbar has a white background. Did you have an alternative suggestion?

> >--- a/browser/themes/osx/browser.css	Tue May 07 10:33:08 2013 -0400
> >+++ b/browser/themes/osx/browser.css	Wed May 08 15:52:35 2013 -0400
> 
> >+#main-window[chromehidden~="toolbar"] > #titlebar {
> >+  height: 22px;
> >+}
> >+
> >+#main-window:not([chromehidden~="toolbar"]) > #titlebar {
> >   padding-top: 9px;
> > }
> 
> Can't you use padding-top in both cases?
> 

Sure, can do.

> > Dao, are there other variations I should care about besides
> > chromehidden~="toolbar"?
> 
> No, this should be sufficient as of bug 855370.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1

Switched from height to padding.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 747429 [details] [diff] [review]
Patch v1.1

>-#titlebar {
>+#main-window[chromehidden~="toolbar"] > #titlebar {
>+  padding-top: 22px;
>+}
>+
>+#main-window:not([chromehidden~="toolbar"]) > #titlebar {
>   padding-top: 9px;
> }

write it like this instead:

#titlebar {
  padding-top: 9px;
}

#main-window[chromehidden~="toolbar"] > #titlebar {
  padding-top: 22px;
}

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2 (r+'d by dao)

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 747429 [details] [diff] [review]
> Patch v1.1
> 
> >-#titlebar {
> >+#main-window[chromehidden~="toolbar"] > #titlebar {
> >+  padding-top: 22px;
> >+}
> >+
> >+#main-window:not([chromehidden~="toolbar"]) > #titlebar {
> >   padding-top: 9px;
> > }
> 
> write it like this instead:
> 
> #titlebar {
>   padding-top: 9px;
> }
> 
> #main-window[chromehidden~="toolbar"] > #titlebar {
>   padding-top: 22px;
> }

Ah, yes, that's much better / clearer. Thank you!

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Landed in UX as https://hg.mozilla.org/projects/ux/rev/81e58dc6d039

Comment 10

[Chenxia Liu [:liuche]]

Has this fix started being built in the nightlies? I'm seeing broken popup titlebar behavior in the most recent (2013/7/8) build of the UX branch.

Comment 11

[Matthew N. [:MattN]]

(In reply to Chenxia Liu [:liuche] from comment #10)
> Has this fix started being built in the nightlies? I'm seeing broken popup
> titlebar behavior in the most recent (2013/7/8) build of the UX branch.

Yes, it should have been there two months ago. I suppose you should re-open bug 888022 then.

Comment 12

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/81e58dc6d039