The default bug view has changed. See this FAQ.

Make customizable menubars work without regressing OSX

RESOLVED FIXED in seamonkey2.0a3

Status

SeaMonkey
UI Design
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Philip Chee, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.0a3
All
Mac OS X
Dependency tree / graph
Bug Flags:
blocking-seamonkey2.0a3 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
+++ 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.
(Assignee)

Updated

8 years ago
Blocks: 406780
(Assignee)

Comment 1

8 years ago
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

8 years ago
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.
(Assignee)

Comment 3

8 years ago
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.
(Assignee)

Comment 4

8 years ago
If you set display: none locally in your build does this cause any regressions or peculiar behaviour in the Mac system menubar?
(Assignee)

Comment 5

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

Comment 6

8 years ago
(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.
(Assignee)

Comment 7

8 years ago
Created attachment 361987 [details] [diff] [review]
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.
Attachment #361987 - Flags: review?(stefanh)
(Assignee)

Comment 8

8 years ago
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)

Comment 9

8 years ago
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?
(Assignee)

Comment 11

8 years ago
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

8 years ago
(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.
(Assignee)

Comment 13

8 years ago
> 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?
(Assignee)

Comment 14

8 years ago
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

8 years ago
(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

8 years ago
(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

8 years ago
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 ;)
(Assignee)

Comment 18

8 years ago
> 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

8 years ago
(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

8 years ago
(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.
(Assignee)

Comment 23

8 years ago
Created attachment 362382 [details] [diff] [review]
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.
Attachment #362382 - Flags: superreview?(neil)
Attachment #362382 - Flags: review?(stefanh)

Comment 24

8 years ago
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.
(Assignee)

Updated

8 years ago
Attachment #362382 - Flags: superreview?(neil)
Attachment #362382 - Flags: review?(stefanh)

Comment 25

8 years ago
(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

8 years ago
(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...
(Assignee)

Comment 28

8 years ago
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?
(Assignee)

Comment 29

8 years ago
> 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
(Assignee)

Comment 31

8 years ago
> 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
(Assignee)

Comment 32

8 years ago
Created attachment 362473 [details] [diff] [review]
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:
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)

Updated

8 years ago
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 34

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

r=me with Neil's comments addressed.
Attachment #362473 - Flags: review?(stefanh) → review+
(Assignee)

Comment 35

8 years ago
Created attachment 362519 [details] [diff] [review]
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.
Attachment #362473 - Attachment is obsolete: true
Attachment #362519 - Flags: superreview+
Attachment #362519 - Flags: review+
Attachment #362519 - Flags: approval-seamonkey2.0a3?
(Assignee)

Updated

8 years ago
Flags: blocking-seamonkey2.0a3?

Comment 36

8 years ago
Not blocking an alpha release on features ;-)
Flags: blocking-seamonkey2.0a3? → blocking-seamonkey2.0a3-

Updated

8 years ago
Attachment #362519 - Flags: approval-seamonkey2.0a3? → approval-seamonkey2.0a3+
(Assignee)

Updated

8 years ago
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
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0a3
(Assignee)

Comment 38

8 years ago
> Philip, could you check how you create your patches ?

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

Updated

8 years ago
Blocks: 479559
(Assignee)

Updated

8 years ago
No longer blocks: 479559
You need to log in before you can comment on or make changes to this bug.