Closed Bug 660672 Opened 10 years ago Closed 10 years ago

Toolbar dropdowns looks different than the other buttons in Aero glass

Categories

(Thunderbird :: Theme, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 8.0

People

(Reporter: andreasn, Assigned: Paenglab)

References

Details

Attachments

(5 files, 3 obsolete files)

Attached image screenshot of issue
The dropdowns looks different from the other buttons under Aero glass.
Preemptively blocking bug 608792 in case a patch here causes similar accessibility problems as are already seen with the toolbar buttons with high-contrast themes [or as a reminder to consider such aspects ;-) ].
Blocks: 608792
Attachment #536147 - Flags: ui-review?(nisses.mail)
Comment on attachment 536147 [details] [diff] [review]
Patch changing the menulist appearance

Patch giving in mainWindow menulists the toolbarbutton appearance. I removed the highlight background color on menulists with focus but leaved the focus ring.

rsx11m: The change applies only on Aero Basic and Glass. On Classic and High color the default appearance is used. If we want the new appearance on classic, we have to double the code because -moz-windows-default-theme applies only on Basic and Glass.
Sorry for the spam. This patch needs Bug 658328 applied.
Blocks: 645294
This label looks a bit odd. I think it looked better before.
But apart from that it looks all good, so I would give this a ui-r+ with that fixed.
Attached patch New menulist appearance (obsolete) — Splinter Review
The only change is a "connecting" background between label and menulist under Aero.
Assignee: nobody → richard.marti
Attachment #536147 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #536147 - Flags: ui-review?(nisses.mail)
Attachment #543474 - Flags: ui-review?(nisses.mail)
Would this look better? The label needs a background on Aero like the menubar. With "connecting" the label to the menulist it's clearer for what the label is. I could also try with more padding on top/bottom to give the background the same high as the menulist, but I don't know if this always fitts with non default font height.
(In reply to comment #8)
> Created attachment 543475 [details]
> Label background with last patch
> 
> Would this look better? The label needs a background on Aero like the
> menubar. With "connecting" the label to the menulist it's clearer for what
> the label is.

You're right, it does need some background, so the previous patch in comment #3 is good in that case (the connecting label don't look as good). Just to double check (since for some odd reason this patch fails to apply for me now), could you post a screenshot of these controls inside the customization dialog as well?
Attached patch New menulist appearance (obsolete) — Splinter Review
This is the old patch against the actual file in hg. This should apply now.
Attachment #543474 - Attachment is obsolete: true
Attachment #543474 - Flags: ui-review?(nisses.mail)
Attachment #544010 - Flags: ui-review?(nisses.mail)
Comment on attachment 544010 [details] [diff] [review]
New menulist appearance

Works great!
Attachment #544010 - Flags: ui-review?(nisses.mail) → ui-review+
Attachment #544010 - Flags: review?(bwinton)
Comment on attachment 544010 [details] [diff] [review]
New menulist appearance

Review of attachment 544010 [details] [diff] [review]:
-----------------------------------------------------------------

Other than the minor nit below, the code looks fine to me, although I'll be interested to see how it interacts with a different background.

r=me, assuming that that's fine.  (I'm building it now, and will test it after that.)

Thanks,
Blake.

::: mail/themes/qute/mail/mailWindow1-aero.css
@@ -174,0 +174,34 @@
> > +  menulist {
> > +    -moz-appearance: none;
> > +    padding: 1px 5px !important;
> > +    background: rgba(151, 152, 153, .05)
NaN more ...

I think these should line up with the "0 0" on the previous line.
Attachment #544010 - Flags: review?(bwinton) → review+
Do you mean the two spaces before menulist {  etc.? This is an indentation after the @media all and (-moz-windows-default-theme)
Patch addressing the comment for check-in with ui-r+ and r+ from previous patch.
Attachment #544010 - Attachment is obsolete: true
Attachment #544554 - Flags: ui-review+
Attachment #544554 - Flags: review+
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/1cb6eac2ee6f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 8.0
You need to log in before you can comment on or make changes to this bug.