Closed Bug 959 (Accesskey-XUL) Opened 26 years ago Closed 23 years ago

[FEATURE] Accesskey attribute not implemented yet for XUL

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: angus, Assigned: jag+mozilla)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Keywords: access, helpwanted, Whiteboard: done for html, needs to be done for xul)

Attachments

(6 files, 24 obsolete files)

(deleted), text/html
Details
(deleted), image/jpeg
Details
(deleted), text/html
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
attinasi
: review+
hewitt
: superreview+
Details | Diff | Splinter Review
(deleted), application/vnd.mozilla.xul+xml
Details
Several tricks here to implement... <input type="button" value="Test" accesskey="T"> - when the user presses alt-(accesskey), the appropriate element should get focus. If an accesskey is specified, it overrides any accesskey in the viewer application. For example, if the accesskey on a form element were "F," pressing "alt-F" would set focus on the element, not bring up the "File" menu. I assume joki owns this piece of the bug... - we should underline the first instance of the accesskey character. In the example above, the first "T" in the word "Test" would be underlined. I assume chris or kevin or kipp or someone owns this piece of the bug...
Status: NEW → ASSIGNED
Elements that utilize attribute ACCESSKEY and suggested actions: A: Should select or activate link AREA: Should select or activate hotspot on imagemap BUTTON: Should scroll to or activate button INPUT: Depends on type- (See below list) TEXT and PASSWORD: Should scroll automatically to INPUT box CHECKBOX and RADIO: Should select ofractivate/check SUBMIT, IMAGE and RESET: Should scroll to or select button LABEL: Should scroll to/select label LEGEND: Should scroll to/select legend TEXTAREA: Should scroll to/select TEXTAREA
Setting all current Open/Normal to M4.
per leger, assigning QA contacts to all open bugs without QA contacts according to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser
QA Contact: 4015 → 4137
QA contact re-assigned to INPUT element owner.
*** Bug 3422 has been marked as a duplicate of this bug. ***
Target Milestone: M4 → M5
This is not getting done by M4. Moving to M5.
Target Milestone: M5 → M6
OS: Windows 95 → All
In the case of <a accesskey="a" href="http://www.mozilla.org">mozilla</a> Please implement this by ACTIVATING the link (loading the page) when a user presses a key, rather than giving the link focus (as IE5 does).
Moving out to M7
Target Milestone: M7 → M8
Moving to M8
Target Milestone: M8 → M9
No time for M8
Whiteboard: [MAKINGTEST] Antti.Nayha@oulu.fi
Target Milestone: M9 → M11
No dependencies yelling for this so its lower priority. Moving to M11.
Is this important for beta? Hyatt, will the menu key listener interfer with this?
Speaking for QA (and QA only) this is unimportant for beta. angus?
Moving multiple bugs to m12
Moving to m13 because Joki seems to be distracted.
Whiteboard: [MAKINGTEST] Antti.Nayha@oulu.fi
Moving off bugs that won't make M13
Target Milestone: M13 → M14
Summary: accesskey attribute not implemented yet → [FEATURE] accesskey attribute not implemented yet
Target Milestone: M14 → M16
Moving M16.
*** Bug 7559 has been marked as a duplicate of this bug. ***
A couple of suggestions: * IE's behavior for accesskeys -- selecting a link or button, rather than activating it -- is very sensible. It prevents a malicious Web author from, for example, doing this <a href="thesamepage.html" accesskey="f"><font color="white">.</font></a> <a href="thesamepage.html" accesskey="e"><font color="white">.</font></a> <a href="thesamepage.html" accesskey="v"><font color="white">.</font></a> <a href="thesamepage.html" accesskey="g"><font color="white">.</font></a> <a href="thesamepage.html" accesskey="b"><font color="white">.</font></a> <a href="thesamepage.html" accesskey="t"><font color="white">.</font></a> <a href="thesamepage.html" accesskey="h"><font color="white">.</font></a> which would have the effect of completely, and (from the user's point of view) inexplicably, disabling keyboard access to the Navigator menus. (Using Alt+ arrow keys would still be possible, but the user probably wouldn't realize that.) So, hitting an access key should select a link or button, not activate it. And pressing Shift+Alt+accesskey should override any accesskeys in the content and go to the menus, so that there is always a way to access a menu immediately with the keyboard even where content uses the same access key as the menu. * When an accesskey is used, the content should always scroll so that the widget being manipulated is visible, before actually changing the widget state. If the widget wasn't visible, maybe even a short delay (~0.5 s) between the scrolling and the change in state would be nice, so that the user gets a chance to see what's happening to the widget (preventing the `aargh! what did I just do?' problem). * On Mac, the modifier key for an accesskey should be Ctrl, not Option(/Alt). Unlike Windows and Unix, MacOS uses the Option(/Alt) key for entering non-ASCII characters, so unlike on Windows and Unix, Option shouldn't really be used for anything else. Some Mac apps, such as Photoshop and BBEdit, use the equivalent of accesskeys in their dialogs. They use the Command key to get to them (e.g. Command+D = `Don't Save'). So using Ctrl would be inconsistent with that. But Mozilla couldn't really be consistent and use Command, because in the world outside of dialogs, Mozilla (like other apps) uses Command for a whole lot of keyboard shortcuts; and the usability nightmare from accesskeys conflicting with these would be worse than the slight usability problem from being inconsistent with other MacOS apps. That's why I recommend Ctrl, which is little-used for anything else on MacOS at the moment. It would even be vaguely in line with the Mac HIGs, which say Ctrl should be used for `shortcut key sequences that the user defines using a macro-key facility'. Replace `user' with `page author', and this is pretty much what Mozilla would be doing. We now return you to our regularly scheduled `Moving out to M[n+1]' comments ...
Hardware: PC → All
*** Bug 33318 has been marked as a duplicate of this bug. ***
Is it a security problem for keyboard-only users that pages can play with the alt keys, or will they be able to lift alt before pressing the letter to access the menu, on all platforms?
Nominating for nsbeta2 and adding the 'html4' keyword. Adding dependency on this bug to bug 7954 (HTML 4.0 compliance meta bug).
Blocks: html4.01
Keywords: html4, nsbeta2
Per ekrock mail: "Sigh. This falls into the category of "important features we really better get in by 5/16. Since this enables accessibility by the disabled, we could increase our exposure to a lawsuit if we don't implement this. So it's probably nsbeta2+[5/16-]." Putting on [nsbeta2+][5/16] radar.
Whiteboard: [nsbeta2+][5/16]
changing QA contact
QA Contact: cpratt → sairuh
*** Bug 33318 has been marked as a duplicate of this bug. ***
Okay, this is 90% fixed. Accesskey is in and works for everything except links. Working on that last bit.
Rods has generously volunteered to finish this bug, which more or less means adding Register and and UnRegister calls for accesskeys on links.
Assignee: joki → rods
Status: ASSIGNED → NEW
Attached file Brief acccesskey test case (deleted) —
Accesskeys now work for all form controls which is the most important part of this bug. The only part that does not work is acess keys on links, which isn't really very important. I probably won't get this fixed today. I suggest this now be downgraded to nsbeta2-
Status: NEW → ASSIGNED
Whiteboard: [nsbeta2+][5/16] → [nsbeta2+][5/16] see my comment below (rods)
now regs and unregs in SetDocument - fixed
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
over to janc 9or jrgm?) to verify.
QA Contact: sairuh → janc
This must work for XUL as well. Should I give myself a separate bug for XUL, or do we want to use this bug?
i am now taking this bug for xul. it's done for html, and now it needs to be done for xul.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [nsbeta2+][5/16] see my comment below (rods) → [nsbeta2+]
taking bug...
Assignee: rods → hyatt
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Whiteboard: [nsbeta2+] → [nsbeta2+] done for html, needs to be done for xul
removing nsbeta2, since this bug is now about xul and should probably be re-evaluated
Whiteboard: [nsbeta2+] done for html, needs to be done for xul → done for html, needs to be done for xul
moving to m18.
Target Milestone: M16 → M18
Putting on [nsbeta2-] radar.
Summary: [FEATURE] accesskey attribute not implemented yet → [FEATURE]Accesskey attribute not implemented yet
Whiteboard: done for html, needs to be done for xul → [nsbeta-]done for html, needs to be done for xul
Whiteboard: [nsbeta-]done for html, needs to be done for xul → [nsbeta2-]done for html, needs to be done for xul
Target Milestone: M18 → M20
There's still a few problems with the current implementation: 1. The access keys specified by the HTML document don't disable those of the application. For instance, while viewing the attached test case on Linux, activating the first radio button by pressing Alt+a also activates browser's Select All function. The same goes for Alt+l, which activates Open Web Location. 2. Access keys on links should just give the link focus, instead of following the link immediately. See Matthew's earlier comment. To clarify my point, let's take another example: A long page has a navigation link at the bottom: <a href="first.html" accesskey="f"><strong>F</strong>irst page</a> This link is not visible without scrolling, so the user has no way of knowing that 'f' access key is used by the page. Now, immediately after arriving on the page, the user presses Alt+f to bring down the File menu. Instead (s)he's taken to the first page of the site, which makes him/her switch back to IE in frustration. :-) 3. The first occurrence of the access key in the link text, button text or label should be highlighted. Maybe a separate bug should be filed for this issue?
> 3. The first occurrence of the access key in the link text, button text or > label should be highlighted. Maybe a separate bug should be filed for this > issue? Yes. There needs to be a nice algorithm which takes a text string and an accelerator character, and returns the most appropriate character to underline to indicate that accelerator (with a value of 0 if the accelerator is not in the string at all). The most appropriate character isn't necessarily the first occurrence of that character. As much as you could determine it with an algorithm, it is, in order: * the first occurrence of that character at the start of a word; * the first occurrence of that character at the end of a word; * the first occurrence of that character at all.
#1 - bug 40071 #3 - How do you decide when to turn that algorithm off (for example, if I put bold/underline on occurence of the accesskey letter that makes the most sense to me)?
#2 - isn't it generally worse to accidentally submit a form than to accidentally follow a link? (should accesskey be disabled in e-mail? comment in bug 28327.)
Sorry if I sound argumentative. I agree with you guys :)
> #2 - isn't it generally worse to accidentally submit a form than to > accidentally follow a link? Definitely. I think access keys shouldn't directly activate any elements - only give them focus. Most importantly, they should never - follow links (<a>, <area>) - "push" buttons (<button>, <input type="submit|reset|image|button">) It's true that directly activating radio buttons, checkboxes and file dialogs (<input type="file">) isn't _quite_ as disastrous from a usability viewpoint as directly activating links and buttons. I still think it may seriously confuse users in some situations. Also, for the sake of consistency, identical behaviour between all accesskey-enabled elements might be the best way to go. (I guess I only mentioned links in my last comment because the link was the most obviously problematic element included in the test case. Silly me.)
Isn't this long done?
*** Bug 43772 has been marked as a duplicate of this bug. ***
*** Bug 43772 has been marked as a duplicate of this bug. ***
I think we'll have to live without this in current release, -> Future
Target Milestone: M20 → Future
Blocks: uaag
Mass update: changing qacontact to ckritzer@netscape.com
QA Contact: janc → ckritzer
Nominating for nsbeta3.
Keywords: nsbeta3
nsbeta3-. This bug report has morphed too much, and had so many lists of suggestions and problems added to it that I don't know what it is about any more. Please file separate bugs for any issues that you care about, and only nominate major or critical severity parts for nsbeta3, since we already have too many nasty bugs to fix in the time remaining.
Whiteboard: [nsbeta2-]done for html, needs to be done for xul → [nsbeta2-][nsbeta3-] done for html, needs to be done for xul
Keywords: access
Anyone notice that if ACCESSKEY is set to "C" on a Mac, the command can be confusing to the user? I.E. if Command + C is pressed, will it Copy what's highlighted or follow the ACCESSKEY link???
Don't know what the std says, but in practice we're planning to mimic IE4 behavior on this, the accesskey will act on the event, the menu will ignore it see bug 40071.
Upon further investigation, it seems that I was right. Command+"A" (Select All on Macs) highlights the button, but also selects all. Same goes for Command + "C" doing a copy and also selecting the "C" button. My recommendation would be to change this ACCESSKEY to default to the "Option" key instead of the "Command" key-or Control. How about a toggle switch in Preferences? Just tossing out ideas. Working on build 2000-09-04-08, MacOS 9.0.4, on file: http://slip/projects/marvin/html/button_accesskey.html
Hey Dan. I suggest you file the above as a separate bug report, if it should be done. Otherwise, it will get no attention.
*** Bug 49968 has been marked as a duplicate of this bug. ***
Sorry about that. After being slapped upside the head, reopening bug and closing 959. The difference is that 959 was a "Not Implemented Yet" bug, whereas this is a "Implemented But Not Really Working" bug. Any other bugs stemming from that should be opened separately-it was getting out of control. Moving Mac ACCESSSKEY issue to http://bugzilla.mozilla.org/show_bug.cgi?id=51940. I also recommend we start tearing this bug into smaller more manageable pieces...no? There is waaaay too much going on here, I think but I'd like to hear some input about this idea...
This bug was FIXED on 5/16, but hyatt resurrected it to deal with the XUL case. However, subsequent to that, a number of HTML issues were noted. Yes, those should be filed as separate bugs. I will file a bug on hyatt specifically for any XUL issues. Then this bug can be closed out (returned to FIXED for those issues noted up to 5/16).
Blocks: 51940
Updating QA Contact.
QA Contact: ckritzer → lorca
Updating Summary to reflect Status since it seems fairly relevant.
Summary: [FEATURE]Accesskey attribute not implemented yet → [FEATURE]Accesskey attribute not implemented yet for XUL
Blocks: robin's
Can someone with the permissions remove the 7954 block and html4 keyword? The attached HTML testcase works fine-I think all HTML accesskey issues have moved to other bugs.
Yep, you're right. Removing those.
No longer blocks: html4.01
Keywords: html4
Blocks: 18575
Nominating for the new nsbeta1 and removing old [nsbeta2-][nsbeta3-].
Keywords: nsbeta1
Whiteboard: [nsbeta2-][nsbeta3-] done for html, needs to be done for xul → done for html, needs to be done for xul
While we're using Control to activate access keys in HTML (and we don't really have any other choice), Mac OS convention is to use the Command key to activate access keys in chrome -- e.g. many users are used to pressing Command+D for `Don't Save' in `Save before closing?' alerts. If we follow this convention, then XP we will need to avoid using the accesskeys A, C, Q, V, W, X, Y, and Z in chrome where possible, since Command+ those keys does editing/closing operations. Or we could just use Control for chrome like we do for HTML, and not have to worry about this.
I agree with mpt's comments above-in that we should be careful to not to use the Command + [key] combinations common in editing. Despite that, my vote is for using Command for the chrome commands though-I would never expect to use Control in a Mac app basically-especially being that Command is the norm for UI controls. Oh, and underlining the accesskey in chrome would certainly go a faaaar way for me as well (in a good way).
I'm not sure this is the right bug, but i'm tired... Instead of worrying about both html and chrome bindings, let's worry about one set at a time. Basically, either you're using the browser, or you're using the content. When users run mozilla, it defaults to binding chrome keybindings. There is a single key trigger (pick a key, I like sysrq or any fkey or ` or whatever aaron and mpt can agree on) as well as a menu trigger. Most likely, there is a notification/toggle in the bar that has security and online status. A tooltip for this explains what the current state is and has a help link/button. When the user wants to switch to html bindings, the user presses the key, or clicks the toggle or selects the menu item. The indicator switches to indicate that commands will go to the document [I think a special border or something should be given to the active region]. While html keys are bound, mozilla can based on a user stylesheet more actively hint at their existence [possibly inlining hints]. Users can switch back to chrome using the same toggles. I am under the impression that only letters and possibly a few other keys can be bound by html. So I would suggest that back and forward (accel+left, accel+right), form submit (accel+enter), and possibly help (f1?) retain their bindings. Activation of this feature should display a dialog explaining what is happening, with a check box to allow the user to not be warned again.
*** Bug 64730 has been marked as a duplicate of this bug. ***
REMOVING nsbeta2/3 and replacing with nsbeta1 KW. Also clearing Status Whiteboard of nsbeta2/3 result. Accessibility is important.
Keywords: nsbeta2, nsbeta3
Reassigning QA Contact for all open and unverified bugs previously under Lorca's care to Gerardo as per phone conversation this morning.
QA Contact: lorca → gerardok
should the milestone still be future here? clearing it for re-evaluation --afaik, this ought to be fixed for basic accessibility in 6.5... thx!
Keywords: helpwanted
Target Milestone: Future → ---
I agree, this is fairly important for accessibility (although not embedding, really). cc'ing trudelle.
[Really CCing Trudelle]
Summary: [FEATURE]Accesskey attribute not implemented yet for XUL → [FEATURE] Accesskey attribute not implemented yet for XUL
*** Bug 68769 has been marked as a duplicate of this bug. ***
Blocks: 70216
Nominating for mozilla0.9 (this blocks a whole lot of bugs).
Keywords: mozilla0.9
->mozilla1.0, is this absolutely required for accessibility in next rev of the apps?
Target Milestone: --- → mozilla1.0
As long as every GUI element is available by tabbing, I wouldn't regard lack of accesskeys as an accessibility blocker -- just annoying, especially if you are keyboard-bound and {the element which has focus} and {the element you want to get to} are far away from each other.
Blocks: 24873
Blocks: 62703
The test case is attached in the url field. The accesskey works with mouse but are not accessible through the keyboard. Pressing the key opens the menu keys (e.g. Pressign Alt-B, opens Bookmark).
Attached image The image file (deleted) —
Blocks: 78153
Blocks: 78261
Blocks: 68841
*** Bug 70216 has been marked as a duplicate of this bug. ***
QA contact updated
QA Contact: gerardok → madhur
*** Bug 43756 has been marked as a duplicate of this bug. ***
No longer blocks: robin's
I have a preliminary patch! It does checkboxes, focuses text fields, and almost does radio buttons. ;) Push buttons don't work at all at the moment... I'll investigate more. Assigning to me. I'll assign it to someone else if I can't complete it.
Assignee: hyatt → aegis
Blocks: robin's
Status: ASSIGNED → NEW
Attached patch preliminary (obsolete) (deleted) — Splinter Review
Oh yeah, forgot to mention... There are some changes to the find search dialog in there. I used those for testing.
Status: NEW → ASSIGNED
Ok, here are some comments. First of all, thanks for tackling this. It's a much needed XUL1.0 feature. My main concern is over the way the accesskeys are hooked up. Namely every element ever inserted into a document (i.e., all of them) has its accesskey attribute checked. Two problems occur to me with the approach of doing this from SetDocument. The first I just mentioned, namely that you waste time doing an extra GetAttribute string fetch for all XUL elements. The second is a menu issue... menus shouldn't even be searched, since their accesskeys are already handled down a completely different code path. I wouldn't even crawl into menus or menubars or menuitems. The hookup should be kept in the front end. I would do the hookup in the nsTextBox code. Right now your code will hook up access keys on invisible elements (elts with display: none). In order to work, an accesskey has to be visible (and therefore has to have a frame), so you can simply do the hookup in the frame in the same code that already checks accesskeys. That means the content model is not the place to do this hookup, since you don't know if you even have a front end frame object built for that content. Similarly, in the Destroy method of nsTextBox, you can do the unhooking if it has an accesskey. You'll want to avoid doing all of this hooking/unhooking for text frames that are inside menu items and menus though, since they have their own code for accesskey. It's also easy to handle the dynamic case by using the frame code. Your patch does not handle someone dynamically adding an accesskey to an element (or changing/removing the accesskey attribute). The frame code already has to worry about that case, and if you patch the method that builds up the access string, you'll handle this case without writing extra code.
Hyatt: Thanks for the comments. However, dynamic access keys don't work in HTML at the moment either (attaching a testcase). Would you prefer that be a new bug or should it go into this (already cluttered) one?
Attached file dynamic HTML accesskey testcase (deleted) —
Filed bug 88151 for modifying HTML accesskeys via the DOM.
Blocks: 89143
I don't think this bug blocks bug 41368 any longer-at least not since the switchover from HTML to XUL.
No longer blocks: robin's
Attached patch second attempt (obsolete) (deleted) — Splinter Review
Okay, this second patch goes along with hyatt's guidelines. Things that don't work: for some reason, when an accesskey is pressed, buttons simply get focus and don't fire. If I set the case sensitive checkbox in the 'find in this page' dialog's accesskey to be 'C', it doesn't work.
Move the nsBoxFrame code into nsButtonBoxFrame.cpp. This will cover radio buttons, checkboxes, and buttons, and it will ensure that random normal boxes don't waste time checking for an accesskey attribute. It will also prevent a slew of extra hookups (e.g., menus and menuitems) from happening when they don't need to.
Depends on: 90221
Attached patch third patch (obsolete) (deleted) — Splinter Review
That last patch works with checkboxes and pushbuttons. Radio buttons are kinda weird at the moment. Dynamically changing the accesskey via the DOM works too.
Two comments. (1) Don't call the nsBoxFrame::AttributeChanged method from nsButtonBox in the case where the accesskey attribute changes. nsBoxFrame won't do anything with that attr, so avoid the extra call. (2) Don't QI to nsIDOMXULElement to test the "type" of an element. jst fairly recently added a method for querying the type, and you can ask if an element is XUL much more efficiently by using that method. One more iteration should do it. Great job!
Blocks: 90318
Depends on: 90406
Attached patch fourth patch (obsolete) (deleted) — Splinter Review
There is some funkiness going on with radiobuttons still... I did some tracing and the XBL behaves properly in setting |checked| on the radio button... After the setter for selectedItem is called, something unsets |checked| (in some cases). Hopefully some event/XBL guru can tell me what's happening. More investigation tomorrow... hyatt: Thanks for the feedback. It's been really helpful in learning the codebase. This bug is hopefully coming to close...
Attached patch fifth patch (obsolete) (deleted) — Splinter Review
Okay, the radio button problem was in the event state manager. I accidentally had a little bit of the HTML accesskey code path executing on the XUL side. Fifth and hopefully final patch is here, can we get r= and sr=? (bug 90221 and bug 90406 need to be checked in first)
Attached patch sixth patch (obsolete) (deleted) — Splinter Review
Okay, I like this patch the best. It oncurs a little overhead (both processing and memory) for each XUL element, but it's much more flexible and straightforward. in nsXULElement, you can specify which elements support accesskeys. In the event state manager, you can define the behavior for each type of element. This patch works (perfectly?) with textboxes, checkboxes, radio buttons, and push buttons. I've included the fixes for bug 90221 and bug 90406. reviewers?
Blocks: 91050
Keywords: fcc508
We can get this in for 0.9.3.
Target Milestone: mozilla1.0 → mozilla0.9.3
Don't do this: + nsXULElement* element = NS_STATIC_CAST(nsXULElement*, raw_content);
I do not like having the accesskey code in nsBoxFrame. I would like that moved to nsButtonBoxFrame. Is there a reason why the code can't be moved that I am missing? You used the IsContentOfType. That's good, but as waterson said, I wasn't advocating eliminating the QueryInterface all together. I just meant that your initial determination of type should use IsContentOfType. Once you know you're a XUL elt, it's ok to QueryInterface to a XUL element (since you know it will succeed). We're only looking to avoid the cost of missed QIs here, which is why I requested the use of IsContentOfType to ensure you didn't have HTML elts failing QIs to a XUL elt. What is the code in XBL doing? It looks like you've added a redundant implementation into <radio>. <radio> should be handling selection already. Why was this code added?
There are other non-button elements that need to have accesskey support. Textboxes right now, and possibly more later. One way to do it would be putting the same code in every frame class that needed accesskey support, but this would be a pain to maintain. I'll fix the QI stuff. The odd radio button behavior is addressed by bug 90406. Basically, calling nsXULElement::Click() on a <radio> element had very bizarre behavior. I made a testcase with two radio buttons and two pushbuttons. The pushbuttons would call click() on their respective radio buttons. The radio group kept getting into situations where either one button would be selected or both would be. In today's commercial build, calling click() on a radio element doesn't do anything at all. Either way, that patch to XBL fixes the problem. There are probably a hundred other ways to fix it, and if anyone knows a better way, feel free to comment.
Attached patch seventh patch (obsolete) (deleted) — Splinter Review
pushing to 0.9.4 for aegis. won't make the 0.9.3 train unfortunately...
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Oops, ignore the change to checkbox.xml.
Could you explain why <textbox> needs to support accesskeys? As far as I can tell, it doesn't need accesskey support. It would be a label associated with the textbox that would require accesskey support, and that would need to listen for the accesskey and activate the elt specified by its "for" attribute when the accesskey got pressed. As far as I can tell (just ticking through the widgets that use access keys) only button box frames and text frames with "for" attributes need to support accesskeys.
<hyatt> heck, a separate bug could cover the "for" case. <hyatt> the "for" case hasn't been addressed by any patches. <hyatt> i want the code in boxframe moved to buttonboxframe <hyatt> right now it's in boxframe but there's a guard like <hyatt> if (i'm a checkbox, radio, textbox, or button) { do accesskey stuff } <hyatt> but textbox doesn't support accesskeys (since you have to use a label with a for attr for that case) <hyatt> and the other three are all buttonboxframes. <hyatt> so the code doesn't need to be in boxframe <hyatt> if it were moved to buttonboxframe you could get rid of the if statement completely <hyatt> and just always check for accesskeys
Attached patch eighth patch (obsolete) (deleted) — Splinter Review
Wow, cool. Finally all those underlined letters in Mozilla for Windows and X won't be just for giggles. On Mac OS, however, looks like this won't work. I don't see any `#ifdef PC' or whatever in these patches, so I'm assuming you're not aware of how chrome accesskeys (as opposed to HTML accesskeys) are dealt with on Mac. Two preliminary facts: (a) The only widgets on Mac OS which take keyboard focus are text fields and listboxes (trees and outliners). (b) the modifier key used to activate accesskeys on Mac OS is Command, which is the same key as is used for keyboard shortcuts e.g. (Copy = Command+C). So, here's a rough algorithm which is probably thoroughly inaccurate from a code point of view: 1. When Command+{something} is typed, let it be handled by other event handlers first. If it's a modal dialog, it should be taken if {something} is one of {A, C, V, X, Z, Shift+Z}. If it's a non-modal window, then just about everything will be taken probably (e.g. Command+N will open a new browser window, Command+W will close the focused window, Command+O will open a file ...). 2. If nothing else takes it, and there's an element with that accesskey, then: - if it's a label for a text field or listbox, focus it; - if it's a checkbox, toggle it, but do not change the focus from what it was; - if it's a button or radio button, activate it, but do not change the focus from what it was; - if it's a label for a radio group, activate the next radio button in the group after the one which is currently activated, but do not change the focus from what it was; - if it's anything else, ignore it. A test is to start a Composer document, enter some text, and close the window. Typing Command+D in the resulting alert should trigger the `Don't Save' button.
Attached patch patch numero neuf (obsolete) (deleted) — Splinter Review
mpt: Sorry, I don't have a Mac. saari?hyatt: The latest patch has code in nsTextBoxFrame also because text box frames don't inherit off of nsBoxFrame.
I started working on a tenth patch to address the fact that nsBoxFrame and nsTextBoxFrame could share some registration code, but it turned out that I could only factor out maybe ten lines of code, and it's not really worth it. Reviewers?
Ok, I'm cool with everything in this patch except for radio.xml. That patch is incorrect for a few reasons. First of all, <radiogroup> can contain nested boxes, etc., so you can't assume <radio>'s parentNode is a <radiogroup>. That's true most of the time, but not necessarily all of the time. Second, I don't understand why it's needed. The "command" DOM event should fire on the <radio> when you invoke Click(), and that should bubble up to the <radiogroup>, which should have a handler in place. Perhaps the target of the DOM event is not being set correctly when Command() is invoked programmatically? Basically I'm thinking there's a bug somewhere but this isn't the right fix. If you want to check in without radio.xml, sr=hyatt, but file a separate bug on the radio issue. If you want to hold up, then we can investigate and do a new patch here. Either is cool with me.
I just did some investigation, and if you don't change radio.xml, pressing a radiobutton's accesskey (calling nsXULElement::Click() on it) treats it just like a checkbox (you can have multiple items selected at a time, or none at all...). Something is quite screwy with the event code here.
Attached patch patch #10! (obsolete) (deleted) — Splinter Review
jag? blake? wanna r= me? filed bug 94222 for the freaky radiobuttons
Depends on: 94222
I noticed one thing you could do better. In nsTextboxFrame, it's better to check for "for" before "accesskey". The former will be much less common than the latter, which is used in all of the menuitems (i.e., you'll slow down menus a little with the current patch, since you'll *find* an accesskey attr but won't find a "for" attr).
I don't need to see another patch, but reversing the "for" and the "accesskey" would be cool. sr=hyatt
Attached patch eleventh patch (obsolete) (deleted) — Splinter Review
I don't have CVS access anyway, so here's the eleventh patch.
One note. Support for <label control=""> is in. Not holding up the patch, but I don't want to see a bunch of people running around after this lands adding <text for=""> when that has been deprecated in favor of <label control="">. :)
Attached patch patch 12 (obsolete) (deleted) — Splinter Review
patch 12 has timeless's whitespace nits. Nothing else changed.
I went over the code with aegis and found a few things that could be done better. After some discussion with aegis we fixed/changed a few other things, which resulted in the latest patch (#13!). There are still a few open issues, I'll get back on those after I've talked to hyatt. For now I think this patch is fine. r=jag (and I'm sure r=aegis for the things I changed).
+ nsresult rv = mContent->GetAttribute(kNameSpaceID_None, + nsHTMLAtoms::_for, + forAttribute); Change the above to nsXULAtoms::control. The visibility collapsed or hidden case is not being covered. The user-focus property is also not being checked. We need to (if the visibility is collapse/hidden or if user-focus is not set to normal) not do the Click(). The event state manager does this on frames, so you can check the code over there to see this.
Basically tab panels will allow you to "click" objects in panels that aren't showing if we don't patch this to deal with visibility. Yet another issue (that can be resolved in a separate bug) is the problem of crossing iframes (as is the case in the prefs panel). You should be looking through *all* iframes in the chrome tree, so if you fail to find a registered match in one event state manager, you need to crawl (possibly both up and down) to the other event state managers (but avoid crawling into sandboxed HTML content).
We have no nsXULAtoms::control yet. ;) Should I add it? Does it magically Just Work in the parser?
I also don't know how to check visibility on a content node... I don't think I'm going to be able to get to a new patch today (my last day). Anyone else want to finish up?
Attached file testcase (XUL) (obsolete) (deleted) —
Attached file testcase (JS) (obsolete) (deleted) —
Now that aegis is gone, who wants to take over this code? Jag? Aegis mentioned that you had improvements you wanted to make to this patch before it was checked in.
Taking this.
Assignee: aegis → jaggernaut
Status: ASSIGNED → NEW
Attached patch [patch] Umpteenth patch (obsolete) (deleted) — Splinter Review
This patch includes the patch in bug 94222. This won't work without that patch. Looking for constructive comments, r= and sr=
I'm assuming you know what you're doing with the autostretch restructure. You probably should update the comment about the |for| attribute, as it now should be the |control| attribute. Did you ever find out why accesskeys are registered as |PRUint32|'s? nsEventStateManager has damn long methods. :) Code Complete would not be pleased. Try removing the <radio> special case in nsEventStateManager.cpp. The NS_XUL_CLICK fix should make that special case unnecessary. Speaking of which, the NS_XUL_CLICK code should be given r= by hyatt (or you, jag), as I'm the one who wrote that code. And as long as you're changing whitespace, do we really need the // releases. Duh. comment?
->0.9.5
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Wah. Why?
Too risky for 0.9.4, no immediate need for it, can wait till 0.9.5?
Attached patch Incorporate aegis' remarks (obsolete) (deleted) — Splinter Review
Removed <radio> specialcasing, fixed comment from 'for' to 'control'. Leaving PRUint32 "key" attributes and the redundant "release" comments for another patch.
Status: NEW → ASSIGNED
Jag says this is ready for r=/sr= All the important special cases are in there, says he. Reviewers? blake? hyatt?
upping severity to blocker since it is blocking work on several accessibility bugs
Severity: normal → blocker
picking this up for the end game
Assignee: jaggernaut → saari
Status: ASSIGNED → NEW
-> 0.9.6
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.5 → mozilla0.9.6
*** Bug 103783 has been marked as a duplicate of this bug. ***
Blocks: 104166
*** Bug 106339 has been marked as a duplicate of this bug. ***
Priority: P2 → P1
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Attached patch Patch updated to trunk (obsolete) (deleted) — Splinter Review
Attachment #40242 - Attachment is obsolete: true
Attachment #41734 - Attachment is obsolete: true
Attachment #41889 - Attachment is obsolete: true
Attachment #42040 - Attachment is obsolete: true
Attachment #42087 - Attachment is obsolete: true
Attachment #42245 - Attachment is obsolete: true
Attachment #43303 - Attachment is obsolete: true
Attachment #44472 - Attachment is obsolete: true
Attachment #44632 - Attachment is obsolete: true
Attachment #45001 - Attachment is obsolete: true
Attachment #45046 - Attachment is obsolete: true
Attachment #45176 - Attachment is obsolete: true
Attachment #45230 - Attachment is obsolete: true
Attachment #46741 - Attachment is obsolete: true
Attachment #48748 - Attachment is obsolete: true
Attached patch Above -buw (obsolete) (deleted) — Splinter Review
Attached file simple XUL testcase (obsolete) (deleted) —
Attachment #45447 - Attachment is obsolete: true
Attachment #45449 - Attachment is obsolete: true
For this testcase you'll need to make sure the content area is focused (click in the textbox for example). access+e should focus the "hello" checkbox (we don't support access key underlining in marked up text (e.g. xul:html) yet, the rest of the access keys should visible.
Blocks: 111031
Attached patch Update to nsBoxFrame.cpp (obsolete) (deleted) — Splinter Review
Attached patch Above, -uw (obsolete) (deleted) — Splinter Review
+ + nsString mOldAccessKey; + This is unnecessary. UnregisterAccessKey should be patched to not require that a key be specified, since only one key can be registered for an element anyway. We don't want to bloat all box frames by 24 bytes in order to cache the old string.
Blocks: 94802
Blocks: 113946
Blocks: 68899
Jag, it looks like you're still missing #ifdef PC (or #ifndef MAC) in those patches. The Control key is implemented on the Mac as the least nasty choice for accesskeys in HTML; but in XUL, either Command+accesskey should activate the controls, or nothing should. And if it's Command+accesskey, then you need what I described in comment 111. Meanwhile, I filed bug 117068 to remove the accesskey underlining if this bug doesn't get fixed before 1.0.
No longer blocks: 117068
Attached patch Latest patch (obsolete) (deleted) — Splinter Review
This patch still needs some work: - make click() generate mousedown, mouseup events in addition to click - add method to XBL to find out whether a property is inherited: this so we know when we should and shouldn't register accesskeys
Attachment #59807 - Attachment is obsolete: true
Attachment #59808 - Attachment is obsolete: true
Attachment #60367 - Attachment is obsolete: true
Attachment #60368 - Attachment is obsolete: true
The XBL problem should be fixed as follows: (1) In the XUL text frame, call GetBindingParent on your content node. (2) If you have a binding parent, then get the binding manager from the content node's document. (3) Add a new API to nsIBindingManager called IsAttributeInherited that takes the binding parent, the content node for the text frame, and an atom attribute (accesskey). (4) In the IMPL in binding manager, get the binding for the binding parent, and call IsAttributeInherited on the nsIXBLBinding. (5) In the impl of nsXBLBinding, you will have to crawl up until you find the binding with the anonymous content. When you hit that binding, call into the prototype binding. (6) In the prototype binding, you'll have to go to the attribute table and get the entry of all prototype elements that inherit the attribute. These are elements in the prototype content template (i.e., the original XML document). You'll need to use LocateInstance to get the corresponding real content node. If the real content node that you get for one of the prototype elements matches the content node of the text frame, then return true. Otherwise return false.
Never mind. I'm an idiot. The registration was only being done for text frames with a control attribute. That works and avoids the need for the XBL extensions.
Back to me again :-)
Assignee: saari → jaggernaut
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.7 → mozilla0.9.9
So the one remaining issue is to handle multi-line labels. You need to make a new kind of frame (nsLabelFrame) instead of just creating an nsBlockFrame in nsCSSFrameConstructor. You could just use this frame if the control attribute is present on the label. You then need to make this label frame get and cache the accesskey (responding to updates of the accesskey attribute in its attributechanged method as well). This label frame should have a customized paint method that first paints the block and then crawls its children looking for the first instance of the accesskey character. It should find the coordinates of the character, compute the coordinates of an underline, and then translate those coordinates back into the parent frame's space (by offsetting as you crawl back up your parent chain). The parent frame should then paint the underline. If you want to optimize this code, you could cache the coordinates of the underline after reflows to avoid crawling into your children on every paint, but that could come later. This should be fixed before 959 lands.
Blocks: 40761
Hyatt, it looks like this needs sr=. Can we get this in and file a separate bug for the multi line labels?
No, there's also the issue of getting rid of mOldAccessKey. I did that a few weeks ago and may still have the patch if you want it. But it's not that difficult...
Blake, if you could post that I'd appreciate it.
Keywords: nsbeta1
I have a new version that gets rid of mOldAccessKey in the nsBoxFrame case. I'll attach that.
Nevermind, it's already attached above. I could apply the same logic in the nsTextBoxFrame case, if deemed necessary, but unregistering these elements is faster when you know the accesskey (though I could store the key as a PRUnichar instead of as a nsString).
My patch didn't store it at all, anywhere. It just modified nsESM::UnregisterAccessKey to retrieve the accesskey itself from the content if aKey was null.
Also, we should use HasAttr when checking for the |control| attr, and probably |accesskey| as well.
I'll make those changes.
Doh, I thought about that (and forgot I thought about it), but the problem there is that when you're changing the accesskey you don't have the old accesskey to unregister on, only the new one.
I ran into that problem too, and fixed it by putting this in nsDocument::AttributeWillChange: if (aAttribute == nsXULAtoms::accesskey) { nsIPresShell* shell = (nsIPresShell*) mPresShells.SafeElementAt(0); if (shell) { nsCOMPtr<nsIPresContext> context; shell->GetPresContext(getter_AddRefs(context) ); if (context) { nsCOMPtr<nsIEventStateManager> esm; context->GetEventStateManager(getter_AddRefs(esm)); esm->UnregisterAccessKey(nsnull, aChild, nsnull); } } } perhaps hyatt can comment on whether that's lame or not.
That would do the trick. I'll but hyatt later.
context->GetEventStateManager(getter_AddRefs(esm)); esm->UnregisterAccessKey(nsnull, aChild, nsnull); Is it worthwhile to do a null check on esm, or will it always be returned?
I thought this also worked for: <label accesskey="f" control="bar">foopy</label> <button id="bar" value="Bar!"/> but apparently I still need to add some Reg/Unreg code to whatever frame deals with the above case.
Attachment #64816 - Attachment is obsolete: true
Looks like I should be able to add code similar to what's in nsBoxFrame.* to nsAreaFrame.*, but I'm hoping there's a cleaner way.
Blocks: 121680
Attached patch -uw of above (deleted) — Splinter Review
Attachment #68863 - Attachment is obsolete: true
Comment on attachment 70111 [details] [diff] [review] -uw of above sr=me
Attachment #70111 - Flags: superreview+
Comment on attachment 70111 [details] [diff] [review] -uw of above r=attinasi for the AreaFrame changes
Attachment #70111 - Flags: review+
a=asa (on behalf of drivers) for checkin to 0.9.9
Keywords: mozilla0.9.9+
nsbeta1+ per ADT triage team
Keywords: nsbeta1nsbeta1+
Checked in.
.
Status: NEW → RESOLVED
Closed: 25 years ago23 years ago
Resolution: --- → FIXED
No longer blocks: 113946
*** Bug 113946 has been marked as a duplicate of this bug. ***
This patch doesn't deal with tabpanel accesskeys. Was that just an oversight?
Yeah, that was. Could you file a new bug on that though? It shouldn't be too hard to add.
QA Contact: madhur → rakeshmishra
Attached file Test case (deleted) —
Test case for xul file
Attachment #59811 - Attachment is obsolete: true
verified on the trunk build 2002-04-30-08-trunk on Windows 2000.
Status: RESOLVED → VERIFIED
Blocks: robin's
Alias: AccesskeyXUL
Alias: AccesskeyXUL → Accesskey-XUL
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: