Display default dialog buttons in pulsating blue color

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Cocoa
--
minor
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: phil locke, Assigned: mstange)

Tracking

(Blocks: 1 bug, {polish})

Trunk
mozilla1.9.2a1
All
Mac OS X
polish
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030507
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4b) Gecko/20030507

i've noticed that many (or most, or all) of the dialogs do not have the default
button highlighted.  for example, in the dialog asking whether to save an unsent
message in the drafts folder, the default button (activated by pressing Return)
is "Save".  in the dialog asking whether to send a message in plain text, text &
html, or html only, the default is "Send".  in the dialog asking whether to save
the password for a particular page/site, the default is [i forget the exact name
of the button, but it would be either "Save" or "Yes" or "OK"].

the highlight is an important visual cue to let users know whether they can
perform the action using the keyboard or if they have to use the mouse.  if
there is a default, but it is not highlighted, the user won't know what will
happen if they hit Return.

Reproducible: Always

Steps to Reproduce:
1. open a message and type some text, and close it -- that brings up one dialog

or

2. open a message, type some text, address it to someone for whom mozilla
doesn't know if they should be getting text or html or both, and hit send --
this brings up another dialog

or

3. go to a site with a user name & password entry.  put in your info & submit
it.  if you haven't already set your password preferences for the site, the
other dialog i mentioned should appear.

Actual Results:  
dialogs without highlighted defaults appeared

Expected Results:  
presented dialogs with highlighted defaults

Comment 1

15 years ago
Well, bug 203734 is about showing the fuzzy focus ring around buttons, but I
can't find a bug about displaying the default button in the pulsating blue color
as in other OS X apps.
Assignee: sspitzer → jaggernaut
Blocks: 73812
Severity: minor → normal
Status: UNCONFIRMED → NEW
Component: Mail Window Front End → XP Toolkit/Widgets
Ever confirmed: true
Keywords: classic
Product: MailNews → Browser
QA Contact: esther → jrgm
Summary: dialogs do not have the default button appropriately highlighted → Display default dialog buttons in pulsating blue color
added trivial test URL:  javascript:alert('foo');
note: same problem with Pinstripe theme (classic keyword currently set)

Comment 3

15 years ago
Pinstripe is a third-party theme based on Classic.

Comment 4

14 years ago
This bug is easily reproducible in Jaguar (MacOS 10.2), but seems to go away
when running Firebird on Panther (MacOS 10.3). 

Comment 5

12 years ago
*** Bug 326692 has been marked as a duplicate of this bug. ***

Comment 6

12 years ago
Changing component since this probably should be fixed in the widget code...
Assignee: jag → joshmoz
Component: XP Toolkit/Widgets → Widget: Mac
QA Contact: jrgmorrison → mac

Comment 7

12 years ago
*** Bug 236958 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Keywords: classic
Severity: normal → minor
Component: Widget: Mac → Widget: Cocoa
Keywords: polish
QA Contact: mac → cocoa

Updated

10 years ago
Assignee: joshmoz → nobody
(Assignee)

Comment 8

9 years ago
Created attachment 356555 [details] [diff] [review]
fix v1

There's no documented way of drawing default buttons with NSButtonCell, but we can do it with HITheme.

This patch is on top of the patch in bug 468507.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Attachment #356555 - Flags: superreview?(roc)
Attachment #356555 - Flags: review?(joshmoz)
(Assignee)

Updated

9 years ago
Attachment #356555 - Flags: review?(enndeakin)
(Assignee)

Comment 9

9 years ago
Comment on attachment 356555 [details] [diff] [review]
fix v1

The "button-periodic-redraw" binding is very similar to the undetermined progressbar one:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/progressmeter.xml?mark=117-150#117
Attachment #356555 - Flags: superreview?(roc) → superreview+

Comment 10

9 years ago
Comment on attachment 356555 [details] [diff] [review]
fix v1

>+              var width = hbox.boxObject.width;

We should probably be avoiding using the boxObject and use getClientBoundingRect or something instead. No big deal though.

