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)
Core
DOM: UI Events & Focus Handling
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...
Updated•26 years ago
|
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
Comment 3•26 years ago
|
||
per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser
Updated•26 years ago
|
Target Milestone: M4 → M5
Comment 6•26 years ago
|
||
This is not getting done by M4. Moving to M5.
Updated•25 years ago
|
Target Milestone: M5 → M6
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).
Comment 8•25 years ago
|
||
Moving out to M7
Updated•25 years ago
|
Target Milestone: M7 → M8
Comment 9•25 years ago
|
||
Moving to M8
Updated•25 years ago
|
Target Milestone: M8 → M9
Comment 10•25 years ago
|
||
No time for M8
Updated•25 years ago
|
Whiteboard: [MAKINGTEST] Antti.Nayha@oulu.fi
Updated•25 years ago
|
Target Milestone: M9 → M11
Comment 11•25 years ago
|
||
No dependencies yelling for this so its lower priority. Moving to M11.
Comment 12•25 years ago
|
||
Is this important for beta?
Hyatt, will the menu key listener interfer with this?
Comment 13•25 years ago
|
||
Speaking for QA (and QA only) this is unimportant for beta. angus?
Comment 14•25 years ago
|
||
Moving multiple bugs to m12
Comment 15•25 years ago
|
||
Moving to m13 because Joki seems to be distracted.
Updated•25 years ago
|
Whiteboard: [MAKINGTEST] Antti.Nayha@oulu.fi
Updated•25 years ago
|
Summary: accesskey attribute not implemented yet → [FEATURE] accesskey attribute not implemented yet
Target Milestone: M14 → M16
Comment 17•25 years ago
|
||
Moving M16.
Comment 18•25 years ago
|
||
*** Bug 7559 has been marked as a duplicate of this bug. ***
Comment 19•25 years ago
|
||
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
Comment 20•24 years ago
|
||
*** Bug 33318 has been marked as a duplicate of this bug. ***
Comment 21•24 years ago
|
||
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?
Comment 22•24 years ago
|
||
Nominating for nsbeta2 and adding the 'html4' keyword.
Adding dependency on this bug to bug 7954 (HTML 4.0 compliance meta bug).
Comment 23•24 years ago
|
||
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]
Comment 25•24 years ago
|
||
*** Bug 33318 has been marked as a duplicate of this bug. ***
Comment 26•24 years ago
|
||
Okay, this is 90% fixed. Accesskey is in and works for everything except links.
Working on that last bit.
Comment 27•24 years ago
|
||
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
Comment 28•24 years ago
|
||
Comment 29•24 years ago
|
||
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)
Comment 30•24 years ago
|
||
now regs and unregs in SetDocument - fixed
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 32•24 years ago
|
||
This must work for XUL as well. Should I give myself a separate bug for XUL, or
do we want to use this bug?
Comment 33•24 years ago
|
||
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+]
Updated•24 years ago
|
Status: NEW → ASSIGNED
Updated•24 years ago
|
Whiteboard: [nsbeta2+] → [nsbeta2+] done for html, needs to be done for xul
Comment 35•24 years ago
|
||
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
Comment 37•24 years ago
|
||
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
Updated•24 years ago
|
Target Milestone: M18 → M20
Comment 38•24 years ago
|
||
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?
Comment 39•24 years ago
|
||
> 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.
Comment 40•24 years ago
|
||
#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)?
Comment 41•24 years ago
|
||
#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.)
Comment 42•24 years ago
|
||
Sorry if I sound argumentative. I agree with you guys :)
Comment 43•24 years ago
|
||
> #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.)
Comment 44•24 years ago
|
||
Isn't this long done?
Comment 45•24 years ago
|
||
*** Bug 43772 has been marked as a duplicate of this bug. ***
Comment 46•24 years ago
|
||
*** Bug 43772 has been marked as a duplicate of this bug. ***
Comment 47•24 years ago
|
||
I think we'll have to live without this in current release, -> Future
Target Milestone: M20 → Future
Comment 48•24 years ago
|
||
Mass update: changing qacontact to ckritzer@netscape.com
QA Contact: janc → ckritzer
Comment 50•24 years ago
|
||
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
Comment 51•24 years ago
|
||
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???
Comment 52•24 years ago
|
||
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.
Comment 53•24 years ago
|
||
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
Comment 54•24 years ago
|
||
Hey Dan. I suggest you file the above as a separate bug report, if it should be
done. Otherwise, it will get no attention.
Comment 55•24 years ago
|
||
*** Bug 49968 has been marked as a duplicate of this bug. ***
Comment 56•24 years ago
|
||
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...
Comment 57•24 years ago
|
||
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).
Comment 59•24 years ago
|
||
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
Comment 60•24 years ago
|
||
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.
Comment 61•24 years ago
|
||
Yep, you're right. Removing those.
Comment 62•24 years ago
|
||
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
Comment 63•24 years ago
|
||
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.
Comment 64•24 years ago
|
||
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).
Comment 65•24 years ago
|
||
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.
Comment 66•24 years ago
|
||
*** Bug 64730 has been marked as a duplicate of this bug. ***
Comment 67•24 years ago
|
||
REMOVING nsbeta2/3 and replacing with nsbeta1 KW. Also clearing
Status Whiteboard of nsbeta2/3 result. Accessibility is important.
Comment 68•24 years ago
|
||
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
Comment 69•24 years ago
|
||
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 → ---
Comment 70•24 years ago
|
||
I agree, this is fairly important for accessibility (although not embedding,
really). cc'ing trudelle.
Comment 71•24 years ago
|
||
[Really CCing Trudelle]
Summary: [FEATURE]Accesskey attribute not implemented yet for XUL → [FEATURE] Accesskey attribute not implemented yet for XUL
Comment 72•24 years ago
|
||
*** Bug 68769 has been marked as a duplicate of this bug. ***
Comment 73•24 years ago
|
||
Nominating for mozilla0.9 (this blocks a whole lot of bugs).
Keywords: mozilla0.9
Comment 74•24 years ago
|
||
->mozilla1.0, is this absolutely required for accessibility in next rev of the apps?
Target Milestone: --- → mozilla1.0
Comment 75•24 years ago
|
||
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
Comment 76•23 years ago
|
||
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).
Comment 77•23 years ago
|
||
Comment 78•23 years ago
|
||
*** Bug 70216 has been marked as a duplicate of this bug. ***
Comment 80•23 years ago
|
||
*** Bug 43756 has been marked as a duplicate of this bug. ***
Comment 81•23 years ago
|
||
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.
Comment 82•23 years ago
|
||
Comment 83•23 years ago
|
||
Oh yeah, forgot to mention... There are some changes to the find search dialog
in there. I used those for testing.
Status: NEW → ASSIGNED
Comment 84•23 years ago
|
||
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.
Comment 85•23 years ago
|
||
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?
Comment 86•23 years ago
|
||
Comment 87•23 years ago
|
||
Filed bug 88151 for modifying HTML accesskeys via the DOM.
Comment 88•23 years ago
|
||
I don't think this bug blocks bug 41368 any longer-at least not since the
switchover from HTML to XUL.
Comment 89•23 years ago
|
||
Comment 90•23 years ago
|
||
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.
Comment 91•23 years ago
|
||
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.
Comment 92•23 years ago
|
||
Comment 93•23 years ago
|
||
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.
Comment 94•23 years ago
|
||
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!
Comment 95•23 years ago
|
||
Comment 96•23 years ago
|
||
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...
Comment 97•23 years ago
|
||
Comment 98•23 years ago
|
||
Comment 99•23 years ago
|
||
Comment 100•23 years ago
|
||
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?
Comment 101•23 years ago
|
||
We can get this in for 0.9.3.
Target Milestone: mozilla1.0 → mozilla0.9.3
Comment 102•23 years ago
|
||
Don't do this:
+ nsXULElement* element = NS_STATIC_CAST(nsXULElement*, raw_content);
Comment 103•23 years ago
|
||
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?
Comment 104•23 years ago
|
||
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.
Comment 105•23 years ago
|
||
Comment 106•23 years ago
|
||
pushing to 0.9.4 for aegis. won't make the 0.9.3 train unfortunately...
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Comment 107•23 years ago
|
||
Oops, ignore the change to checkbox.xml.
Comment 108•23 years ago
|
||
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.
Comment 109•23 years ago
|
||
<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
Comment 110•23 years ago
|
||
Comment 111•23 years ago
|
||
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.
Comment 112•23 years ago
|
||
Comment 113•23 years ago
|
||
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.
Comment 114•23 years ago
|
||
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?
Comment 115•23 years ago
|
||
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.
Comment 116•23 years ago
|
||
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.
Comment 117•23 years ago
|
||
Comment 118•23 years ago
|
||
jag? blake? wanna r= me?
filed bug 94222 for the freaky radiobuttons
Comment 119•23 years ago
|
||
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).
Comment 120•23 years ago
|
||
I don't need to see another patch, but reversing the "for" and the "accesskey"
would be cool. sr=hyatt
Comment 121•23 years ago
|
||
Comment 122•23 years ago
|
||
I don't have CVS access anyway, so here's the eleventh patch.
Comment 123•23 years ago
|
||
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="">. :)
Comment 124•23 years ago
|
||
Comment 125•23 years ago
|
||
patch 12 has timeless's whitespace nits. Nothing else changed.
Assignee | ||
Comment 126•23 years ago
|
||
Assignee | ||
Comment 127•23 years ago
|
||
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).
Comment 128•23 years ago
|
||
+ 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.
Comment 129•23 years ago
|
||
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).
Comment 130•23 years ago
|
||
We have no nsXULAtoms::control yet. ;) Should I add it? Does it magically
Just Work in the parser?
Comment 131•23 years ago
|
||
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?
Comment 132•23 years ago
|
||
Comment 133•23 years ago
|
||
Comment 134•23 years ago
|
||
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.
Assignee | ||
Comment 136•23 years ago
|
||
Assignee | ||
Comment 137•23 years ago
|
||
This patch includes the patch in bug 94222. This won't work without that patch.
Looking for constructive comments, r= and sr=
Comment 138•23 years ago
|
||
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?
Comment 140•23 years ago
|
||
Wah. Why?
Assignee | ||
Comment 141•23 years ago
|
||
Too risky for 0.9.4, no immediate need for it, can wait till 0.9.5?
Assignee | ||
Comment 142•23 years ago
|
||
Assignee | ||
Comment 143•23 years ago
|
||
Removed <radio> specialcasing, fixed comment from 'for' to 'control'. Leaving
PRUint32 "key" attributes and the redundant "release" comments for another
patch.
Status: NEW → ASSIGNED
Comment 144•23 years ago
|
||
Jag says this is ready for r=/sr=
All the important special cases are in there, says he.
Reviewers? blake? hyatt?
Comment 145•23 years ago
|
||
upping severity to blocker since it is blocking work on several accessibility bugs
Severity: normal → blocker
Comment 146•23 years ago
|
||
picking this up for the end game
Assignee: jaggernaut → saari
Status: ASSIGNED → NEW
Comment 147•23 years ago
|
||
-> 0.9.6
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Comment 148•23 years ago
|
||
*** Bug 103783 has been marked as a duplicate of this bug. ***
Comment 149•23 years ago
|
||
*** Bug 106339 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Priority: P2 → P1
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Assignee | ||
Comment 150•23 years ago
|
||
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
Assignee | ||
Updated•23 years ago
|
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
Assignee | ||
Comment 151•23 years ago
|
||
Assignee | ||
Comment 152•23 years ago
|
||
Attachment #45447 -
Attachment is obsolete: true
Attachment #45449 -
Attachment is obsolete: true
Assignee | ||
Comment 153•23 years ago
|
||
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.
Comment 154•23 years ago
|
||
Attachment #59808 -
Flags: review+
Assignee | ||
Comment 155•23 years ago
|
||
Assignee | ||
Comment 156•23 years ago
|
||
Comment 157•23 years ago
|
||
+
+ 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.
Comment 158•23 years ago
|
||
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
Assignee | ||
Comment 159•23 years ago
|
||
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
Comment 160•23 years ago
|
||
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.
Comment 161•23 years ago
|
||
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.
Assignee | ||
Comment 162•23 years ago
|
||
Back to me again :-)
Assignee: saari → jaggernaut
Status: ASSIGNED → NEW
Target Milestone: mozilla0.9.7 → mozilla0.9.9
Comment 163•23 years ago
|
||
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.
Comment 164•23 years ago
|
||
Hyatt, it looks like this needs sr=.
Can we get this in and file a separate bug for the multi line labels?
Comment 165•23 years ago
|
||
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...
Comment 166•23 years ago
|
||
Blake, if you could post that I'd appreciate it.
Assignee | ||
Comment 167•23 years ago
|
||
I have a new version that gets rid of mOldAccessKey in the nsBoxFrame case. I'll
attach that.
Assignee | ||
Comment 168•23 years ago
|
||
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).
Comment 169•23 years ago
|
||
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.
Comment 170•23 years ago
|
||
Also, we should use HasAttr when checking for the |control| attr, and probably
|accesskey| as well.
Assignee | ||
Comment 171•23 years ago
|
||
I'll make those changes.
Assignee | ||
Comment 172•23 years ago
|
||
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.
Comment 173•23 years ago
|
||
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.
Assignee | ||
Comment 174•23 years ago
|
||
That would do the trick. I'll but hyatt later.
Comment 175•23 years ago
|
||
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?
Assignee | ||
Comment 176•23 years ago
|
||
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
Assignee | ||
Comment 177•23 years ago
|
||
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.
Assignee | ||
Comment 178•23 years ago
|
||
Assignee | ||
Comment 179•23 years ago
|
||
Attachment #68863 -
Attachment is obsolete: true
Comment 180•23 years ago
|
||
Comment on attachment 70111 [details] [diff] [review]
-uw of above
sr=me
Attachment #70111 -
Flags: superreview+
Comment 181•23 years ago
|
||
Comment on attachment 70111 [details] [diff] [review]
-uw of above
r=attinasi for the AreaFrame changes
Attachment #70111 -
Flags: review+
Assignee | ||
Comment 184•23 years ago
|
||
Checked in.
Assignee | ||
Comment 185•23 years ago
|
||
.
Status: NEW → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
Comment 186•23 years ago
|
||
*** Bug 113946 has been marked as a duplicate of this bug. ***
Comment 187•23 years ago
|
||
This patch doesn't deal with tabpanel accesskeys. Was that just an oversight?
Assignee | ||
Comment 188•23 years ago
|
||
Yeah, that was. Could you file a new bug on that though? It shouldn't be too
hard to add.
Updated•22 years ago
|
QA Contact: madhur → rakeshmishra
Assignee | ||
Comment 189•22 years ago
|
||
Test case for xul file
Attachment #59811 -
Attachment is obsolete: true
Comment 190•22 years ago
|
||
verified on the trunk build 2002-04-30-08-trunk on Windows 2000.
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Alias: AccesskeyXUL
Updated•20 years ago
|
Alias: AccesskeyXUL → Accesskey-XUL
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
Comment 191•5 years ago
|
||
Keywords: sec508
You need to log in
before you can comment on or make changes to this bug.
Description
•