Closed Bug 301105 Opened 20 years ago Closed 19 years ago

[Mac] Classic: Use native-styled menus/menupopups

Categories

(SeaMonkey :: General, defect)

1.8 Branch
PowerPC
macOS
defect
Not set
normal

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+
Details | Diff | Splinter Review
847 bytes, image/gif
neil
: review+
Details
23.79 KB, image/gif
Details
692 bytes, patch
asaf
: review+
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.
Summary: Classic: Use native-styled menus/menupopups, line up items in urlbar popups → Classic: Use native-styled menus/menupopups
Whiteboard: ETA: SeaMonkey 1.0b
*** Bug 306284 has been marked as a duplicate of this bug. ***
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
> Just re-name it to classic.jar Err, no need to rename it, of course...
Attached file Testcase for xul menus
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.
> 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°).
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. :)
(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...
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
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).
(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 ?
Blocks: 46175
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)
Attachment #196420 - Flags: superreview?(neil.parkwaycc.co.uk)
Blocks: 179760
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-
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)
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)
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.
Attached patch Even better version (obsolete) — Splinter Review
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)
Attached patch correct version (obsolete) — Splinter Review
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)
Summary: Classic: Use native-styled menus/menupopups → [Mac] Classic: Use native-styled menus/menupopups
Comment on attachment 204949 [details] [diff] [review] correct version Giving this to Mnyromyr instead...
Attachment #204949 - Flags: review?(bugs.mano) → review?(mnyromyr)
Attachment #204949 - Flags: review?(bugs.mano)
Attachment #204949 - Flags: review?(mnyromyr)
Whiteboard: ETA: SeaMonkey 1.0b → ETA: SeaMonkey 1.1
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).
Target Milestone: --- → mozilla1.8.1
Version: Trunk → 1.8 Branch
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 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+
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 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.
Attached image empty scrollbox
First thing I noticed after applying this patch. In other news, nice work for remote xul, feel free to port it back to Pinstripe :)
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 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-
(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.
(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.
Attached image scrollbox before patch
OK, ignore the modifiers comment; i tested an earlier build.
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.
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)
Comment on attachment 218306 [details] [diff] [review] New version addressing Mano's comments Just transfering Mnyromyr's r+...
Attachment #218306 - Flags: review+
> (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 on attachment 218306 [details] [diff] [review] New version addressing Mano's comments r=mano
Attachment #218306 - Flags: review?(bugs.mano) → review+
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...
(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]).
(In reply to comment #45) >Compare with how it looks now *I* can't...
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 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?
(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.
(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.
> 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 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-
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+
Attachment #221843 - Flags: superreview?(neil) → superreview+
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
I'll let this bake on trunk for a while before asking branch approval.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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.
Neil, can you r/sr the image so it can be checked in?
Attachment #223216 - Flags: superreview?(neil)
Attachment #223216 - Flags: review?(neil)
Attachment #223216 - Flags: superreview?(neil)
Attachment #223216 - Flags: superreview+
Attachment #223216 - Flags: review?(neil)
Attachment #223216 - Flags: review+
Attachment #223216 - Attachment description: New menu-arrow.dis → New menu-arrow.dis [checked in on trunk]
Whiteboard: ETA: SeaMonkey 1.1 → ETA: SeaMonkey 1.1a (fixed on trunk)
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
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?
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
Attached patch 1.8 diff of autocomplete.css (obsolete) — Splinter Review
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
Attached patch Use correct font for menuitems (obsolete) — Splinter Review
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.
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 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)
> 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 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+
Attachment #226635 - Flags: superreview?(neil) → superreview+
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+
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 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)
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.

Attachment

General

Created:
Updated:
Size: