Closed
Bug 301105
Opened 20 years ago
Closed 19 years ago
[Mac] Classic: Use native-styled menus/menupopups
Categories
(SeaMonkey :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey1.1alpha
People
(Reporter: stefanh, Assigned: stefanh)
References
Details
(Keywords: fixed-seamonkey1.1a, relnote)
Attachments
(13 files, 11 obsolete files)
3.29 KB,
application/vnd.mozilla.xul+xml
|
Details | |
847 bytes,
image/gif
|
Details | |
854 bytes,
image/gif
|
Details | |
847 bytes,
image/gif
|
Details | |
850 bytes,
image/gif
|
Details | |
19.48 KB,
image/png
|
Details | |
36.35 KB,
image/gif
|
Details | |
527 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
8.74 KB,
image/gif
|
Details | |
15.40 KB,
patch
|
stefanh
:
review+
neil
:
superreview+
csthomas
:
approval-seamonkey1.1a+
|
Details | Diff | Splinter Review |
847 bytes,
image/gif
|
neil
:
review+
neil
:
superreview+
|
Details |
23.79 KB,
image/gif
|
Details | |
692 bytes,
patch
|
asaf
:
review+
neil
:
superreview+
csthomas
:
approval-seamonkey1.1a+
|
Details | Diff | Splinter Review |
Since Mano now have implemented NS_THEME_MENU_POPUP and NS_THEME_MENUITEM in bug
118025 we can now use -moz-appearance: menuitem, menupopup in order to make our
xul menus/menupopups more in line with the native ones.
Assignee | ||
Updated•20 years ago
|
Summary: Classic: Use native-styled menus/menupopups, line up items in urlbar popups → Classic: Use native-styled menus/menupopups
Assignee | ||
Updated•20 years ago
|
Whiteboard: ETA: SeaMonkey 1.0b
Assignee | ||
Comment 1•20 years ago
|
||
*** Bug 306284 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 2•20 years ago
|
||
At http://hem.fyristorg.com/s_her/050903/classic.jar you'll find a classic.jar
package at that can be used for testing, comments are welcome. Just re-name it
to classic.jar, navigate to the chrome folder and replace the existing
classic.jar. Should work fine on a nightly trunk build (files are taken from the
050903 build). Note that the files are going to be cleaned up before i post any
patches here.
Apart from switching to native-styled menus (colors etc) icons and text now line
up in menus. I also replaced "radio"-styled checkmarks (dots) with normal
checkmarks and the scrollbox now uses the same arrow as in the native menus
(ripped the arrow from pinstripe).
to-do:
1) Fix menuitems in bookmark menus, so the hover/select color extend all the way
to the right. Currently, there's a white gap at the right.
2) Get new arrows for the sub-menus (right-arrows) - I'll just flip the
scrollbox arrow 90°.
3) Clean up the files
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•20 years ago
|
||
> Just re-name it to classic.jar
Err, no need to rename it, of course...
Assignee | ||
Comment 4•20 years ago
|
||
Just attaching a testcase to demonstrate how xul menus will look like. Atm, the
only issue is that the switch to blue background-color is a bit slow when you
select a menu.
Assignee | ||
Comment 5•20 years ago
|
||
> Just attaching a testcase to demonstrate how xul menus will look like. Atm, the
> only issue is that the switch to blue background-color is a bit slow when you
> select a menu.
Note that Firefox' Pinstripe theme doesn't seem to handle this correct. Hmm, it
seems that the scrollbox-arrow (down-arrow in menus) that I use is Pinstripe's
right-arrow (flipped 90°).
Comment 6•20 years ago
|
||
Alright, I gave this a shot today and just have a few comments/questions. Before
the negative...... It's a great improvement. It integrates with OS X much better
than SM used to. *Thank you* Now... onto the questions.
For the record, my UA is:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050828
SeaMonkey/1.1a
1) First thing I noticed was that the lack of highlight on the location bar.
When typing in a URL and pressing the down arrow, the suggestions (assuming
you've been to enough websites) should be highlight indicating they're being
selected. Right now we just bold the description text (but what if there isn't
any?). Keep in mind, this is only for auto-complete. Clicking the actual down
arrow on the right allows you to select options with the light blue, standard
highlight color. Which brings me to the next question...
2) The selection color in the location bar when clicking the down arrow is light
blue. I think this should be replaced with the dark blue "standard" highlight
color used in the menus.
3) On submenus... When opening a submenu from SM, we're a few pixels low from
where we should be (I think). If you'll notice the application menu's submenus,
the highlight from the submenu and the top item match up perfectly (pixel
perfect even). However, our menus are about 3 or 4 pixels too low from matching
up. Can this be fixed? (Note: there is one exception to this when the submenu is
too close to the top of the screen, or in our case menubar, the highlights don't
quite match up. They're about 6 or 7 pixels low.)
Anyway, besides what you mentioned, that's all I've noticed so far (Is there a
bug for improving the look of tabs on Mac?). Great work. :)
Assignee | ||
Comment 7•20 years ago
|
||
(In reply to comment #6)
Simon, thanks for the feedback :)
> 1) First thing I noticed was that the lack of highlight on the location bar.
> When typing in a URL and pressing the down arrow, the suggestions (assuming
> you've been to enough websites) should be highlight indicating they're being
> selected. Right now we just bold the description text (but what if there isn't
> any?).
Ah, yeah. Forgot that, will fix it.
> 2) The selection color in the location bar when clicking the down arrow is light
> blue. I think this should be replaced with the dark blue "standard" highlight
> color used in the menus.
That might be difficult, unfortunately. The only way to get the blue color
(correctly) is to give the item a complete menu-look (same background-color as
the menus will be applied).
> 3) On submenus... When opening a submenu from SM, we're a few pixels low from
> where we should be (I think). If you'll notice the application menu's submenus,
> the highlight from the submenu and the top item match up perfectly (pixel
> perfect even). However, our menus are about 3 or 4 pixels too low from matching
> up. Can this be fixed?
Hmm, I have to have a further look at this...
Assignee | ||
Comment 8•20 years ago
|
||
Assignee | ||
Comment 9•20 years ago
|
||
Assignee | ||
Comment 10•20 years ago
|
||
Assignee | ||
Comment 11•20 years ago
|
||
Comment on attachment 196329 [details]
autorepeat-arrow-dn.gif, should probably be in a new mac/global/scrollbox dir
Hang on, lots of wrong stuff...
Attachment #196329 -
Attachment is obsolete: true
Assignee | ||
Comment 12•20 years ago
|
||
Assignee | ||
Comment 13•20 years ago
|
||
Assignee | ||
Comment 14•20 years ago
|
||
This patch assumes:
1) that autorepeat-arrows (attachment #196331 [details] and 196332) are in a new
classic/global/mac/scrollbox directory.
2) that the new menu-arrow.gif and menu-arrow-hover.gif ( attachment #196325 [details]
and #196326) are in classic/global/mac/menu.
The patch doesn't indent icons in certain menulists, (like the one in Sidebar
from where you choose search engine).
Comment 15•20 years ago
|
||
(In reply to comment #12)
>autorepeat-arrow-dn.gif should probably be in a new
classic/global/mac/scrollbox dir
Or possibly classic/global/mac/arrow ?
Assignee | ||
Comment 16•20 years ago
|
||
Comment on attachment 196420 [details] [diff] [review]
Make mac menus/menupopups native-styled
Mano,
can you take a look at this, please? The directory/directories for the images
might be located in a different place, but I feel I'm done with the other
stuff (css etc).
Attachment #196420 -
Flags: review?(bugs.mano)
Assignee | ||
Updated•20 years ago
|
Attachment #196420 -
Flags: superreview?(neil.parkwaycc.co.uk)
Comment 17•20 years ago
|
||
Comment on attachment 196420 [details] [diff] [review]
Make mac menus/menupopups native-styled
Menuitems aren't padded enough with this patch (at least one more pixel at the left side, not sure about vertical paddings).
Index: themes/classic/global/mac/autocomplete.css
===================================================================
/* ::::: autocomplete popups ::::: */
.autocomplete-result-popup,
.autocomplete-history-popup {
- border-width: 1px;
- -moz-border-top-colors: ThreeDDarkShadow;
- -moz-border-right-colors: ThreeDDarkShadow;
- -moz-border-bottom-colors: ThreeDDarkShadow;
- -moz-border-left-colors: ThreeDDarkShadow;
+ -moz-appearance: none !important;
Why is this change?
Index: themes/classic/global/mac/menu.css
===================================================================
+.autocomplete-history-popup > menuitem[_moz-menuactive="true"] {
+ color: inherit !important;
+ background-color: Highlight !important;
+
} ?
Index: themes/classic/global/mac/popup.css
===================================================================
menupopup,
popup {
- -moz-appearance: menu;
- border-top: 2px solid;
- border-right: 3px solid;
- border-bottom: 3px solid;
- border-left: 2px solid;
- -moz-border-top-colors: #000000 #FFFFFF;
- -moz-border-right-colors: #000000 #000000 #777777;
- -moz-border-bottom-colors: #000000 #000000 #777777;
- -moz-border-left-colors: #000000 #FFFFFF;
- background-color: Menu;
+ -moz-appearance: menupopup;
+ padding-bottom: 4px;
+ padding-top: 4px;
Why do the first and last menuitems need more padding? This also seem to break menulists (their scrollbar is misplaced).
Attachment #196420 -
Flags: review?(bugs.mano) → review-
Assignee | ||
Comment 18•20 years ago
|
||
Comment on attachment 196420 [details] [diff] [review]
Make mac menus/menupopups native-styled
(In reply to comment #17)
> (From update of attachment 196420 [details] [diff] [review] [edit])
> Menuitems aren't padded enough with this patch (at least one more pixel at the
> left side, not sure about vertical paddings).
>
I didn't changed the left padding, I had some trouble when I compared with the OS menuitems since the text is smaller in our menuitems. But I'll add 1px to left/right padding.
> Index: themes/classic/global/mac/autocomplete.css
> ===================================================================
>
> /* ::::: autocomplete popups ::::: */
>
> .autocomplete-result-popup,
> .autocomplete-history-popup {
> - border-width: 1px;
> - -moz-border-top-colors: ThreeDDarkShadow;
> - -moz-border-right-colors: ThreeDDarkShadow;
> - -moz-border-bottom-colors: ThreeDDarkShadow;
> - -moz-border-left-colors: ThreeDDarkShadow;
> + -moz-appearance: none !important;
>
> Why is this change?
Since the history drop-down is a menu/menupopup it will look like a menu with the changes in menu.css ("-moz-appearance: menuitem;"). The grey background will shine through.
>
> Index: themes/classic/global/mac/menu.css
> ===================================================================
>
>
> +.autocomplete-history-popup > menuitem[_moz-menuactive="true"] {
> + color: inherit !important;
> + background-color: Highlight !important;
> +
>
> } ?
I want to keep the text black upon select/hover. Without the color style rule the text would turn white because of the "-moz-mac-menutextselect" rule at line 177 in menu.css. I also want the background to behave the same as the autocomplete-popups. With this change the history-dropdown in the url bar will look/behave the same as the autocomplete-popup. We could hardcode the blue background on hovered/selected menuitems if you prefer the history-drop-down to behave/look different from the autocomplete popup (we could also make the background of the items in both widgets turn blue upon hover/select
In Firefox, both widgets look the same in the url bar, though. And i thought that from a user pof it would be more consistent to make them both appear the same.
One way of making the history-dropdown to look like a menu without the grey background is to hardcode the background-color to blue upon select/hover. If we do that, I think we should do it for the autocomplete-popup as well. I can't use "-moz-mac-menuselect" (actually, it also returns a hard-coded color), since it works only on trunk. On branch it returns the old-style color.
>
> Index: themes/classic/global/mac/popup.css
> ===================================================================
>
> menupopup,
> popup {
> - -moz-appearance: menu;
> - border-top: 2px solid;
> - border-right: 3px solid;
> - border-bottom: 3px solid;
> - border-left: 2px solid;
> - -moz-border-top-colors: #000000 #FFFFFF;
> - -moz-border-right-colors: #000000 #000000 #777777;
> - -moz-border-bottom-colors: #000000 #000000 #777777;
> - -moz-border-left-colors: #000000 #FFFFFF;
> - background-color: Menu;
> + -moz-appearance: menupopup;
> + padding-bottom: 4px;
> + padding-top: 4px;
>
> Why do the first and last menuitems need more padding? This also seem to break
> menulists (their scrollbar is misplaced).
The padding that is set for the menuitems in menu.css doesn't affect the bottom/top of the menu when you hover over the first/last menuitem in a menu. In native menus there's a white space above/under the first/last menutiem when you hover it. I missed the issue with the menulist, though. Looking at toolkit's popup.css, I now see that the padding should be added to the ".popup-internal-box" instead. Will fix that.
Attachment #196420 -
Flags: superreview?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 19•20 years ago
|
||
I've used 2 more pixels left/right padding on menuitems,they now look exactly as Apples. The height of the menuitems are now set to a minimum of 19px, which is the same height as the native ones has without icons (in menus and context menus). I also fixed the top/bottom distance to the seaparator.
I've used less margin etc in menulists since the font is rather small.
.autocomplete-history-popup > menuitem {
+ -moz-appearance: none !important;
max-width: none !important;
font: message-box;
+ height: 1.3em;
+ min-height: 18px;
+ padding: 0px 2px 0px 2px;
+ margin: 0;
+}
The last 4 lines of styles are copied from tree.css. This is to make items in the urlbar history-popup look the same as items in the autocomplete one.
I also fixed the issue with the padding in menulists.
As the last patch, this one also uses the images in previous attachments.
Attachment #196420 -
Attachment is obsolete: true
Attachment #202699 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #202699 -
Flags: review?(bugs.mano)
Assignee | ||
Comment 20•20 years ago
|
||
Comment on attachment 202699 [details] [diff] [review]
native-styled menus/menupopups - better version
> The height of the menuitems are now set to a minimum of 19px, which
> is the same height as the native ones has without icons (in menus and context
> menus).
After using the patch for some time I start to feel that this might not be the best idea. Since we're using a smaller font (and I can't see any way of changing the font-size without hard-coding it) than Apple, menuitems appear to have too much vertical space.
Assignee | ||
Comment 21•20 years ago
|
||
This patch is identical to the previous one, except for:
1) No min-heights
2) A single whitespace correction
Attachment #202699 -
Attachment is obsolete: true
Attachment #203143 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #203143 -
Flags: review?(bugs.mano)
Attachment #202699 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #202699 -
Flags: review?(bugs.mano)
Assignee | ||
Comment 22•20 years ago
|
||
New, corrected version: included diff of jar.mn file, corrected an issue with the autocomplete popup's top/bottom padding (shouldn't be any). Otherwise the same as previous patch.
The question here is (in my point of view) the height of the menuitems. Should the height be set to a minimum of 19px or shouldn't there be any height set at all? One could argue that it's more correct not specifying any height, it does looks like apples own menuitems vary in height (according to font-size etc). Currently, with this patch, when no min-height is specified, menuitems in a "normal" context-menu will have a height of 18px.
Attachment #203143 -
Attachment is obsolete: true
Attachment #204949 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #204949 -
Flags: review?(bugs.mano)
Attachment #203143 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #203143 -
Flags: review?(bugs.mano)
Assignee | ||
Updated•20 years ago
|
Summary: Classic: Use native-styled menus/menupopups → [Mac] Classic: Use native-styled menus/menupopups
Blocks: sm1.1
Assignee | ||
Comment 23•20 years ago
|
||
Comment on attachment 204949 [details] [diff] [review]
correct version
Giving this to Mnyromyr instead...
Attachment #204949 -
Flags: review?(bugs.mano) → review?(mnyromyr)
Updated•20 years ago
|
Attachment #204949 -
Flags: review?(bugs.mano)
Assignee | ||
Updated•20 years ago
|
Attachment #204949 -
Flags: review?(mnyromyr)
Assignee | ||
Updated•20 years ago
|
Whiteboard: ETA: SeaMonkey 1.0b → ETA: SeaMonkey 1.1
Assignee | ||
Comment 24•19 years ago
|
||
Comment on attachment 204949 [details] [diff] [review]
correct version
Mano, if you don't have the time for this, please bump the request to mnyromyr. It didn't made it for 1.0, but I would really like to have this for 1.1. Once we're past 1.1, this patch is pretty useless (assuming cocoa toolkit will be used on trunk).
Assignee | ||
Updated•19 years ago
|
Target Milestone: --- → mozilla1.8.1
Version: Trunk → 1.8 Branch
Assignee | ||
Comment 25•19 years ago
|
||
Comment on attachment 204949 [details] [diff] [review]
correct version
I haven't heard a sound from Mano for some time. Mnyromyr, can you take a look at this?
Attachment #204949 -
Flags: review?(mnyromyr)
Comment 26•19 years ago
|
||
Comment on attachment 204949 [details] [diff] [review]
correct version
>Index: themes/classic/global/mac/autocomplete.css
>===================================================================
>+ -moz-appearance: none !important;
>+ border-top: 1px solid ThreeDDarkShadow !important;
>+ padding: 0 !important;
Are all these !importants here (and in the other files) really necessary?
The first one here isn't, I didn't check the others...
Anyway, this patch does work, I do indeed now see native menuitems in our menus, which is very nice.
But you really should get a review from a "Mac-savvy" person, which I'm not (yet), though...
Attachment #204949 -
Flags: review?(mnyromyr) → review+
Assignee | ||
Comment 27•19 years ago
|
||
Comment on attachment 204949 [details] [diff] [review]
correct version
Asaf, can you please add a comment about when you plan to look at this?
Comment 28•19 years ago
|
||
Comment on attachment 204949 [details] [diff] [review]
correct version
Going to review this on Wednesday now that my mac is finally back... Sorry for the latency, Stefan.
Comment 29•19 years ago
|
||
First thing I noticed after applying this patch.
In other news, nice work for remote xul, feel free to port it back to Pinstripe :)
Comment 30•19 years ago
|
||
Also, we need to replace the submenu arrow images (and please open a new bug on nsINativeThemeing that, and the scrollbox images; there are values for them in the HITheme/Old Quickdraw API IIRC).
Comment 31•19 years ago
|
||
Comment on attachment 204949 [details] [diff] [review]
correct version
Also:
* Modifiers (at least command and shift) are not visible in popup menus after applying this patch. You can test it in the webdeveloper-toolbar extension.
* We need new images for checked menuitems, esp. for the :hover case.
Attachment #204949 -
Flags: review?(bugs.mano) → review-
Assignee | ||
Comment 32•19 years ago
|
||
(In reply to comment #30)
> Also, we need to replace the submenu arrow images (and please open a new bug on
Hmm, the sub-menus have new images - see attachment #196325 [details] and attachment #196326 [details]. Did you replace the old images?
> nsINativeThemeing that, and the scrollbox images; there are values for them in
> the HITheme/Old Quickdraw API IIRC).
>
(In reply to comment #31)
> (From update of attachment 204949 [details] [diff] [review] [edit])
> Also:
> * Modifiers (at least command and shift) are not visible in popup menus after
> applying this patch. You can test it in the webdeveloper-toolbar extension.
That's bug 331803 ;-)
> * We need new images for checked menuitems, esp. for the :hover case.
Hmm, I suppose I could take Pinstripe's.
Comment 33•19 years ago
|
||
(In reply to comment #32)
> (In reply to comment #30)
> > Also, we need to replace the submenu arrow images (and please open a new bug on
>
> Hmm, the sub-menus have new images - see attachment #196325 [details] [edit] and attachment
> #196326 [edit]. Did you replace the old images?
oops, sorry.
> > nsINativeThemeing that, and the scrollbox images; there are values for them in
> > the HITheme/Old Quickdraw API IIRC).
> >
>
> (In reply to comment #31)
> > (From update of attachment 204949 [details] [diff] [review] [edit] [edit])
> > Also:
> > * Modifiers (at least command and shift) are not visible in popup menus after
> > applying this patch. You can test it in the webdeveloper-toolbar extension.
>
> That's bug 331803 ;-)
Hrm, I didn't notice it Seamonkey before applying this patch.
>
> > * We need new images for checked menuitems, esp. for the :hover case.
>
> Hmm, I suppose I could take Pinstripe's.
>
ok.
Assignee | ||
Comment 34•19 years ago
|
||
Comment 35•19 years ago
|
||
OK, ignore the modifiers comment; i tested an earlier build.
Assignee | ||
Comment 36•19 years ago
|
||
Just attaching a testcase here. The reason that the scrollbox-arrows appear in the empty "Imported IE Favorites" seems to be because menupopups has a "min-width: 1px" rule in popup.css. If you remove that rule nothing will appear.
Pinstripe doesn't have any min-height rule. But in Firefox you'll notice a tiny "menupopup" in the testcase - guess it comes from the margin added to the autorepeatbutton in Pinstripe's scrollbox.css.
Assignee | ||
Comment 37•19 years ago
|
||
Assignee | ||
Comment 38•19 years ago
|
||
Assignee | ||
Comment 39•19 years ago
|
||
Assignee | ||
Comment 40•19 years ago
|
||
New version (please use images in previous 3 attachments):
1) Uses Pinstripe's checkmarks in menuitems
2) Fixes the empty scrollbox by removing the "min-width: 1px;" rule in popup.css and adding margin to the autorepeatbutton in scrollbox.css
Attachment #204949 -
Attachment is obsolete: true
Attachment #218306 -
Flags: superreview?(neil)
Attachment #218306 -
Flags: review?(bugs.mano)
Attachment #204949 -
Flags: superreview?(neil)
Assignee | ||
Comment 41•19 years ago
|
||
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments
Just transfering Mnyromyr's r+...
Attachment #218306 -
Flags: review+
Assignee | ||
Comment 42•19 years ago
|
||
> (and please open a new bug on
> nsINativeThemeing that, and the scrollbox images; there are values for them in
> the HITheme/Old Quickdraw API IIRC).
>
Filed bug 333910.
Comment 43•19 years ago
|
||
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments
r=mano
Attachment #218306 -
Flags: review?(bugs.mano) → review+
Comment 44•19 years ago
|
||
When I apply this last changes then the scrollbox for empty bookmark folders vanishes - almost. I still see a thin 1px(?) wide vertical white line, about 2/3 the height of the folder entry...
Assignee | ||
Comment 45•19 years ago
|
||
(In reply to comment #44)
> When I apply this last changes then the scrollbox for empty bookmark folders
> vanishes - almost. I still see a thin 1px(?) wide vertical white line, about
> 2/3 the height of the folder entry...
>
That's by purpose. It's the margin I added in scrollbox.css. Better show something than nothing at all. Compare with how it looks now (screenshot in attachment #218274 [details]).
Comment 46•19 years ago
|
||
(In reply to comment #45)
>Compare with how it looks now
*I* can't...
Assignee | ||
Comment 47•19 years ago
|
||
Assignee | ||
Comment 48•19 years ago
|
||
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments
Neil, any chance you could look at this soon? It would be nice to finally have this on branch for 1.1a.
Comment 49•19 years ago
|
||
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments
>- skin/classic/global/menu/menu-check.gif (global/mac/menu/menu-check.gif)
>- skin/classic/global/menu/menu-check-dis.gif (global/mac/menu/menu-check-dis.gif)
>- skin/classic/global/menu/menu-check-hov.gif (global/mac/menu/menu-check-hov.gif)
>+ skin/classic/global/menu/menu-check.png (global/mac/menu/menu-check.png)
>+ skin/classic/global/menu/menu-check-dis.png (global/mac/menu/menu-check-dis.png)
>+ skin/classic/global/menu/menu-check-hov.png (global/mac/menu/menu-check-hov.png)
Why the change from .gif to .png? Do radio menuitems use tick marks too?
>+.menu-iconic,
>+.menuitem-iconic {
>+ padding: 2px 2px 2px 18px !important;
>+ margin: 0 !important;
>+}
>+
>+.menu-iconic[checked="true"],
>+.menuitem-iconic[checked="true"],
>+.menuitem-iconic[value] {
>+ padding-left: 0px !important;
>+}
Why do these change only when the menuitem is checked or has a value?
[menus are never checked]
> menulist > menupopup > menuitem,
> .menulist-menupopup > menuitem {
> max-width: none;
>- padding: 0px 20px 0px 0px;
>+ padding-right: 20px;
Why this change?
Assignee | ||
Comment 50•19 years ago
|
||
(In reply to comment #49)
> (From update of attachment 218306 [details] [diff] [review] [edit])
> >- skin/classic/global/menu/menu-check.gif (global/mac/menu/menu-check.gif)
> >- skin/classic/global/menu/menu-check-dis.gif (global/mac/menu/menu-check-dis.gif)
> >- skin/classic/global/menu/menu-check-hov.gif (global/mac/menu/menu-check-hov.gif)
> >+ skin/classic/global/menu/menu-check.png (global/mac/menu/menu-check.png)
> >+ skin/classic/global/menu/menu-check-dis.png (global/mac/menu/menu-check-dis.png)
> >+ skin/classic/global/menu/menu-check-hov.png (global/mac/menu/menu-check-hov.png)
> Why the change from .gif to .png?
I took Pinstripe's tick marks (see comment #40). Do you want me to convert them to gif's?
> Do radio menuitems use tick marks too?
I've never seen those "radio"-styled marks on mac. It looks pretty weird having those in some of the menuitems. So, yes - they should use tick marks as well. Hmm... oh, I could just use the style rules for the "checkbox menuitem" (rename the commented title to "Checked menuitems" instead of "Checkbox menuitems").
> >+.menu-iconic,
> >+.menuitem-iconic {
> >+ padding: 2px 2px 2px 18px !important;
> >+ margin: 0 !important;
> >+}
> >+
> >+.menu-iconic[checked="true"],
> >+.menuitem-iconic[checked="true"],
> >+.menuitem-iconic[value] {
> >+ padding-left: 0px !important;
> >+}
> Why do these change only when the menuitem is checked or has a value?
I actually spotted two errors here. What I want here is the text to line up vertically, text in a checked menuitem should align vertically with text in a unchecked menuitem. Tick marks should appear "outside". But I see now that I missed 2px here:
+.menuitem-iconic[value] {
+ padding-left: 0px !important;
padding-left should be 2px (since the icon is 16x16). I should also add "menuitem-iconic[disabled="true"]"
> [menus are never checked]
I suppose I wanted to be on the safe side :-/
>
> > menulist > menupopup > menuitem,
> > .menulist-menupopup > menuitem {
> > max-width: none;
> >- padding: 0px 20px 0px 0px;
> >+ padding-right: 20px;
> Why this change?
>
I want the same top, left and bottom padding as menuitems in menupopups.
Comment 51•19 years ago
|
||
(In reply to comment #50)
>>Why the change from .gif to .png?
>I took Pinstripe's tick marks (see comment #40). Do you want me to convert them
>to gif's?
Either that or I suppose you could just use absolute paths.
>Hmm... oh, I could just use the style rules for the "checkbox menuitem" (rename
>the commented title to "Checked menuitems" instead of "Checkbox menuitems").
Yes, that would work too.
>>>+.menu-iconic[checked="true"],
>>>+.menuitem-iconic[checked="true"],
>>>+.menuitem-iconic[value] {
>>>+ padding-left: 0px !important;
>>>+}
>>Why do these change only when the menuitem is checked or has a value?
>I actually spotted two errors here. What I want here is the text to line up
>vertically, text in a checked menuitem should align vertically with text in a
>unchecked menuitem. Tick marks should appear "outside". But I see now that I
>missed 2px here:
>
>+.menuitem-iconic[value] {
>+ padding-left: 0px !important;
>
>padding-left should be 2px (since the icon is 16x16). I should also add
>"menuitem-iconic[disabled="true"]"
>>[menus are never checked]
> I suppose I wanted to be on the safe side :-/
I'm not following you. Why does the padding depend on whether the tick mark image is assigned to the image or not? In other themes, the image size is set to 16px whether or not the tick is present. And I have no idea why you might want to change padding depending on whether the item is disabled or has a value.
Assignee | ||
Comment 52•19 years ago
|
||
> I'm not following you. Why does the padding depend on whether the tick mark
> image is assigned to the image or not? In other themes, the image size is set
> to 16px whether or not the tick is present. And I have no idea why you might
> want to change padding depending on whether the item is disabled or has a
> value.
Uh, forget what I said... I should of course use ".menuitem-iconic[type="checkbox"]" etc. Thinking of it, I would actually prefer to use variants of .menu-iconic-left to indent the icons/tick marks. Not sure if it will work, though (will check).
Comment 53•19 years ago
|
||
Comment on attachment 218306 [details] [diff] [review]
New version addressing Mano's comments
Minusing for now as I'm expecting an updated patch which addresses the outstanding comments.
Attachment #218306 -
Flags: superreview?(neil) → superreview-
Assignee | ||
Comment 54•19 years ago
|
||
Changes since last patch:
-- menu.css --
* ".menu-iconic-left" is used to indent icons. Icons in menulists menuitems, and tick marks in menuitems are not indented with ".menu-iconic-left", though. In the previous patch icons (like in the bm menu) didn't line up 100% with label text in menuitems without icons - now they do.
* Removed the style rules for checked radio menuitems.
FYI, this:
+.menu-iconic,
+.menuitem-iconic {
+ margin-top: 0 !important;
+}
is because of the negative top margin set in communicator/bookmarks/bookmarksToolbar.css#130. I just simplyfied it (used margin: 0 !important; before).
-- jar.mn --
* Now using Pinstripe's tick marks straight from /toolkit/themes/pinstripe/global/menu.
Transferring r+
Attachment #218300 -
Attachment is obsolete: true
Attachment #218301 -
Attachment is obsolete: true
Attachment #218302 -
Attachment is obsolete: true
Attachment #218306 -
Attachment is obsolete: true
Attachment #221843 -
Flags: superreview?(neil)
Attachment #221843 -
Flags: review+
Updated•19 years ago
|
Attachment #221843 -
Flags: superreview?(neil) → superreview+
![]() |
||
Comment 55•19 years ago
|
||
Checked in by request of stefanh:
Checking in themes/classic/jar.mn;
/cvsroot/mozilla/themes/classic/jar.mn,v <-- jar.mn
new revision: 1.142; previous revision: 1.141
done
Checking in themes/classic/global/mac/autocomplete.css;
/cvsroot/mozilla/themes/classic/global/mac/autocomplete.css,v <-- autocomplete.css
new revision: 1.14; previous revision: 1.13
done
Checking in themes/classic/global/mac/menu.css;
/cvsroot/mozilla/themes/classic/global/mac/menu.css,v <-- menu.css
new revision: 1.41; previous revision: 1.40
done
Checking in themes/classic/global/mac/popup.css;
/cvsroot/mozilla/themes/classic/global/mac/popup.css,v <-- popup.css
new revision: 1.11; previous revision: 1.10
done
Checking in themes/classic/global/mac/scrollbox.css;
/cvsroot/mozilla/themes/classic/global/mac/scrollbox.css,v <-- scrollbox.css
new revision: 1.6; previous revision: 1.5
done
Checking in themes/classic/global/mac/menu/menu-arrow.gif;
/cvsroot/mozilla/themes/classic/global/mac/menu/menu-arrow.gif,v <-- menu-arrow.gif
new revision: 1.2; previous revision: 1.1
done
Checking in themes/classic/global/mac/menu/menu-arrow-hov.gif;
/cvsroot/mozilla/themes/classic/global/mac/menu/menu-arrow-hov.gif,v <-- menu-arrow-hov.gif
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/themes/classic/global/mac/scrollbox/autorepeat-arrow-dn.gif,v
done
Checking in themes/classic/global/mac/scrollbox/autorepeat-arrow-dn.gif;
/cvsroot/mozilla/themes/classic/global/mac/scrollbox/autorepeat-arrow-dn.gif,v <-- autorepeat-arrow-dn.gif
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/themes/classic/global/mac/scrollbox/autorepeat-arrow-up.gif,v
done
Checking in themes/classic/global/mac/scrollbox/autorepeat-arrow-up.gif;
/cvsroot/mozilla/themes/classic/global/mac/scrollbox/autorepeat-arrow-up.gif,v <-- autorepeat-arrow-up.gif
initial revision: 1.1
done
Assignee | ||
Comment 56•19 years ago
|
||
I'll let this bake on trunk for a while before asking branch approval.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 57•19 years ago
|
||
Hm. We need a new disabled menu-arrow. Now when I think of it, KaiRo actually asked me about that when he was about to check in the patch. For some reason I thought it was okay to not having a new menu-arrow-dis.gif.
Assignee | ||
Comment 58•19 years ago
|
||
Neil, can you r/sr the image so it can be checked in?
Attachment #223216 -
Flags: superreview?(neil)
Attachment #223216 -
Flags: review?(neil)
Updated•19 years ago
|
Attachment #223216 -
Flags: superreview?(neil)
Attachment #223216 -
Flags: superreview+
Attachment #223216 -
Flags: review?(neil)
Attachment #223216 -
Flags: review+
Assignee | ||
Comment 59•19 years ago
|
||
Updated•19 years ago
|
Attachment #223216 -
Attachment description: New menu-arrow.dis → New menu-arrow.dis [checked in on trunk]
Assignee | ||
Updated•19 years ago
|
Whiteboard: ETA: SeaMonkey 1.1 → ETA: SeaMonkey 1.1a (fixed on trunk)
Assignee | ||
Comment 60•19 years ago
|
||
Moving this to a product where the correct flags can be set...
Component: Themes → General
Product: Core → Mozilla Application Suite
Target Milestone: mozilla1.8.1 → seamonkey1.1alpha
Assignee | ||
Comment 61•19 years ago
|
||
Comment on attachment 221843 [details] [diff] [review]
New version addressing Neil's comments
Since I'll be gone for two weeks I request approval now. There's no need to hurry, though - this is just in case things should tighten up for 1.1a before I get back (seems unlikely, but anyway...) . If that is the case and the patch gets approval, I kindly request the granter to check in the patch.
Attachment #221843 -
Flags: approval-seamonkey1.1a?
Assignee | ||
Comment 62•19 years ago
|
||
To avoid any confusion if someone checks in the patch while I'm away - here's what needs to get in:
1) Attachment #196325 [details], attachment #196326 [details] and attachment #223216 [details] in themes/classic/global/mac/menu
2) A 'scrollbox' directory needs to be created in themes/classic/global/mac
3) Attachment #196331 [details] and attachment #196332 [details] can then be checked in to the new scrollbox directory
4) Attachment #221843 [details] [diff] (the patch)
Note: since bug 311737 doesn't seems to have been checked in on the 1.8 branch (at least the mac version of autocomplete.css hasn't been changed), the patch will fail in autocomplete.css - I'll attach diff for the branch version of autocomplete.css
Assignee | ||
Comment 63•19 years ago
|
||
Assignee | ||
Comment 64•19 years ago
|
||
Comment on attachment 223513 [details] [diff] [review]
1.8 diff of autocomplete.css
The patch in bug 311737 have now landed on the 1.8 branch.
Attachment #223513 -
Attachment is obsolete: true
Assignee | ||
Comment 65•19 years ago
|
||
Since bug 335683 is fixed on trunk/1.8 we can now use the correct font in mac menuitems. Before, we used the standard system fonts for menus/menuitems - with this patch we'll use the real menuitemfont (1px larger on the locales I know). I also removed the bold font for default menuitems - I haven't seen this in mac apps (well, firefox seem to use it in the search pop-down for the default selected search engine, but that's it). I also belive it's a bit of an overkill to use it and with the larger font it also becomes a little bit too large.
Assignee | ||
Comment 66•19 years ago
|
||
Comment on attachment 226500 [details] [diff] [review]
Use correct font for menuitems
Sight, forgot to set the review flags - see previous comment for an explanation
Attachment #226500 -
Flags: superreview?(neil)
Attachment #226500 -
Flags: review?(bugs.mano)
Comment 67•19 years ago
|
||
Comment on attachment 226500 [details] [diff] [review]
Use correct font for menuitems
Let's leave the bold style rule for now (At least for xul-default-themes consistency).
Attachment #226500 -
Flags: review?(bugs.mano)
Assignee | ||
Comment 68•19 years ago
|
||
> Let's leave the bold style rule for now (At least for xul-default-themes
> consistency).
Sure (note: Pinstripe doesn't have it - but Winstripe does).
Attachment #226500 -
Attachment is obsolete: true
Attachment #226635 -
Flags: superreview?(neil)
Attachment #226635 -
Flags: review?(bugs.mano)
Attachment #226500 -
Flags: superreview?(neil)
Comment 69•19 years ago
|
||
Comment on attachment 226635 [details] [diff] [review]
Keep bold style (Checked in trunk & 1.8 branch)
Weird, as far as i remember, i did see the bold style rule in Pinstripe (in Firefox's searchbar) but LXR tells me i'm wrong. Nevertheless, we should have a separate bug for this.
r=mano.
Attachment #226635 -
Flags: review?(bugs.mano) → review+
Comment on attachment 221843 [details] [diff] [review]
New version addressing Neil's comments
a=me for 1.1a
Attachment #221843 -
Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
Updated•19 years ago
|
Attachment #226635 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 71•19 years ago
|
||
Comment on attachment 226635 [details] [diff] [review]
Keep bold style (Checked in trunk & 1.8 branch)
This will complete the native style of our menus/menuitems - correct system font for menuitems.
Attachment #226635 -
Flags: approval-seamonkey1.1a?
Comment on attachment 226635 [details] [diff] [review]
Keep bold style (Checked in trunk & 1.8 branch)
a=me for 1.1a
Attachment #226635 -
Flags: approval-seamonkey1.1a? → approval-seamonkey1.1a+
Keywords: relnote
Comment 73•19 years ago
|
||
Comment on attachment 221843 [details] [diff] [review]
New version addressing Neil's comments
Checking in (1.8 branch)
jar.mn;
new revision: 1.136.2.6; previous revision: 1.136.2.5
global/mac/autocomplete.css;
new revision: 1.12.8.2; previous revision: 1.12.8.1
global/mac/menu.css;
new revision: 1.40.28.1; previous revision: 1.40
global/mac/popup.css;
new revision: 1.10.28.1; previous revision: 1.10
global/mac/scrollbox.css;
new revision: 1.5.150.1; previous revision: 1.5
global/mac/menu/menu-arrow.gif;
new revision: 1.1.150.1; previous revision: 1.1
global/mac/menu/menu-arrow-hov.gif;
new revision: 1.1.150.1; previous revision: 1.1
global/mac/menu/menu-arrow-dis.gif;
new revision: 1.1.150.1; previous revision: 1.1
global/mac/scrollbox/autorepeat-arrow-dn.gif;
new revision: 1.1.4.1; previous revision: 1.1
global/mac/scrollbox/autorepeat-arrow-up.gif;
new revision: 1.1.4.1; previous revision: 1.1
done
Comment 74•19 years ago
|
||
Comment on attachment 226635 [details] [diff] [review]
Keep bold style (Checked in trunk & 1.8 branch)
Checking in (trunk)
menu.css;
new revision: 1.42; previous revision: 1.41
done
Checking in (1.8 branch)
menu.css;
new revision: 1.40.28.2; previous revision: 1.40.28.1
done
Attachment #226635 -
Attachment description: Keep bold style → Keep bold style (Checked in trunk & 1.8 branch)
Keywords: fixed-seamonkey1.1a
Assignee | ||
Comment 75•19 years ago
|
||
Everything is in on trunk and branch now - thanks!
Whiteboard: ETA: SeaMonkey 1.1a (fixed on trunk)
You need to log in
before you can comment on or make changes to this bug.
Description
•