William Cook Reporter
	Description • 23 years ago

Common dialogs have no support for shifting the focus between buttons using the 
left and right arrow keys.  


Reproducible: Always
Steps to Reproduce:
1. Compare the functionality of dialog boxes with multiple buttons in mozilla 
with those in other applications.

Note:  Not sure if this is expected behaviour on platforms othe than windows.


Actual Results:  When a button in a set 

( eg [Save] [Don't Save] [Cancel] )

has the focus the cursor keys can't be used 
to shift the focus between buttons.

	John Morrison
	Comment 2 • 23 years ago

patch is to commonDialog.js and selectDialog.js app-specific key cycling logic.
I think this really belongs to ben, in the end.

	Peter Trudelle
	Comment 3 • 23 years ago

making it so

	Ben Goodger (use ben at mozilla dot org for email)
	Comment 4 • 23 years ago

This isn't how I'd solve this problem. 

It's probably a better idea to make a <controlgroup/> tag using XBL (look for a
bug that has 'controlgroup' in the summary) that handles this kind of navigation
amongst its child widgets automatically. 

Dialogs like this would the use controlgroup for their buttons. 

	William Cook Reporter
	Comment 5 • 23 years ago

<controlgroup/> sample uploaded to:

http://www.croczilla.com/bugs/84308/controlgroup.xul

doesn't work with radio(groups) but seems fairly happy with buttons...

	John Morrison
	Comment 6 • 23 years ago

Ooh! Sweet! I like this a lot. Nice work. Blake, have a look at this, which is 
the same topic as bug 56720 "Require a 'controlgroup' widget to handle keyboard 
navigation among adjacent widgets".

(I've set it up with the right mimetype for loading over the network at 
http://jrgm.mcom.com/bugs/84308/controlgroup.xul 

[which is a URL that is only accessible behind the firewall here, but is 
just mirroring the xul/xml/css files at http://www.croczilla.com/bugs/84308/. 
William, you just need to add the line 
  AddType application/vnd.mozilla.xul+xml .xul
to your httpd.conf file to get this to load over the network.]

	Blake Ross
	Comment 7 • 23 years ago

This is indeed really cool!  Thanks for doing it.

I talked to Ben about this a bit in the past few days, and I think we decided to
just build this functionality into the widgets themselves, and <titledbox/>.  It
doesn't seem right to require that users specify a controlgroup, and we're not
sure when the 'element' attribute would be useful.

So basically, tabbing would do what it's always done -- tab between all widgets
(across multiple titledboxes, etc.), and the arrowkeys would cycle focus between
widgets in a titledbox (with some specialized behavior -- obviously nothing
would happen for textboxes, and radioitems would also need to become selected as
they take focus, as is the case now).

However, on Windows, the arrow keys also cycle (only) between buttons that
aren't in a titledbox.  I don't think the same holds true for checkboxes, but I
can't find an example of a checkbox that isn't in a titledbox on Windows, at the
moment.  So we'd also need to build similar functionality into buttons not in
titledboxes.

I know this seems much more complex than just specifying a controlgroup as in
the example (and it's completely open to discussion...), but we're afraid that
people are going to forget to include controlgroups in some places (resulting in
inconsistent keyboard accessibility), and indeed it's something they really
shouldn't have to think about.

	William Cook Reporter
	Comment 8 • 23 years ago

I'd agree that it would be much better if keyboard navigation was built-in, 
rather than being applied/used in a haphazard manner.  I was just wondering if 
there was still a place for a controlgroup widget(or some other grouping 
mechanism) when you want to group together a set of controls for keyboard 
navigation.

	Mike Kaply [:mkaply]
	Comment 9 • 23 years ago

Are there any plans to actually check these changes in?

	John Morrison
	Comment 10 • 23 years ago

Yo. Ben/Blake. Comments?

	Mike Kaply [:mkaply]
	Comment 11 • 23 years ago

I tried this patch on a current build, and it breaks message boxes 
(they don't come up at all, they just hang)

	Mike Kaply [:mkaply]
	Comment 12 • 23 years ago

False alarm. Build issues.

This patch does work on the current build and does fix the problem.

	jhp (no longer active)
	Comment 13 • 23 years ago

Is this every going to be checked in?

	Mike Kaply [:mkaply]
	Comment 14 • 23 years ago

So basically, instead of a near term fix for the problem, we are simply never 
going to have a fix for this?

where is the bug to do the more complex stuff?

Is this going to make it for Mozilla 1.0?

	u46933
	Comment 15 • 23 years ago

Monthly question if this bug is ever going to go into Mozilla.

	Mike Kaply [:mkaply]
	Comment 16 • 22 years ago

Continuing to ask my monthly question as to why this hasn't been checked in.

	jhp (no longer active)
	Comment 17 • 22 years ago

The previous patch would not build, so I updated it (against
MOZILLA_1_0_BRANCH).  Let me know what you think.

	jhp (no longer active)
	Comment 18 • 22 years ago

fix indentation

	Aaron Leventhal Assignee
	Comment 19 • 22 years ago

Javier, have you takane into account blake's comments about <controlgroup> ?

	jhp (no longer active)
	Comment 20 • 22 years ago

Aaron, yes I looked into this, but the idea was rejected in bug 56720. 
Therefore, I decided to just update the patch.

	Aaron Leventhal Assignee
	Comment 21 • 22 years ago

Jan and Dean, could you take a look at this for r=?

	Dean Tessman
	Comment 22 • 22 years ago

Comment on attachment 89583 [details] [diff] [review]
updated patch to Mozilla 1.0 branch

1. Just a quick note to fix the spacing around parentheses.  Examples:

>+function handleKeyDown( aEvent)

handleKeyDown(aEvent)

>+  if( code == 37 || code == 38 )

if (code == 37 || code == 38)

>+    for( nextTarget = aEvent.target.nextSibling; 

for (nextTarget = ...

>+  if( nextTarget ) {

if (nextTarget)


2. >Index: selectDialog.js

Pardon my ignorance, but what's selectDialog?


This looks pretty reasonable.  I want to actually apply it once I restore my
build to working condition in order to test.

	Vidar Haarr (not reading bugmail)
	Comment 23 • 22 years ago

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

	Dean Tessman
	Comment 24 • 22 years ago

+    if( button && !button.hasAttribute("hidden") )

Should you also test for "collapsed" here?

Works pretty well for me.  If you can point me to somewhere where selectDialog
is used, fix the formatting I mentioned before, and answer my above question,
I'll r= it.

	Mike Kaply [:mkaply]
	Comment 25 • 22 years ago

We've checked around, and it appears that the selectdialog isn't used anywhere.

	Dean Tessman
	Comment 26 • 22 years ago

Looks like it may be used in embedding...

http://lxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/src/nsPromptService.cpp#53

That's fine, the code is very similar in there.

	Mike Kaply [:mkaply]
	Comment 27 • 22 years ago

Yeah, it exists in prompt service, but we've never seen anyone use it.

Know of any good way to search on someone calling promptservice->select in LXR?

	jhp (no longer active)
	Comment 28 • 22 years ago

>  +    if( button && !button.hasAttribute("hidden") )
>  Should you also test for "collapsed" here?

Well, I check for both "hidden" and "collapsed" in the handleKeyDown() function,
when I am looking for the next target button.  Do you think I should be checking
for collapsed also when adding the event listener?  Also, I am not too familiar
with this code:  is there a chance that a button could be made visible or
uncollapsed between the time the listener is added to the button and the time
the arrow keys are pressed?

	Dean Tessman
	Comment 29 • 22 years ago

That's why I asked, because you had done it in the other place.  As far as the
look of the dialog changing after it's initialized, I hope not.  That would just
be bad UI.

I just noticed that the handlers are added in setCheckbox().  Why are they done
there and not in commonDialogOnLoad()?

	William Cook Reporter
	Comment 30 • 22 years ago

the original location for the addition of event listeners was at the end of 
commonDialogOnLoad. i think the file structure has changed quite a bit over the 
past year resulting in the code migrating to setCheckbox (which used to be the 
function after commonDialogOnLoad).

	jhp (no longer active)
	Comment 31 • 22 years ago

Moved code to commonDialogOnLoad() and cleaned up spacing.

	Brian King [:kinger]
	Comment 32 • 22 years ago

> Know of any good way to search on someone calling promptservice->select in LXR?

http://lxr.mozilla.org/seamonkey/search?string=promptservice-%5C%3Eselect

	timeless
	Comment 33 • 22 years ago

mkaply, you can't. a promptservice can be retrieved/cached in one js file and used in a xul/js file somewhere else to make a call to .select(). so you'd need to use lxr to find all places that get the service, trace each variable that stores it and then search to see if any of those variables call select().



or you could try to ask for permission to add an assert "We don't think this is ever used, comment in bug 84308 if you've found a caller", but i doubt you'd get approval for that :)

	William Cook Reporter
	Comment 34 • 22 years ago

doing a search with the expression  select\( 
(http://lxr.mozilla.org/seamonkey/search?string=select%5C%28)

yields a couple of possibles:
http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/ComposerComma
nds.js#1202

http://lxr.mozilla.org/seamonkey/source/extensions/wallet/src/singsign.cpp#392

	Mike Kaply [:mkaply]
	Comment 35 • 22 years ago

I think I got it.

If you have multiple userid/passwords for one page, you get it.

So for bugzilla, I setup two IDs on the login page and I get the select dialog 
when I first hit the login page.

	jhp (no longer active)
	Comment 36 • 22 years ago

This patch works for the multiple ID dialog too.

	Dean Tessman
	Comment 37 • 22 years ago

Comment on attachment 91510 [details] [diff] [review]
patch with changes from comments

r=me

	Aaron Leventhal Assignee
	Comment 38 • 22 years ago

I think this should be done in button.xml, so that any dialogs with buttons get
this behavior.

	Aaron Leventhal Assignee
	Comment 39 • 22 years ago

While playing around with dialogs in Windows, I noticed that up arrow and left
arrow both go to the same previous item, and down/right both go to the same
next item. They don't actually go up/down/left/right. I chose to stay
consistent with that behavior.

Also, when focused on a button, a user should be able to press an accesskey
without a modifier to get to another button. In this way simply pressing "Y" or
"N" can activate "Yes" or "No". That is fixed with this patch as well.

Seeking r=/sr=

I'm having a little bit of trouble with the prefs dialog. We might need to file
a separate bug on that. The buttons aren't all in the same dom document, and
I'm having trouble getting a list of all the buttons in the dialog. This means
that the arrow keys only cycle through the buttons in the current subframe of
prefs.

If someone knows how to get a list of all the buttons in a multi-frame dialog,
please tell me - I tried using:
	var buttons;
	var frames = window.parent.frames;
	for (var count = 0; count < frames.length; count ++) {
	  buttons += frames[count].document.getElementsByTagName("button");
	}

Anyway, seeking r=/sr=

	Dean Tessman
	Comment 40 • 22 years ago

> Also, when focused on a button, a user should be able to press an accesskey
> without a modifier to get to another button.

FYI, bug 161410.

	Aaron Leventhal Assignee
	Comment 41 • 22 years ago

* Uses GetElementsByTagNameNS
* gets buttons for all subdocs in a dialog
* gets xbl buttons from <dialog>
* if a potential accesskey is pressed, test to see if the currently focused
button matches that, before iterating through the array of buttons. If it does,
activate it and return early.

I could not use concat to make 1 big array of all the buttons, because it
always created an array of arrays. I even tried Array.prototype.concat.call,
but that did the same thing.

So I used an array of arrays for the buttons and have an outerloop and
innerloop.

	Samir Gehani
	Comment 42 • 22 years ago

Comment on attachment 101720 [details] [diff] [review]
A number of improvements, after talking with Samir

r=sgehani  
One nit: |var xulns| can be made |const kXULNS|.  Review stands either way.

	Aaron Leventhal Assignee
	Comment 43 • 22 years ago

I'm going to redo this so that it doesn't walk the whole tree every time. Sorry.

	Aaron Leventhal Assignee
	Comment 44 • 22 years ago

I'm rewriting this patch so that we don't walk the whole tree of the dialog
every time.

Looking at it, it seems like up/left could just do a shift+tab, and down/right
could just do a tab.

The reason I say that, is now that I've tested dialogs in various apps, I see
that it's not completely standard. In some, this doesn't work, in others, it
works only on buttons. Sometimes it also works on checkboxes. Sometimes it
cycles, sometimes not. Generally, though, the arrow keys do *something*.

	Aaron Leventhal Assignee
	Comment 45 • 22 years ago

Seeking r=samir

	Aaron Leventhal Assignee
	Comment 46 • 22 years ago

Still seeking r=

	Samir Gehani
	Comment 47 • 22 years ago

Comment on attachment 104435 [details] [diff] [review]
Use window.document.commandDispatcher instead of window.top.document.commandDispatcher

r=sgehani

	Alec Flett
	Comment 48 • 22 years ago

Comment on attachment 104435 [details] [diff] [review]
Use window.document.commandDispatcher instead of window.top.document.commandDispatcher

sr=alecf

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 49 • 22 years ago

in the .properties file:
\ No newline at end of file

maybe you should fix that

	Aaron Leventhal Assignee
	Comment 50 • 22 years ago

checked in

	neil@parkwaycc.co.uk
	Comment 51 • 22 years ago

Comment on attachment 104435 [details] [diff] [review]
Use window.document.commandDispatcher instead of window.top.document.commandDispatcher

>+            !node.disabled && !node.collapsed && !node.hidden)

>+                !test.disabled && !test.collapsed && !test.hidden) {

Same tests twice? :-)

	Aaron Leventhal Assignee
	Comment 52 • 22 years ago

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

	neil@parkwaycc.co.uk
	Comment 53 • 22 years ago

Comment on attachment 104435 [details] [diff] [review]
Use window.document.commandDispatcher instead of window.top.document.commandDispatcher

>+        if (event.keyCode || event.charCode <= ' ')
>+          return;  // No arrow key or potential accesskey pressed

Unfortunately event.charCode is an integer,
you should be comparing it to 32 (for space) instead.

I spotted this investigating a treewalker exception;
perhaps treewalker doesn't like anonymous content?

	Aaron Leventhal Assignee
	Comment 54 • 22 years ago

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