127892 - dialogKeys keyset has incorrect behaviour

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Brian King [:kinger]]

The scenario is this:
1. Open a dialog that uses this keyset
2. Tab between the buttons [OK, Cancel ...(or equivalents)]
3. Settle on the Cancel button (or equivalent)
4. Hit Return/Enter

Expected behaviour: The Cancel operation is carried out
Actual behaviour: The OK behaviour is carried out.

This can be seen in many places including:
- Renaming a folder in Mail
- Save Page dialog in Composer (closing the window with changes pending)

It is marked as windows but the code is the same all platforms as far as I can see.
The flaw is in the 'oncommand' code of the keys contained the keyset.
<key keycode="VK_ENTER" oncommand="if (!document.getElementById('ok').disabled)
doOKButton();"/>
<key keycode="VK_RETURN" oncommand="if (!document.getElementById('ok').disabled)
doOKButton();"/>
This code is not sufficient in that all it does is checks if the OK button is
disabled or not, and carries out the handler if it is not. In most cases, the OK
button will always be enabled. Also, it does not pay attention to the focus
context, i.e what button the user wishes to activate when ENTER is pressed, so
any behaviour other than OK will not be carried out.

Possible solutions:
1. Remove use of dialogKeys in all dialogs. I'm not sure how the window handlers
for ondialogaccept/ondialogcancel, etc. behave, but using them may bypass the
need for this keyset. Dialogs that use them behave correctly.
2. Clean up the code. Add a function to dialogOverlay.js that "does the right
thing". This is then called from the keys (patch to follow).

Comment 1

[Brian King [:kinger]]

Attachment: proposed patch for ENTER behaviour with dialogKeys keyset

This patch does the following:
- If a button has the focus, execute the function hooked up to it
- If no button has focus, do the default button (if not disabled)
- If no button has focus, and the default button is disabled, do nothing

Comment 2

[Kathleen :Brade]

do we need to fix other platforms?
this bug sounds familiar; are there duplicates out there?

Comment 3

[Aaron Leventhal]

Brian, any chance you can fix 84308 as well?
(Should be able to arrow between buttons in dialog boxes)

Comment 4

[timeless]

Comment on attachment 71520 [details] [diff] [review]
proposed patch for ENTER behaviour with dialogKeys keyset

this patch can't be used because only windows like os's get this behavior. (i
think we have to read bugs from mpt to see which os's are windows like wrt
this)

Comment 5

[Brian King [:kinger]]

Kathy: I could not find duplicates. There may be bugs out there for this
behaviour in individual dialogs, but this I believe is the root fo the issue.

Aaron: I'll have a look at that bug when I get a chance.

timeless: If you have any information (or know where I can get it) about the
behaviour on other platforms, I would be glad to hear it.

Comment 6

[timeless]

mpt suggested bug 63728

Comment 7

[Brian King [:kinger]]

There was some discussion about platform behaviour over in 63728, though I don't
think there was complete resolution even though it is marked VERIFIED. There is
still clearly incorrect behaviour on windows at least (see first comment in this
bug).

Adding possibly interested folks.

Comment 8

[Matthew T (active 1999-2002)]

*** This bug has been marked as a duplicate of 63728 ***

Comment 9

[Brian King [:kinger]]

Reopening due to closure of #63728. This is still an issue. We can address the
Mac issues here too or open a bug on it.

Comment 10

[Brian King [:kinger]]

Proposing this be fixed for mozilla 1.0. This is basic UI functionality.

Comment 11

[timeless]

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

Comment 12

[Jim Song]

You should also need to modify .xul file of unix/mac/os2?

Comment 13

[Brian King [:kinger]]

Yes, I will submit a new patch but most likely will leave the current behaviour
for the Mac, which should apparently always do the ok behaviour on ENTER.

Comment 14

[Brian King [:kinger]]

Attachment: Updated patch to include other platforms

Assed the new call to keys in unix and os/2 overlays. Mac issue is addressed by
ignoring it. Leave functionality as is there, as I am told ENTER should always
do ok (or equivalent) routine there.

Comment 15

[Brian King [:kinger]]

blaker, hewitt, brade - could you please have a look at this patch for review.

Comment 16

[Kathleen :Brade]

* can we add a comment above doButtonChoice() function that explains that it
isn't desirable to have this called on Macintosh (in case someone reading the
code doesn't realize that it's not used on all platforms at this time)?

* I wonder if we should check for buttons 2 and 3 before cancel (in case somehow
the "default" flag got set on more than 1 button?).  The cancel button *might*
be worse; it's really hard to say.

* I wonder if we should beep in this block (or is that a Mac-thing to do?):
+     // If no button has focus, and the default button is disabled, do nothing
+    else  {
+        return;
+    }

Comment 17

[sairuh (rarely reading bugmail)]

see bug 132720 for comparison.

Comment 18

[Brian King [:kinger]]

Bryner, nominating for 1.0, as this potentially has a more widespread effect
than bug #132720 which is Mac only. Feel free to change or add comments as you wish.

Comment 19

[Aaron Leventhal]

Attachment: In dialogKeys, don't handle Enter/Return if already on a button -- let it fall through and the normal binding for button will handle it

Much smaller patch that accomplishes the same thing.

Comment 20

[Aaron Leventhal]

Have patch, taking

Comment 21

[[e-mail]]

Comment on attachment 127271 [details] [diff] [review]
In dialogKeys, don't handle Enter/Return if already on a button -- let it fall through and the normal binding for button will handle it

Bah, why do we still have so many clients of this stuff?
Actually, I do know one reason - a <dialog> is limited in the placement of the
buttons so it's not even any good for switch profile.
I've come up with two possible alternatives:
1. A <dialogbuttons> tag which you can include anywhere you like in your
<dialog>, defaulting to the anonymous one in the <dialog>
2. Move the keyset, button labels and oncommand into dialogOverlay.xul and add
dlgtype to the buttons in platformDialogOverlay.xul so that you can overlay
platformDialogOverlay.xul if you want to be able to put buttons in a different
place in the dialog.

Comment 22

[Aaron Leventhal]

checked into 1.5b trunk