624295 - css enhancements for editBMPanel

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Stefan [:stefanh]]

While doing bug 620132, I found some stuff that could be improved a bit. It's basically getting rid of some important rules, descendant selectors. I also spotted a new thing; the checkbox list-style-image could be switched to a background-image (override the one in listbox.css instead of adding a list-style-image).

Comment 1

[Stefan [:stefanh]]

Attachment: Fix style rules

I did some more in bug 620132, but I don't think you're interested in that (grouped focus rules etc together and styled a hidden checkbox).

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 502400 [details] [diff] [review]
Fix style rules

>-#editBookmarkPanel .expander-up,
>-#editBookmarkPanel .expander-down:hover:active {
>+.expander-up,
>+.expander-down:hover:active {
>   list-style-image: url("chrome://browser/skin/hud-style-expander-open.png") !important;
> }
> 
>-#editBookmarkPanel .expander-down,
>-#editBookmarkPanel .expander-up:hover:active {
>+.expander-down,
>+.expander-up:hover:active {
>   list-style-image: url("chrome://browser/skin/hud-style-expander-closed.png") !important;
> }

Extensions may use this class in browser.xul but outside of the panel, so I don't think these changes are correct.

Comment 3

[Stefan [:stefanh]]

Attachment: New version

Yeah, you're right. How about this? Might be an overkill, but I couldn't resist. Otherwise I'll just keep the old rules.

Comment 4

[Dão Gottwald [:dao]]

I think we should keep the old selectors.

Comment 5

[Stefan [:stefanh]]

Attachment: Keep rules

Btw, note that extensions will get pushbutton styling if they use the expander classes. But I suppose that's better than the arrow-panel look.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 502480 [details] [diff] [review]
Keep rules

>-#editBookmarkPanel .expander-up,
>-#editBookmarkPanel .expander-down {
>+#editBMPanel_foldersExpander,
>+#editBMPanel_tagsSelectorExpander {
>   @hudButton@
>   border-radius: 3px;
>   -moz-margin-start: 4px;
>   -moz-margin-end: 2px;
>   padding: 0;
>   -moz-padding-start: 4px;
>   min-width: 10px;
>   min-height: 20px;
> }
> 
>-#editBookmarkPanel .expander-up:-moz-focusring,
>-#editBookmarkPanel .expander-down:-moz-focusring {
>+#editBMPanel_foldersExpander:-moz-focusring,
>+#editBMPanel_tagsSelectorExpander:-moz-focusring {
>   @hudButtonFocused@
> }
> 
>-#editBookmarkPanel .expander-up:hover:active,
>-#editBookmarkPanel .expander-down:hover:active {
>+#editBMPanel_foldersExpander:hover:active,
>+#editBMPanel_tagsSelectorExpander:hover:active {
>   @hudButtonPressed@
> }

Please also avoid these changes.

Comment 7

[Stefan [:stefanh]]

Attachment: For check-in, comment #6 addressed

I also removed a change that wasn't supposed to be there:
 #customizeToolbarSheetPopup {
   -moz-window-shadow: sheet;
+  -moz-appearance: none;
+  background-color: transparent;
 }

Comment 8

[Stefan [:stefanh]]

Comment on attachment 502494 [details] [diff] [review]
For check-in, comment #6 addressed

css-cleanup, small and safe.

Comment 9

[Stefan [:stefanh]]

Attachment: Final version

Sorry, just found one thing I want to fix... The only new thing here is the "#editBMPanel_tagsSelector .listcell-check" background-color put together with the new "background" and the listbox.css adjustment (I suddenly remember that I forgot to remove that in 2009 and sooner or later someone will remove the no-repeat from there and regress the panel).

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 502953 [details] [diff] [review]
Final version

> #editBMPanel_tagsSelector .listcell-check {
>-  -moz-appearance: none !important;
>-  background-color: transparent;
>+  -moz-appearance: none;
>   border: 0;
>-  list-style-image: url("chrome://browser/skin/hud-style-check-box-empty.png");
>+  background: transparent url("chrome://browser/skin/hud-style-check-box-empty.png") no-repeat 50% 50%;

remove 'transparent'

> .listcell-check[checked="true"] {
>-  background-image: url("chrome://global/skin/checkbox/cbox-check.gif");
>+  background-image: url("chrome://global/skin/checkbox/cbox-check.gif") no-repeat 50% 50%;
> }

background-image doesn't support these values.

Comment 11

[Stefan [:stefanh]]

Agh. OK, so this would also mean that I can remove the -moz-field background-color of the -listcell-check in listbox-css since using 'background' instead of background-image will override it anyway. Anyway, will look at it tomorrow.

Comment 12

[Stefan [:stefanh]]

Attachment: A new final version

Sorry for the confusion, I should go to bed earlier :-(

OK, so I think it's better to keep the background-color in listbox.css despite my previous comment. Another option would be to leave out the listbox.css changes while keeping all the other changes.

Comment 13

[Dão Gottwald [:dao]]

Please leave out the listbox.css.

Comment 14

[Stefan [:stefanh]]

Attachment: Really, really final

(In reply to comment #13)
> Please leave out the listbox.css.
Done.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 503216 [details] [diff] [review]
Really, really final

Thanks!

Comment 16

[Stefan [:stefanh]]

Comment on attachment 503216 [details] [diff] [review]
Really, really final

css-cleanup, small and safe.

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 503216 [details] [diff] [review]
Really, really final

a=beltzner

Comment 18

[Stefan [:stefanh]]

http://hg.mozilla.org/mozilla-central/rev/8fdde63f610e