Closed
Bug 84308
Opened 23 years ago
Closed 22 years ago
no cycling between buttons using arrow keys
Categories
(Core :: XUL, enhancement, P4)
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: william.cook, Assigned: aaronlev)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file, 7 obsolete files)
7.29 KB,
patch
|
samir_bugzilla
:
review+
alecf
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•23 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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...
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.]
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.
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.
Comment 9•23 years ago
|
||
Are there any plans to actually check these changes in?
Comment 10•23 years ago
|
||
Yo. Ben/Blake. Comments?
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)
Comment 12•23 years ago
|
||
False alarm. Build issues. This patch does work on the current build and does fix the problem.
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → Future
Comment 13•23 years ago
|
||
Is this every going to be checked in?
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?
Comment 15•23 years ago
|
||
Monthly question if this bug is ever going to go into Mozilla.
Comment 16•22 years ago
|
||
Continuing to ask my monthly question as to why this hasn't been checked in.
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.
Updated•22 years ago
|
Attachment #37356 -
Attachment is obsolete: true
Assignee | ||
Comment 19•22 years ago
|
||
Javier, have you takane into account blake's comments about <controlgroup> ?
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.
Assignee | ||
Comment 21•22 years ago
|
||
Jan and Dean, could you take a look at this for r=?
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.
Comment 23•22 years ago
|
||
*** Bug 118288 has been marked as a duplicate of this bug. ***
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.
Comment 25•22 years ago
|
||
We've checked around, and it appears that the selectdialog isn't used anywhere.
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.
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?
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?
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()?
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).
Comment 31•22 years ago
|
||
Moved code to commonDialogOnLoad() and cleaned up spacing.
Attachment #89583 -
Attachment is obsolete: true
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
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 :)
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
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.
Comment 36•22 years ago
|
||
This patch works for the multiple ID dialog too.
Comment 37•22 years ago
|
||
Comment on attachment 91510 [details] [diff] [review] patch with changes from comments r=me
Attachment #91510 -
Flags: review+
Assignee | ||
Comment 38•22 years ago
|
||
I think this should be done in button.xml, so that any dialogs with buttons get this behavior.
Assignee: ben → aaronl
Status: ASSIGNED → NEW
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=
Attachment #91510 -
Attachment is obsolete: true
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.
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.
Assignee | ||
Updated•22 years ago
|
Attachment #101565 -
Attachment is obsolete: true
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.
Attachment #101720 -
Flags: review+
Assignee | ||
Comment 43•22 years ago
|
||
I'm going to redo this so that it doesn't walk the whole tree every time. Sorry.
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*.
Status: NEW → ASSIGNED
Priority: -- → P4
Assignee | ||
Comment 45•22 years ago
|
||
Seeking r=samir
Attachment #101720 -
Attachment is obsolete: true
Assignee | ||
Comment 46•22 years ago
|
||
Still seeking r=
Attachment #104426 -
Attachment is obsolete: true
Comment 47•22 years ago
|
||
Comment on attachment 104435 [details] [diff] [review] Use window.document.commandDispatcher instead of window.top.document.commandDispatcher r=sgehani
Attachment #104435 -
Flags: review+
Comment 48•22 years ago
|
||
Comment on attachment 104435 [details] [diff] [review] Use window.document.commandDispatcher instead of window.top.document.commandDispatcher sr=alecf
Attachment #104435 -
Flags: superreview+
Comment 49•22 years ago
|
||
in the .properties file: \ No newline at end of file maybe you should fix that
Assignee | ||
Comment 50•22 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
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? :-)
Assignee | ||
Comment 52•22 years ago
|
||
*** Bug 178867 has been marked as a duplicate of this bug. ***
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?
Assignee | ||
Comment 54•22 years ago
|
||
*** Bug 187527 has been marked as a duplicate of this bug. ***
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in
before you can comment on or make changes to this bug.
Description
•