Last Comment Bug 348614 - Add generic arrow/dropmarker buttons
: Add generic arrow/dropmarker buttons
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Neil Deakin
:
Mentors:
Depends on: 350739
Blocks: 92174
  Show dependency treegraph
 
Reported: 2006-08-14 09:15 PDT by Neil Deakin
Modified: 2006-09-01 13:41 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Style default <dropmarker> (67.33 KB, patch)
2006-08-18 08:57 PDT, Neil Deakin
neil: first‑review-
Details | Diff | Splinter Review
Mac Theme Changes (3.78 KB, patch)
2006-08-18 09:17 PDT, Neil Deakin
roc: first‑review+
Details | Diff | Splinter Review
Fix review comments (67.35 KB, patch)
2006-08-22 07:49 PDT, Neil Deakin
neil: first‑review+
asaf: second‑review+
Details | Diff | Splinter Review

Description Neil Deakin 2006-08-14 09:15:16 PDT
For the date/time pickers, a native themed dropbutton is needed and/or some simple arrow buttons. It would be useful to have these available as stock buttons.
Comment 1 alexander :surkov 2006-08-14 09:37:36 PDT
Sorry :(
Comment 2 Neil Deakin 2006-08-18 08:57:08 PDT
Created attachment 234417 [details] [diff] [review]
Style default <dropmarker>

Not sure if this approach is a good idea. It might be more practical though to just add a new element.
Comment 3 Neil Deakin 2006-08-18 09:17:41 PDT
Created attachment 234422 [details] [diff] [review]
Mac Theme Changes
Comment 4 neil@parkwaycc.co.uk 2006-08-19 12:40:29 PDT
Comment on attachment 234417 [details] [diff] [review]
Style default <dropmarker>

Gosh, I hadn't realised that our dropmarkers were in such a mess.
This is just a inspection review, I haven't actually tried this out.

> .toolbarbutton-menu-dropmarker {
>-  list-style-image: url("chrome://global/skin/arrow/arrow-dn.gif");
>+  -moz-appearance: none !important;
>+  border: none !important;
>+  background-color: transparent !important;
>   -moz-image-region: auto; /* cut off inheritance */
> }
Please set -moz-image-region when you set list-style-image (except to none or to override another rule) so that you don't inherit a region by mistake.

> .autocomplete-history-dropmarker {
>-  -moz-appearance: menulist-button;
>   min-width: 17px;
I don't think we need to keep this width (multiple occurrences).

> .button-menu-dropmarker,
> .button-menubutton-dropmarker {
>-  margin: 1px;
>-  background-image: url("chrome://global/skin/arrow/arrow-dn.gif");
>+  -moz-appearance: none !important;
>+   margin: 1px;
>+  list-style-image: none !important;
>+   background-image: url("chrome://global/skin/arrow/arrow-dn.gif");
>+  background-repeat: no-repeat;
>+  background-position: center center;
>+  background-color: transparent !important;
>+  border: none !important;
Does native themeing conflict with the use of a list-style-image?
If so, your indent on the margin and background-image is wrong, and you could consider setting all the background properties at once i.e.
background: url("chrome://global/skin/arrow/arrow-dn.gif") center no-repeat;

>+  padding-top: 2px;
>+  padding-bottom: 0px;
>+  -moz-padding-start: 2px;
>+  -moz-padding-end: 0px;
XPFE isn't RTL-ised so you can get away with padding: 2px 0px 0px 2px; here.
[I'm hoping for someone to implement -moz-border-padding-direction: rtl;]

>+dropmarker[disabled="true"] {
>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn-dis.gif");
Don't you already inherit that?

> .button-menu-dropmarker,
> .button-menubutton-dropmarker {
>   margin: 1px;
>+  list-style-image: none;
>+  background-color: transparent;
>   background-image: url("chrome://global/skin/arrow/arrow-dn.gif");
>+  border: none;
>   min-width:9px;
>   min-height:5px;
> }
Does this element not work as a list-style-image either?

>+  -moz-border-radius-topleft: 2px;
>+  -moz-border-radius-bottomleft: 0;
I think you meant to set them both to zero? (I'd also prefer 0px)
Comment 5 Neil Deakin 2006-08-22 07:49:12 PDT
Created attachment 234931 [details] [diff] [review]
Fix review comments
Comment 6 neil@parkwaycc.co.uk 2006-08-22 14:18:42 PDT
Comment on attachment 234931 [details] [diff] [review]
Fix review comments

>+  padding-top: 2px 0 0 2px;
[needs fixing twice] I'm sure I said "padding:" ;-)

>+dropmarker[disabled="true"]
Missing {

(In reply to comment #4)
>>+dropmarker[disabled="true"] {
>>+  list-style-image: url("chrome://global/skin/arrow/arrow-dn-dis.gif");
>Don't you already inherit that?
Oops, I must have been asleep :-[
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-08-27 03:40:33 PDT
Comment on attachment 234931 [details] [diff] [review]
Fix review comments

Pinstripe doesn't support RTL chrome either (Winstripe does), please simplify the style rules there too.

r=mano with that fixed.

Note You need to log in before you can comment on or make changes to this bug.