>+button[default="true"] {
>+  -moz-binding: url("chrome://global/content/bindings/button.xml#button-periodic-redraw");

I'm not familiar enough with css cascading, but does this work is someone had set <button type="menu" default="true"/> ? Not useful, but the menu binding should be used not the default=true one.
Attachment #356555 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 11

9 years ago
(In reply to comment #10)
> >+button[default="true"] {
> >+  -moz-binding: url("chrome://global/content/bindings/button.xml#button-periodic-redraw");
> 
> I'm not familiar enough with css cascading, but does this work is someone had
> set <button type="menu" default="true"/> ? Not useful, but the menu binding
> should be used not the default=true one.

Hm, global.css is applied after xul.css, so right now the default=true binding overrides the menu binding. I can see three different ways to fix this:
1. Move the default=true binding to xul.css (ifdefed) above the other button
   bindings
2. Use button[default="true"]:not([type="menu"):not([type="repeat"])... as
   selector
3. Set !important on the other bindings in xul.css

I prefer 1, what do you think?

Comment 12

9 years ago
1 is better yes, assuming its in a mac-only block.
(Assignee)

Comment 13

9 years ago
Created attachment 357328 [details] [diff] [review]
v2
Attachment #356555 - Attachment is obsolete: true
Attachment #357328 - Flags: review?(joshmoz)
Attachment #356555 - Flags: review?(joshmoz)

Updated

9 years ago
Attachment #357328 - Flags: review?(joshmoz) → review+
(Assignee)

Comment 14

9 years ago
http://hg.mozilla.org/mozilla-central/rev/d1555870e786
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Hardware: PowerPC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 15

9 years ago
When I checked in this patch, the mochichrome test for bug 398289 started to fail because it was comparing screenshots that also contained a default button. Since these snapshots were made at different times, the default button looked different in the snapshots.
I fixed the failure by pushing the default button out of the snapshotted area.
http://hg.mozilla.org/mozilla-central/rev/a17f34b6f31b
(In reply to comment #11)
> Hm, global.css is applied after xul.css, so right now the default=true binding
> overrides the menu binding. I can see three different ways to fix this:
> 1. Move the default=true binding to xul.css (ifdefed) above the other button
>    bindings
> 2. Use button[default="true"]:not([type="menu"):not([type="repeat"])... as
>    selector
> 3. Set !important on the other bindings in xul.css
> 
> I prefer 1, what do you think?
But then 3rd party themes waste CPU cycles animating an inanimate button...
(Assignee)

Updated

9 years ago
Attachment #357328 - Flags: approval1.9.1?
(Assignee)

Comment 17

9 years ago
Comment on attachment 357328 [details] [diff] [review]
v2

nice polish, has baked for 2 months
(Assignee)

Comment 18

9 years ago
(In reply to comment #16)
> But then 3rd party themes waste CPU cycles animating an inanimate button...

The CPU cycles will only be wasted on changing the step attribute, not on repainting it. The repaint is caused by nsNativeThemeCocoa::WidgetStateChanged which doesn't apply if the button isn't native-themed.

But you're right, it would be nice if we didn't do that. Which of my suggestions in comment 11 do you prefer? Or do you have a different idea?
(In reply to comment #18)
> (In reply to comment #16)
> > But then 3rd party themes waste CPU cycles animating an inanimate button...
> The CPU cycles will only be wasted on changing the step attribute, not on
> repainting it. The repaint is caused by nsNativeThemeCocoa::WidgetStateChanged
> which doesn't apply if the button isn't native-themed.
OK, so although I don't know offhand what the penalty for updating an unused attribute is, I guess it can't be that bad.

> But you're right, it would be nice if we didn't do that. Which of my
> suggestions in comment 11 do you prefer? Or do you have a different idea?
I'm tempted to suggest 2) but just using :not([type])
Comment on attachment 357328 [details] [diff] [review]
v2

Need a test here.  Re-request approval once we have this addressed.
Attachment #357328 - Flags: approval1.9.1? → approval1.9.1-
You need to log in before you can comment on or make changes to this bug.