Closed Bug 475920 Opened 11 years ago Closed 11 years ago

Make customizable menubars work without regressing OSX

Categories

(SeaMonkey :: UI Design, enhancement)

All
macOS
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a3

People

(Reporter: philip.chee, Assigned: philip.chee)

References

Details

Attachments

(1 file, 3 obsolete files)

+++ 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.
Blocks: 406780
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!
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.
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.
If you set display: none locally in your build does this cause any regressions or peculiar behaviour in the Mac system menubar?
stefan: could you try display: collapsed; in the ifdef section in XUL.css?
(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.
Attached patch Patch v0.1 (obsolete) — Splinter Review
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.
Attachment #361987 - Flags: review?(stefanh)
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.
Attachment #361987 - Flags: superreview?(neil)
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?
(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?
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?
(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.
> 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?
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?
(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.
(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;
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 ;)
> 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
(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.
(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.
(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.
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.
Attached patch Patch v0.2 Minimalist fix (obsolete) — Splinter Review
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.
Attachment #362382 - Flags: superreview?(neil)
Attachment #362382 - Flags: review?(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.
Attachment #362382 - Flags: superreview?(neil)
Attachment #362382 - Flags: review?(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
(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.
(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...
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?
> 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/
(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
> 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
Attached patch Patch v0.3 (obsolete) — Splinter Review
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:
Attachment #361987 - Attachment is obsolete: true
Attachment #362382 - Attachment is obsolete: true
Attachment #362473 - Flags: superreview?(neil)
Attachment #362473 - Flags: review?(stefanh)
Attachment #361987 - Flags: superreview?(neil)
Attachment #361987 - Flags: review?(stefanh)
Attachment #362473 - Flags: superreview?(neil) → superreview+
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 on attachment 362473 [details] [diff] [review]
Patch v0.3

r=me with Neil's comments addressed.
Attachment #362473 - Flags: review?(stefanh) → review+
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.
Attachment #362473 - Attachment is obsolete: true
Attachment #362519 - Flags: superreview+
Attachment #362519 - Flags: review+
Attachment #362519 - Flags: approval-seamonkey2.0a3?
Flags: blocking-seamonkey2.0a3?
Not blocking an alpha release on features ;-)
Flags: blocking-seamonkey2.0a3? → blocking-seamonkey2.0a3-
Attachment #362519 - Flags: approval-seamonkey2.0a3? → approval-seamonkey2.0a3+
Keywords: checkin-needed
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).
Attachment #362519 - Attachment description: Patch v1.0 Final [for checkin] → Patch v1.0 Final [for checkin] [Checkin: Comment 37]
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0a3
> Philip, could you check how you create your patches ?

I use hg qdiff > patch.diff. I could use hg export instead I suppose.
Blocks: 479559
No longer blocks: 479559
You need to log in before you can comment on or make changes to this bug.