206636 - Display default dialog buttons in pulsating blue color

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[phil locke]

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

[Greg K.]

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.

Comment 2

[Bill McGonigle (not currently reading bugmail; please contact directly)]

added trivial test URL:  javascript:alert('foo');
note: same problem with Pinstripe theme (classic keyword currently set)

Comment 3

[Greg K.]

Pinstripe is a third-party theme based on Classic.

Comment 4

[Leon Out]

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

[Stefan [:stefanh]]

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

Comment 6

[Stefan [:stefanh]]

Changing component since this probably should be fixed in the widget code...

Comment 7

[Stefan [:stefanh]]

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

Comment 8

[Markus Stange [:mstange]]

Attachment: 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.

Comment 9

[Markus Stange [:mstange]]

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

Comment 10

[Neil Deakin]

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.

Comment 11

[Markus Stange [:mstange]]

(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

[Neil Deakin]

1 is better yes, assuming its in a mac-only block.

Comment 13

[Markus Stange [:mstange]]

Attachment: v2

Comment 14

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/d1555870e786

Comment 15

[Markus Stange [:mstange]]

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

Comment 16

[[e-mail]]

(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...

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 357328 [details] [diff] [review]
v2

nice polish, has baked for 2 months

Comment 18

[Markus Stange [:mstange]]

(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?

Comment 19

[[e-mail]]

(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 20

[Damon Sicore (:damons)]

Comment on attachment 357328 [details] [diff] [review]
v2

Need a test here.  Re-request approval once we have this addressed.