475920 - Make customizable menubars work without regressing OSX

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Philip Chee]

+++ This bug was initially created as a clone of Bug #471372 +++

(From Bug 471372 Comment 29)
> To clear up any misunderstanding: to avoid the broken Mac "menubar", I meant to
> suggest that the menubar changes could be backed out for now.

Comment 1

[Philip Chee]

Suggestions:

1. If I can preprocess content/communicator.css I can have an OSX specific rule to hide the grippy for toolbar[type="menubar"].

2. I can modify (or extend) the grippy-toolbar XBL and make the constructor hide the grippy if the platform is Mac.

3. Please suggest better solutions!

Comment 2

[Stefan [:stefanh]]

Hmm, I wonder if there's a reason for
224 %ifdef XP_MACOSX
225 toolbar[type="menubar"] {
226   min-height: 0 !important;
227   border: 0 !important;
228 }
229 %endif

instead of "display: none;" in toolkit's xul.css.

Comment 3

[Philip Chee]

I tried to do some CVS archaeology but ended up in some massive firefox/toolkit reorganization with minimal documentation and no reviewed patch. In that era it seems people just checked in large swathes of code without any formal review nor approval process.

Comment 4

[Philip Chee]

If you set display: none locally in your build does this cause any regressions or peculiar behaviour in the Mac system menubar?

Comment 5

[Philip Chee]

stefan: could you try display: collapsed; in the ifdef section in XUL.css?

Comment 6

[Stefan [:stefanh]]

(In reply to comment #5)
> stefan: could you try display: collapsed; in the ifdef section in XUL.css?

Unfortunately that doesn't make the collapsed grippy disappear. As a side note, I've know realized that "display: none;" is not so optional either. "display: none;"  will not affect the native menubar, but it will make it hard to play with the navigator menus in DOMi's browser view (since they're hidden). I'd say that the best way to fix this is probably to not have any grippies in a 'toolbar type="menubar"' on mac.

Comment 7

[Philip Chee]

Attachment: Patch v0.1

This patch:
1. Turns back on customizable menubars.
2. Adds a platform specific file platformXUL.css that makes the toolbar[type="menubar"] use the non-grippy toolkit binding.

Comment 8

[Philip Chee]

Comment on attachment 361987 [details] [diff] [review]
Patch v0.1

Err ooops. s/Copyright (C) 2006/Copyright (C) 2009/
I'll fix this in a new patch once the reviews are in.

Comment 9

[Stefan [:stefanh]]

Is it worth ifdefing suite/common/communicator.css instead?

Or would it make sense to have

toolbar:not([type="menubar"]) { ...... toolbar.xml#grippytoolbar"... }

in communicator.css and then use a win/nix version of platformXUL.css that adds the grippy to menubar toolbars?

These options require adding 'nowindowdrag="true"' to the toolbar, otherwise the binding in classic/communicator/mac/toolbar.css will be attached. It's no big deal, but in for example mac classic it looks like this:

1. toolbar.xml#toolbar from xul.css
2. toolbar.xml#grippytoolbar from communicator.css
3. toolbar.xml#toolbar-drag from pinstripe's global.css
4. toolbar.xml#grippytoolbar-drag from mac classic's toolbar.css
5. toolbar.xml#toolbar from platformXUL.css

Now, if you used one of the options above, you'd only have one rule (the first). Thoughts?

Comment 10

[[e-mail]]

(In reply to comment #9)
> 4. toolbar.xml#grippytoolbar-drag from mac classic's toolbar.css
Remember that this needs to work for the Modern theme as well, which is not platform-specific. Rather than trying to set yet another binding, would it work just as well to hide the grippy on the menutoolbar?

Comment 11

[Philip Chee]

This is on WinXP via the DOM inspector. I have no idea whether this works on OSX. I tried:

toolbar[type="menubar"] > .toolbar-box > .toolbar-grippy {
  visibility: collapse;
}

but then I had to add these:

toolbar[type="menubar"] {
  min-height: 0px  !important;
}

toolbar[type="menubar"] > .toolbar-box > .toolbar-holder {
  border-width: 0px !important;
}

On the other hand I could just use one rule like:

toolbar[type="menubar"] {
  visibility: collapse;
}

without having to worry about the visibility of various descendant elements.

Comments?

Comment 12

[Stefan [:stefanh]]

(In reply to comment #10)
> (In reply to comment #9)
> > 4. toolbar.xml#grippytoolbar-drag from mac classic's toolbar.css
> Remember that this needs to work for the Modern theme as well, which is not
> platform-specific.

It should work in Modern if you set the nowindowdrag attribute to true for the menubar toolbars.

Comment 13

[Philip Chee]

> It should work in Modern if you set the nowindowdrag attribute to true for the
> menubar toolbars.

Err. I don't understand why it should work in Modern. Could you explain?

Comment 14

[Philip Chee]

Stefan: instead of the binding in platformXUL.css can you try:

toolbar[type="menubar"] {
  visibility: collapse;
}

And let me know if this works without causing any regressions?

Comment 15

[Stefan [:stefanh]]

(In reply to comment #13)
> > It should work in Modern if you set the nowindowdrag attribute to true for the
> > menubar toolbars.
> 
> Err. I don't understand why it should work in Modern. Could you explain?

I was a bit unclear, it doesn't work in Modern in the sense that the binding won't be attached. But we don't want it anyway. toolbar.xml#grippytoolbar-drag from mac classic's toolbar.css just overrides the toolbar.xml#toolbar-drag binding in pinstripe/global/global.css. Both those style rules only applies to toolbars that doesn't have 'nowindowdrag="true"' set.

Comment 16

[Stefan [:stefanh]]

(In reply to comment #14)
> Stefan: instead of the binding in platformXUL.css can you try:
> 
> toolbar[type="menubar"] {
>   visibility: collapse;
> }
> 
> And let me know if this works without causing any regressions?

That will have the same effect as display: none;

Comment 17

[Stefan [:stefanh]]

Actually, we don't need the nowindowdrag attribute. We could just do like this in mac classic's communicator.css: toolbar:not([type="menubar"]):not([nowindowdrag="true"]) {
moz-binding: url("chrome://communicator/content/bindings/toolbar.xml#grippytoolbar-drag");
}

We don't care about the toolbar.xml#toolbar-drag in pinstripe/global/global.css, since that just makes the window draggable from the menubar toolbar, but the toolbar isn't visible anyway ;)

Comment 18

[Philip Chee]

> That will have the same effect as display: none;

Neil seems to prefer |visibility: collapse;| whenever I use |display: none;|

> We could just do like this in mac classic's communicator.css:

What about Modern? And what ever the solution it has to work with third party themes as well

Comment 19

[Stefan [:stefanh]]

(In reply to comment #18)

> What about Modern? And what ever the solution it has to work with third party
> themes as well

It should work fine in Modern if you do like this in content/communicator.css:

toolbar:not([type="menubar"]) { ...... toolbar.xml#grippytoolbar"... }

Then, there are 2 alternatives:

1) Preprocess content/communicator.css:
#ifndef XP_MAC_OSX
toolbar[type="menubar"] { ...... toolbar.xml#grippytoolbar"... }
#endif

2) Have the same style rule in the ifndef in a win/nix file (platformXUL.css). That would basically be the same as your original patch, but instead of adding the grippytoolbarbinding on mac and remove it later in a separate css file, you'll just add the binding to the platforms that wants it.

As a said, it's no big deal, but it seems better to just add wanted stuff, not add unwanted stuff and then remove it in a separate file.

Comment 20

[Stefan [:stefanh]]

(In reply to comment #18)
> > That will have the same effect as display: none;
> 
> Neil seems to prefer |visibility: collapse;| whenever I use |display: none;|

In this case it will hide the menubar. That's fine as long as you don't want to play with the menubar menus in DOMi (View --> Browser). I also think that this would regress content menubars.

Comment 21

[[e-mail]]

(In reply to comment #18)
> > That will have the same effect as display: none;
> Neil seems to prefer |visibility: collapse;| whenever I use |display: none;|
That depends on the use case. Sometimes visibility is better than display.

(In reply to comment #19)
> 2) Have the same style rule in the ifndef in a win/nix file (platformXUL.css).
May I point out that it's Mac that's the oddball here...

The fix:
* Should not affect win/nix at all. Their toolbars are always grippytoolbars.
* Should work with the Modern theme, whose toolbars aren't draggable.
* Should work with the Classic theme, whose toolbars are draggable.

Comment 22

[[e-mail]]

There may be a generic way around this in XBL;

<binding id="grippytoolbar-menubar" xbl:inherits="toolbar.xml#grippytoolbar" display="xul:menubar"/> might hide the toolbar in Mac chrome only.

Comment 23

[Philip Chee]

Attachment: Patch v0.2 Minimalist fix

Different approach: No new files. Instead I use an inline stylesheet in mac/platformCommunicatorOverlay.xul e.g.

+<?xml-stylesheet href='data:text/css,
+toolbar[type="menubar"] {
+  display: none;
+}' type="text/css"?>
+

I don't seem to be able to use a CDATA section here but so far no XML errors.

Comment 24

[Stefan [:stefanh]]

My point with saying "That will have the same effect as display: none;" in comment #16 was to inform you that "visibility: collapse;" on the toolbar doesn't work because "display: none;" doesn't work which I said in comment #6.

Comment 25

[Stefan [:stefanh]]

(In reply to comment #22)
> There may be a generic way around this in XBL;
> 
> <binding id="grippytoolbar-menubar" xbl:inherits="toolbar.xml#grippytoolbar"
> display="xul:menubar"/> might hide the toolbar in Mac chrome only.

This seems to work. I used it in toolbar.xml, then in content/communicator.css:

toolbar[type="menubar"] {
  -moz-binding: url("chrome://communicator/content/bindings/toolbar.xml#grippytoolbar-menubar");
}

You'll need to make the toolbar.xml#grippytoolbar-drag not being attached on menubar toolbars in mac classic, otherwise that will overrule the grippytoolbar-menubar binding. In other words, you'll need to do toolbar:not([nowindowdrag="true"]):not([type="menubar"]) {
  -moz-binding: url("chrome://communicator/content/bindings/toolbar.xml#grippytoolbar-drag");
} in classic/communicator/mac/toolbar.css

Comment 26

[Stefan [:stefanh]]

(In reply to comment #25)
> (In reply to comment #22)
> > There may be a generic way around this in XBL;
> > 
> > <binding id="grippytoolbar-menubar" xbl:inherits="toolbar.xml#grippytoolbar"
> > display="xul:menubar"/> might hide the toolbar in Mac chrome only.
> 

OK, so the xbl:inherits doesn't seem to work - I've noticed when navigator.xul is displayed in DOMi, there won't be any menubar grippy. If you use extends="chrome://communicator/content/bindings/toolbar.xml#grippytoolbar" it'll work, though.

Comment 27

[[e-mail]]

(In reply to comment #26)
> extends="chrome://communicator/content/bindings/toolbar.xml#grippytoolbar"
Sorry, that's what I meant to say... it was getting late...

Comment 28

[Philip Chee]

For some reason this makes the menubar disappear on WindowsXP, in both Classic and Modern, and in both OS-WindowsClassic and OS-WindowsXP

<binding id="grippytoolbar-menubar"
         extends="chrome://global/content/bindings/toolbar.xml#grippytoolbar"
         display="xul:menubar"/>

Any other suggestions?

Comment 29

[Philip Chee]

> in mac classic's communicator.css:

> toolbar:not([type="menubar"]):not([nowindowdrag="true"]) {
> moz-binding:
> url("chrome://communicator/content/bindings/toolbar.xml#grippytoolbar-drag");
> }

Questions:
1. Will third party themes have to do this as well?
2. Should behavioural CSS really go in /skin/? rather than /content/

Comment 30

[[e-mail]]

(In reply to comment #28)
> For some reason this makes the menubar disappear on WindowsXP, in both Classic
> and Modern, and in both OS-WindowsClassic and OS-WindowsXP
> 
> <binding id="grippytoolbar-menubar"
>          extends="chrome://global/content/bindings/toolbar.xml#grippytoolbar"
>          display="xul:menubar"/>
> 
> Any other suggestions?
Should be communicator, not global, no?

(In reply to comment #29)
> Questions:
> 1. Will third party themes have to do this as well?
> 2. Should behavioural CSS really go in /skin/? rather than /content/
1. Only if they want to blend in with the OS like Mac Classic does
2. Yes, because it's part of the "blend in with the OS" appearance

Comment 31

[Philip Chee]

> Should be communicator, not global, no?
I put this in content/communicator.css:

toolbar[type="menubar"] {
  -moz-binding:
url("chrome://communicator/content/bindings/toolbar.xml#grippytoolbar-menubar");
}

and the binding in:
content/bindings/toolbar.xml

Comment 32

[Philip Chee]

Attachment: Patch v0.3

After fixing my typos, this should work (not tested on OSX so stefanh needs to retest this patch).

Changes:
1. Use new binding for toolbar[type="menubar"] via content/communicator.css.
2. In classic/communicator/mac/toolbar.css added selector so as not to apply the grippytoolbar-drag binding to menubar-toolbars:

Comment 33

[[e-mail]]

Comment on attachment 362473 [details] [diff] [review]
Patch v0.3

>-  
>+
Nit: lots of irrelevant whitespace changes creeping in...

>+  <binding id="grippytoolbar-menubar"
>+           extends="chrome://communicator/content/bindings/toolbar.xml#grippytoolbar"
>+           display="xul:menubar"/>
Would you mind putting this after the grippytoolbar-drag binding?

>+toolbar[type="menubar"] {
>+  -moz-binding: url("chrome://communicator/content/bindings/toolbar.xml#grippytoolbar-menubar");
>+}
And this after the toolbar CSS.

There is a slight problem with this. Because the rule for toolbar[xpfe="false"] appears before the rule for toolbar[type="menubar"], it will lose out when both attributes are specified. You could move all these rules so that they appear before the [xpfe="false"] rules, or you could add an extra override to ensure that a toolbar with both those attributes uses the toolkit binding too.

Comment 34

[Stefan [:stefanh]]

Comment on attachment 362473 [details] [diff] [review]
Patch v0.3

r=me with Neil's comments addressed.

Comment 35

[Philip Chee]

Attachment: Patch v1.0 Final [for checkin] [Checkin: Comment 37]

Carrying forward r+/sr+

> Nit: lots of irrelevant whitespace changes creeping in...
Fixed.

>>+           display="xul:menubar"/>
>> Would you mind putting this after the grippytoolbar-drag binding?
Fixed.

>>+toolbar[type="menubar"] {
> And this after the toolbar CSS.
Fixed.

> or you could add an extra override to ensure
> that a toolbar with both those attributes uses the toolkit binding too.
Fixed.

Comment 36

[Robert Kaiser]

Not blocking an alpha release on features ;-)

Comment 37

[Serge Gautherie (:sgautherie)]

Comment on attachment 362519 [details] [diff] [review]
Patch v1.0 Final [for checkin]
[Checkin: Comment 37]


http://hg.mozilla.org/comm-central/rev/6f3e2d30201f

***

Philip, could you check how you create your patches ?
They all contain some
"  searching for copies back to rev nnnn"
instead of nothing (or the expected comment).

Comment 38

[Philip Chee]

> Philip, could you check how you create your patches ?

I use hg qdiff > patch.diff. I could use hg export instead I suppose.