Build ID: new trunk (tip) The accesskey attribute of radiobuttons and checkboxes seems to be broken. We specify it in many places, but it has no effect on either element (not just that modifier+accesskey doesn't work, but that no letter is underlined). I'll investigate further when I'm done with what I'm working on.

->saari

This is failing at http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsTextBoxFrame.cpp#1 80 -- accesskey is null, and thus so is mAccessKey (never gets set). Getting the attributes through js, however, works. I think this might turn out to be a problem involving xbl, cc'ing hyatt.

blake: is this windows-only or xp?

dr, hyatt, could one of you just clean this up?

->dr

This problem is XP - seeing it on Linux in XUL. Anyone know when this regressed? Gerv

Is this a priority for accessibility?

Is this not a dup of bug 959?

It's not. As the description says, the problem here is that the correct letter (as defined by the accesskey attr.) is no longer underlined for checkboxes and radiobuttons. But this is useless anyways until bug 959 is fixed, so if that's not an accessibility priority, this isn't.

->1.0, depends on bug 959 per blake. aaronl: kick me if this is the wrong thing to do!

-> aegis, who is handling 959

I had a little talk with jag and we don't underline accesskeys for checkboxes or radiobuttons because they are using html elements rather than xul:text elements for the labels. Anyone know why we aren't using xul:text? Updating summary. access keyword

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

Does anyone actually need checkboxes to have an embedded HTML element rather than a XUL text element?

Oops. Preferences | Mail and Newsgroups | Message Display depends on a checkbox having a multiline label.

yes, they absolutely have to be multiline.

This change is going to be much more involved than I have time for. Feel free to take it.

I no longer have time for these. Anyone else want to take them?

I won't get to this.

->jag

-> 1.0

The fix for this is pretty hard, since checkboxes/radios wrap by default (unlike all other XUL widgets).

-> 1.2 One idea that's been floating around is to insert <u> into the wrapped markup (tricky). Another idea is to override the paint method, and find the position to draw the underline on top of the rest (lot of work). Anyone wanna take a stab at this, be my guest.

So I've got a plan for fixing this, and have started working on it as part of bug 959, but I'll move that development into this bug.

-> 1.0

Nav triage team needs info: aaronl: how critical is this for section 508? jag: is this part of your patch in bug 959

Not necessary for Section 508 compliance

Not part of that patch. I have the fix for this worked out in my head, and have a partial implementation in my tree. Finishing that will be about a day's worth of work.

Jag, can you post your patch in case the accessibility team or someone else gets cycles before you do?

nsbeta1- per Nav triage team, ->1.2

This makes the fix for bug 959 less exciting. A lot of options in Preferences, Find, and other dialogs are check boxes or radio buttons. Without the underline, it's not apparent what the access key is.

Jag, can you attach your work-in-progress, or at least describe your plan?

I believe this depends on bug 56701, "Need to highlight accesskey in HTML elements" The XUL checkboxes and radio buttons use XUL:label for the text, which use layout\html\base\src\nsAreaFrame. Same code. This will be a lot easier if we assume the accesskey is underlined. Making it stylable will create a lot of work, that won't be all that useful.

Taking, patch forthcoming.

This also fixes bug 56701 - underline accesskey in HTML content. This also takes care of both HTML and XUL <label>'s with accesskeys either on the label element or on the form control. It also makes sure that the text frame is redrawn when the accesskey changes. The patch also fixes an error in the click handler of XUL label elements, that prevented the associated form control from getting focused.

Aaron: any chance that click bug you mention is bug 127118?

At the request of Chris Waterson, I'm working on a different fix for this bug. Rather than computing the underline position when painting a text frame, the new fix inserts extra frames during frame construction. [pre textframe] [inlineaccesskeyframe [accesskeyframe]] [post textframe] I've fixed ConstructTextFrame so that it knows exactly when it has to split itself up into extra frames for the accesskey support. However,I need to tell the text frames it creates to use only a part of their content node. The following doesn't work: textFrame->SetOffsets(startOffset, endOffset) -- nsTextFrame::mContentLength get recomputed during reflow. How can I tell a text frame what chunk of the content node it gets, in a way that's not recomputed during reflow?

Although I haven't delved into it, text.xml seems like a pretty generic binding to me. If we implement it this way, will we end up processing the access key twice for some elements with this and what was implemented for bug 989?

I, of course, meant bug 959 not 989.

your caching worries me, what happens if someone changes a label's value? + <method name="formatAccessKey"> + <body> + <![CDATA[ + if (!this.firstChild) return; + if (this.childNodes.length > 1) { + // we don't remove the firstChild + while (this.childNodes.length > 1) + this.removeChild(this.lastChild); + } + var labelText = this.labelText || this.firstChild.data; + // Store original text for use next time this method is called + this.labelText = labelText; + if (this.accessKey) { + var accessKey = this.accessKey; + var accessKeyIndex = labelText.search(accessKey); + if (accessKeyIndex == -1) + accessKeyIndex = labelText.toLowerCase().search(accessKey.toLowerCase()); + if (accessKeyIndex == -1) { + // If accesskey is not in string (upper or lower case), append in parenthesis + labelText += "(" + accessKey + ")"; + accessKeyIndex = labelText.length - 2; + } + // Use <label>pretext<u>accesskey</u>posttext</label> to underline accesskey + this.firstChild.data = labelText.substr(0, accessKeyIndex); + this.appendChild(document.createElementNS("http://www.w3.org/1999/xhtml", "u")) + .appendChild(document.createTextNode(labelText.substr(accessKeyIndex,1))); + this.appendChild(document.createTextNode(labelText.substr(accessKeyIndex+1))); + } + else if (this.firstChild.data != labelText) + this.firstChild.data = labelText; + ]]> + </body> + </method> your setter is expensive for stupid callers: onset="this.setAttribute('accesskey', val); this.formatAccessKey(); return val;" i'd suggest that you change the setter to something like this: + <property name="accessKey" + onget="var accessKey = this.getAttribute('accesskey').charAt(0); + if (!accessKey) { + var labeledEl = this.labeledControlElement; + if (labeledEl) + accessKey = labeledEl.getAttribute('accesskey').charAt(0); + } + return accessKey;" + onset="if (val!=this.getAttribute('accesskey').charAt(0)) { + this.setAttribute('accesskey', val); this.formatAccessKey(); + } return val;" + />

oh right, i should have said something about the first code block. it's a suggested replacement for yours

Timeless, thanks for the suggestions. However, why would you do this? if (this.childNodes.length > 1) { // we don't remove the firstChild while (this.childNodes.length > 1) this.removeChild(this.lastChild); } instead of just this: // we don't remove the firstChild while (this.childNodes.length > 1) this.removeChild(this.lastChild);

Also, why did you switch the order to this? if (this.accessKey) { var accessKey = this.accessKey; ... Why not leave it: var accessKey = this.accessKey; if (accessKey) { ... Also, when you got rid of these conditions in formatAccessKey(): (this.accessKey || this.childNodes.length > 1) it makes it so it has to run that method every time a label is constructed. My new patch goes with that, but changes the constructor to add a condition to short circuit if no accesskey: <constructor> <![CDATA[ if (this.accessKey) this.formatAccessKey(); ]]> </constructor>

Comment #46 - because i wasn't thinking agressively enough. + if ((this.accessKey || this.childNodes.length > 1) && this.firstChild) { ... + var accessKey = this.accessKey; + if (accessKey) { the rewrite factors out the first part of the first line if (this.accessKey) so we can omit the second if which would never be useful because of the code refactoring. i don't quite follow your last thought :-(

Still has problems with bidi. AB_C_DE is coming out as BA_C_DE, heh. Will have to create the frames in reverse order, and figure out why "DE" in't coming out "ED". Smontagu, do you have time to look at this with me? Another problem is that in the case of <label for="cbox1">blah</label><input id="cbox1" type="checkbox" accesskey="a" />, on initial reflow the frame constructor doesn't always know about the checkbox yet. Pretty rare - I'd like to file a separate bug for that. I propose that a hash table is needed so that form controls can point back to their label elements.

This bug looks similar to the troubling bug 65951. But bug 65951 is more general. It seems to me that it would be more effective to join forces and come up with a more general solution that will fix both bugs. Basically, the issue in bug 65951 is that given a text run, "xy ... z" where 'x', 'y', ..., 'z' are unicode points (possibly surrogate pairs), _some_ of these characters have to be rendered while overriding certain styles that would otherwise apply to the entire text frame. In summary, while this bug wants: :-moz-accesskey-character { text-decoration: underline; } bug 65951 wants: :-moz-invariant-character { /* clear those properties (bold, italic, etc) that affects the style... */ font: medium serif; /* ...and only allow size-related properties */ font-size: inherit; font-size-adjust: inherit; font-stretch: inherit; } The main difference between the two bugs is that there can be several invariant characters in a text frame. I have been ruminating weird ideas for bug 65951 which I might throw in for discussion.

Rbs, we need to get together and talk about this. I'm aaronl on IRC.

I am behind a firewall that is rather restrictive and denies chat connections, audio streaming, etc -- so that students don't blow out the departmental budget. Let me summarize what I have been thinking right here. Weird idea #1: "anonymous content/frames", i.e., make the relevant container implement nsIAnonymousContent and split the text content into pieces for which separate frames are created. -> rejected because it calls for duplicate text content (1) the original text node for which no frames are created and (2) the anonymous nodes which are used for the actual rendering. It might also have unanticipated troubles with the selection or JS. Weird idea #2 (my current favorite): "style-switching with continuing frames". Layout is already capable of mapping a content/text node into several frames. These frames can technically be created with different style (or pseudo style) contexts. At the moment however, continuing text frames are created if and only if a single text frame doesn't "fit" in a geometric sense -- i.e., if the text is too long. So rather than just using a "fitting" criteria, the idea is to also allow another "style-switching" criteria. If done this way, the mechanism might cover both bugs, and could be confined to nsTextFrame. There won't be that much changes in the frame construction code (I see a number of latent problems when looking at your patch, including a re-entrancy problem in AttributeChanged. What you have is reminiscent of what is done for ':first-letter' and there are tons of bugs about that, and your patch isn't even doing all the housekeeping that ':first-letter' is doing). About the proposed style-switching idea. In the case of the accesskey, a style-switching would occur at a predefined offset (given by the parent). In the case of style-invariant characters, their computed indices would determine where style-switching has to be triggered. Taking you mockup in comment 38, it will look like this: [primary textframe] [continuing accesskey textframe] [continuing post textframe] where each text frame is Init'ed with its appropriate style context. The implementation of this would require careful modification of the flow between GetNextWord ('word' can be a half piece now), MeasureText (in nsTextFrame), and the textframe's Reflow() to forcibly return NS_FRAME_NOT_COMPLETE to trigger the creation of a continuation. From there on, the existing code that supports the handling of continuations will take over for free. Issues: perf - The splitting arise at reflow. On static pages: nothing to worry. On dynamic pages: there can be perf issues. Hopefully, the acesskey will usually occur on short strings. Maybe the incremental reflow can be optimized to re-use continuations. However, as usual, the initial focus should be to get things right first and optimize later. other?

I didn't check the exact naming of things. For the sake of compeleteness, /nsIAnonymousContent/ above was meant to be the anonymous frame creator hook (from where other contents can then be created at frame construction).

Rbs, I was thinking of using nsIAnonymousContentCreator too. I think that might be the best. It would only need to duplicate text content on those nodes that have accesskeys. As far as weird idea #2, spliting 1 content node into several frames as you suggest. I have a patch here that does that called "Implements via extra inline and text frame construction, still bidi problem."

Beware, there are some subtelties between all these three approaches. Have another chat with waterson. Also keep in mind that what you did isn't general enough to cover bug 65951 and maintaining the integrity of the frame model in the face of dynamic changes is going to be much harder/error-prone. In weird idea #1, there can be potential troubles with the selection and JS, and these might be another can of worms. #2 seems more robust, and it is more likely that the bidi problem will be covered as well since GetNextWord() is already bidi-aware.

is patch from 2002-08-13 still valid? target is also way off 1.2alpha...

clearing milestone.

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

adt: nsbeta1+/adt2

Rbs was right -- there are problems with selection with this anonymous content approach. This only affects HTML accesskeys, because you can't select labels in XUL. One solution would be to check this in only for XUL, and maybe have a hidden pref to turn on HTML access key underlining until we figure out the fix for selection. Or, scrap it and start over. I dunno at this point. This bug is definitely creating too much work for me.

>One solution would be to check this in only for XUL, and maybe have a hidden >pref to turn on HTML access key underlining until we figure out the fix for >selection. I am afraid that adding so much code is going to perpetrate hack upon hack. If you can get hold of a layout folk, I still suggest that you also put the weird idea #2 of "style-switching with continuing frames" on the table. BTW, |nsStyledSubstringTextFrame : public nsInlineFrame| is not a good name. Text frames are leaf frames, and this inheritance looks awkward. The choice of the name of your class isn't good.

Rbs, thansk for the quick feedback. I can change the name -- do you have a better name for nsStyledSubstringFrame? I have already tried weird idea #2, and it worked quite well actually - but it was rejected by the layout folks as changing too much about text frame reflow. The problem is that although text frames have offset and length variables that allow them to use only part of a content node, they always want to change those offsets whenever a reflow happens. > I am afraid that adding so much code is going to perpetrate hack upon hack. Are you talking about the patch in general, or the idea of a pref for HTML accesskey. As far as I'm concerned, we can just file another bug when we check this in, "Cannot select only part of a label element when it has an accesskey attribute"

Notes on the 4 workable approaches I've used so far: Attachment 84727 [details] [diff] - "Draws underline in nsTextFrame.cpp" - rejected because Waterson thought it was the wrong way, because of bidi and nested inlines etc. Attachment 87741 [details] [diff] - "Total XBL fix for XUL radios and checkboxes, still needs layout blocker fixed (151930)" -- rejected because it would be too slow for HTML, although we could do it just for <label> and forget about <a>. It also had a problem with some boxtoblockadapter bugs when the accesskey was changed dynamically. Attachment 95127 [details] [diff] - "Implements via extra inline and text frame construction, still bidi problem." -- same as weird idea #2 . Rejected because it changed too much of the way text is reflowed. Considered too risky. Attachment 123796 [details] [diff] - "Cleaned up diff, removed some wrong stuff" -- this is the latest one that uses nsIAnonymousContentCreator (rbs's weird idea #1). Still waiting for feedback, although dbaron says that interface is deprecated in his opinion. Looking forward to hearing which if these approaches will be officially sanctioned by the layout folks.

>I have already tried weird idea #2, and it worked quite well actually - but it Are we talking about the same thing? Do you have a patch for that, for the record? It is not quite the same as the patch that you had before. (I am more keen on a generic solution that will fix bug 65951 as well.) I mentioned earlier that the change you did in |nsInlineFrame::AttributeChanged| is a recipe for re-entrancy which doesn't play nice in layout. [Specifically, the |doc->AttributeChanged| will roll back to the frame constructor, with the hint to destroy the frame (while the function hasn't yet complete), etc.]

> Weird idea #2 (my current favorite): "style-switching with continuing frames". > Layout is already capable of mapping a content/text node into several frames. > These frames can technically be created with different style (or pseudo style) > contexts. At the moment however, continuing text frames are created if and > only if a single text frame doesn't "fit" in a geometric sense -- i.e., if > the text is too long. So rather than just using a "fitting" criteria, the > idea is to also allow another "style-switching" criteria. If done this way, > the mechanism might cover both bugs, and could be confined to nsTextFrame. This is Attachment 95127 [details] [diff] - "Implements via extra inline and text frame construction, still bidi problem.". Rejected by Kin because it changed too much of the way text is reflowed. Considered too risky. The patch uses the mContentOffset and mContentLength variables in nsTextFrame and nsContinuingTextFrame. > If done this way, the mechanism might cover both bugs, The most recent patch could also be used to cover both bugs, but it would need a little more work to handle more than >= 2 islands of math chars that need the style applied in the same frame text frame. I have some comments in the patch that describe what's needed. > and could be confined to nsTextFrame. No, the problem is that although nsTextFrame and nsContinuingTextFrame do what you say, they also want to constantly redefine their content and offset when they reflow, based on size constraints. It takes a lot of work to make the offsets stay put, while still letting the normal process of word wrapping take place, because the <label> just might need to wrap, *and* have an accesskey. >I mentioned earlier that the change you did in nsInlineFrame::AttributeChanged| > is a recipe for re-entrancy which doesn't play nice in layout. [Specifically, > the |doc->AttributeChanged| will roll back to the frame constructor, with the > hint to destroy the frame (while the function hasn't yet complete), etc.] I haven't found any reentrancy problems, but I believe you. Will try to think of a way around that.

Re: the potential re-entrancy problems. I have no idea why the current patch has no reentrancy problem as you describe. Doesn't reflow occur via events or something?

> This is Attachment 95127 [details] [diff] - "Implements via extra inline and text frame It is similar, but not quite the same. What I am suggesting is way more lightweight than that patch (which is not the same at saying that it is any easier :-)). Currently nsTextFrame _has_ to detect whether or not to break, and what I am suggesting is to try to extend the criteria under which the break happen. > Re: the potential re-entrancy problems. It is a recipe for that to happen, although it might not bite in your limited testing, it is fragile and not really a pattern to build upon.

> What I am suggesting is way more lightweight than that patch (which is not the > same at saying that it is any easier :-)). > Currently nsTextFrame _has_ to detect whether or not to break, and what I am > suggesting is to try to extend the criteria under which the break happen. If you try it, I think you'll find that this is not a light weight change. It's also a lot more of a dangerous change than my most recent patch, which uses anonymous content and new frame types. There is no risk for most pages, which do not use accesskeys, because it changes absolutely nothing about normal text frames. I have patches for 4 approaches that work here, and it's been a lot of work. Will the module owner please step forward and tell me why none of the four approaches can be used.

>If you try it, I think you'll find that this is not a light weight change. I mention the weight in a comparative way with a smiley :-). I stressed that it doesn't mean that it is easy at all. Indeed, it is very finicky and demanding to work in nsTextFrame, and dangerous as you pointed out. It will need intensive care (which isn't helped by a late rush from an nsbeta+ at short notice). However, there might be some (generic) potential here, but there could be pitfalls and other things that I may be overlooking. If this issue was that simple, there wouldn't be 4+1 approaches... I can only encourage you to give this a try too, knowing that it is also a bit speculative... Understandably, I personally don't like either of your patches because they are too restrictive. A lot of code, but only addressing one situation, without regard to similar situations of interest to me as I pointed out (e.g., bug 65951).

Rbs, I don't want to try any more approaches unless it is absolutely necessary. I warn that there are no unused flags in nsTextFrame, and it is extremely difficult to work there without having room for any new state variables or bit flags -- unless you want to increase the size of text frames. Without adding extra state, I don't know how you'll prevent layout from doing it's normal thing with text frames. Is it not possible to use the latest approach for bug 65951? You would need to know when it might be necessary to look through a unicode text node for these unicode math chraracters, and when these characters are found, notify the anonymous text frame creator. You would just need to make it capable of handling more offset, length, pseduo-element trios.

>Rbs, I don't want to try any more approaches unless it is absolutely necessary. I think it might be worth it. But it would be irresponsible on my part to promise the panacea. Theory and practice don't always agree. Sorry, I don't buy your latest patch which involves going to nsCSSFrameConstructor::ConstructAccessKeyFrame() and other stuff there, which don't look naturally extensible. If there are no state bits anymore, those TextFrames of interest can be marked somehow (e.g., as a property in the frame manager).

ConstructAccessKeyFrame can get a new name -- don't think that it's not extensible, it just needs an index, offset and pseudo element so that it can tell nsStyledSubstringFrame (bad name I know) where to change styles.

Comment on attachment 123796 [details] [diff] [review] Cleaned up diff, removed some wrong stuff Seeking sr= for this patch, although there are other possible approachs in this bug that work too. If not this approach, what approach would be accepted?

(I had this comment all written out and I was bitten by the Phoenix autocomplete crasher, so rewriting.) I'm going to use the term mnemonic for the thing that we underline or display differently, since I want to use a different term than accesskey for the display side of things, since it need not be equivalent to the accesskey, etc. I'm not crazy about the term, so feel free to think of a better one. I'm against a solution involving frame construction at this point, since I think our frame construction code is a mess and needs to be rewritten. That precludes using bold text for mnemonics, but in discussions with aaronl I think we can live with that. I think that which key should be underlined is essentially stylistic information. Having that information live in a style struct (nsStyleUIReset?) forces separation of the complexity of figuring out which letter to underline from the complexity of actually painting the underline. It might not be necessary to actually do that in the first pass, but things should definitely be done in a way that allows that to happen easily later if we want to do it (for example, but encapsulating all the determination in nsIFrame::GetMnemonic.) If we were to use style to determine the accesskey, we might want a -moz-mnemonic property (reset, not inherited) that took attr(X) and perhaps also string values. We might need this to do the XUL side, since XUL doesn't have an attribute mapping setup like the one that exists in nsHTMLStyleSheet. (I'd like to have a solution that allows us to replace the code in nsTextBoxFrame rather than having two separate sets of code to do accesskey underlining.) Probably the best way to handle HTML's LABEL and the mess of the FOR attribute is with a custom nsIStyleRule implementation that lives in nsHTMLLabelElement.cpp. To do the painting, I'd probably want to add a method to nsIFrame called something like PaintMnemonic. This would probably be called (when a frame is the first-in-flow and the style struct or nsIFrame method says there's a mnemonic) from nsContainerFrame::Paint and nsLeafFrame::Paint (and maybe other places -- the usage pattern of nsFrame::Paint is bizarre right now and needs to be fixed, but it probably wouldn't work to just put it there). This would be a virtual function, implemented on at least the following classes: + nsFrame, where it would do nothing and return false + nsContainerFrame, where it would loop through the children (something needs to be done to handle next-in-flows, but I haven't thought that through yet -- and it would probably make something here a bit more complicated, since you might (for XUL) want to be able to handle a mnemonic on a split frame, even when that split frame is a leaf) - calling PaintMnemonic on each child until it one returned true (in which case it would immediately return true) or there were no children left (in which case it would return false) + nsTextFrame and nsTextBoxFrame, where they would search for the letter in question (if it's not found, immediately return false) and if it's there, paint the underline (nsTextBoxFrame already has a good bit of code to do this) and return true. At least, that's how I think I'd want to implement it. I'd like feedback from others (rbs, roc, bz) before someone goes ahead with this, or with any of the other ideas. And I haven't proofread the above because I'm afraid of crashing again. I probably forgot something...

dbaron's approach sounds good to me. Fairly straightforward, fairly lightweight, and performant.

Re: comment 76 In terms of "it works", that might do, but unfortunately it seems to me that it would be overkill to have this elaborate system just for accesskey underlining. If all that is needed is underlining, attachment 84727 [details] [diff] [review] might be a simpler alternative. Also, it won't help to eliminate nsTextBoxFrame because of the handling of the |crop| attribute (which looked to me as one of the original core motivations behind the establishment of that separate nsTextBoxFrame). The more I think about this, the more I lean towards thinking that the key to the matter might be to bite the bullet in |nsTextFrame::MeasureText|. Another non fancy, but cleaner version (not meant for general use), say, |nsTextFrame::MeasureTextSlowly| could be introduced to provide a basis for breaking the text in non-conventional ways, thereby allowing to fix this bug, bug 65951, as well as bug 99457, and if pushed harder someday could be used to decide how to crop -- which is needed to replace nsTextBoxFrame.

If I were to implement this, I would go for: 1) style side: rather than a |-moz-mnemonic| property which is going to be invasive and somewhat troublesome to define (e.g., underline+bold?), I will simply go for styles such as: :-moz-accesskey-character { // for this bug text-decoration: underline; font-style: bold; /* i.e., it allows existing properties */ } :-moz-invariant-character { // for bug 65951 ... } 2) implement |nsTextFrame::MeasureTextSlowly| to allow breaking the text in unconventional ways. 3) allow the "continuations" to pick their styles (and thus to be painted according to what their style says). [The style information could be handled via the "additional style" API -- but such specific programming details can be worked out in due course.]

Dbaron, that sounds fine. I don't think I should be the one to do this patch, instead it should be someone more familiar with the style system. Rbs, are you interested in writing up a patch?

As I said in bug 65951 comment 11, I don't see how bug 65951 is related to this bug and I don't see why it should be solved in a way at all similar to the solution here. The difficulty with :-moz-accesskey-character is that it requires either (a) messing with frame construction, and I'm against adding any more complexity to frame construction without cleaning up the mess that's already there (consider how difficult handling block-within-inline via frame construction has been) or (b) hacking both reflow and painting in a way that requires a lot of ugly "I know exactly what this other code is doing" style code. I don't see how what I proposed in comment 76 is elaborate -- I merely described it in significant detail. Furthermore, it has the advantages that it is robust under bidirectional text, under splitting of inlines across lines, and under the element with the accesskey property specified having different 'display' types.

re: comment 81 > (a) messing with frame construction Although aaronl's patch is 'pure' frame construction, what I am proposing happens during reflow and attempts to leverage on what is there (it will be mostly confined to nsTextFrame). > handling block-within-inline That was 'pure' frame construction with its ripple effect... vs. (doing something at reflow -- the approach now advocated in bug 142585 which sounds better to me). I am not a big fan of the 'pure' frame construction of everything. The beauty is in the balance of things. > (b) hacking both reflow and painting This is a special case, and it is not much different (in spirit) from the business of -moz-mnemonic, PaintMnemonic, etc, which are just a manifestation of that special case. I don't see how this bug can be fixed without special-casing. BTW, the advantages that you mentioned are there. And there are more, e.g., bold accesskey will be supported now. Plus, it provides a basis for arbitrary wrapping, cropping. Whereas just asking a bold accesskey will bring the problem back to square one if it is to be added to -moz-mnemonic later. To further clarify what I am suggesting: it is the textframe that is split -- not necessarily its container. For example, MathML frames can contain text frames directly, without an intermediate nsInlineFrame. That's one of the reasons why I have been dubious about the extensibility of aaronl's patch. With what I described, a single container (be it an nsInlineFrame, a XUL frame, or a MathML frame) will be able to contain several child text frames ("continuations"), each handled with its own suitable style to achieve the desired effect. As for bug 65951, once the perspective is that the textframe is split, it becomes essentially the same as this one. The opposite of not splitting the textframe means that all zillion versions of Gfx have to deal with the problem. That is, if layout passes a textrun with those special characters in between, then in the midst of measuring/drawing, one has to turn on/off certains styles. For example a bold font has to become normal just for those special characters. It should be noted that, down in Gfx (e.g., GfxWin), the act of turning on/off certain styles amounts to realizing fonts in the midst of measuring/drawing. And this for all the zillion versions of Gfx. In short, it becomes even more complex unnecessarily. re: comment 80 > Rbs, are you interested in writing up a patch? Nope. At least not right now... But you can get r/sr for your patch if you want. Note that it is just that I won't sign for it. The others might weight things differently.

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

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

This is a major accessibility issues and it seems like all we've done is argue for a year. Did anyone actually decide what the "correct" solution is?

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

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

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

Re: comment 76, quoting: > We might need this to do the XUL side, since XUL doesn't have an > attribute mapping setup like the one that exists in nsHTMLStyleSheet. Now that it's 2004, don't we have unified attribute mapping content code for XUL and HTML? Cc'ing jst. /be

David Baron and I spoke about this bug in person, right after the moz dev day. It's much more important to get this working in XUL than HTML -- authors need to manually underline the labels in HTML anyway since IE doesn't automatically underline accesskeys. OTOH the fact that accesskeys aren't underlined in XUL dialogs is a big usability issue -- accesskeys are much more important in UI anyway. A XUL-only fix also has some advantages: 1) it can be done in XBL, so core Gecko isn't affected. Less risky. 2) it takes care of the important case where the accesskey char is not in the label, and needs to be put in parenthesis afterward. This happens a lot because of localization. For example: <label accesskey="e">Oppin Fyl</label> Oppin Fyl (e)

Need some advice. I can get to the <label> from nsBoxFrame::AttributeChanged() when accesskey is changed on the checkbox itself. How should I reflow the label? This isn't working: + if (labelContent) { + nsIDocument *doc = labelContent->GetDocument(); + doc->AttributeChanged(labelContent, aNameSpaceID, nsXULAtoms::accesskey, aModType); + } + }

Comment on attachment 158164 [details] [diff] [review] XBL fix only for XUL -- relatively safe, and that's where we actually need it. Implements both underlined & paren types This is just for the trunk. At least we can get it in there.

Comment on attachment 158164 [details] [diff] [review] XBL fix only for XUL -- relatively safe, and that's where we actually need it. Implements both underlined & paren types >+ if ((this.accessKey || this.childNodes.length > 1) && this.firstChild) { Don't see the point of testing this.childNodes.length and this.firstChild; perhaps (this.accessKey && this.hasChildNodes()) will do. >+ var labelText = this.labelText? this.labelText: this.firstChild.data; >+ while (this.childNodes.length > 1) >+ this.removeChild(this.lastChild); >+ this.labelText = labelText; // Store original text for use next time this method is called This does make it difficult to change the text of the label... I was considering var labelText = this.textContent.replace(/ \(.\)$/, ""); >+ var accessKey = this.accessKey; >+ if (accessKey) { Might be an idea to insert accessKey = accessKey[0]; I believe our other access key code only looks at the first letter. >+ var accessKeyIndex = labelText.search(accessKey); indexOf would be a better method to use. >+ // Use <label>pretext<u>accesskey</u>posttext</label> to underline accesskey Apparently u is deprecated, the text to html converter wasn't permitted to use it... >+ this.firstChild.data = labelText.substr(0, accessKeyIndex); Can use this.textContent here to save removing the child nodes yourself. Also saves creating a text node if the access key index is zero. >+ this.appendChild(document.createElementNS("http://www.w3.org/1999/xhtml", "u")); >+ this.lastChild.appendChild(document.createTextNode(labelText.substr(accessKeyIndex,1))); Can also use .textContent here; also use labelText[accessKeyIndex] for a single character. >+ this.appendChild(document.createTextNode(labelText.substr(accessKeyIndex+1))); Should check that >+ else if (this.firstChild.data != labelText) >+ this.firstChild.data = labelText; .textContent again; also a reminder: no trailing spaces please. >+ var accessKey = this.getAttribute('accesskey').charAt(0); [0] etc.

(In reply to comment #94) > (From update of attachment 158164 [details] [diff] [review]) > >+ if ((this.accessKey || this.childNodes.length > 1) && this.firstChild) { > Don't see the point of testing this.childNodes.length and this.firstChild; > perhaps (this.accessKey && this.hasChildNodes()) will do. We need the ( ... || this.childNodes.length) so that if the accesskey goes away dynamically that the block of text gets reformatted as 1 item again. Will look at the rest soon.

Neil, I believe I addressed all of your comments but possibly created new issues. You were right, my previous patch made it difficult to dynamically change the label. The new one doesn't have to cache labelText so it doesn't have that problem. The (x) possiblility after the label is done as anon content.

Comment on attachment 158802 [details] [diff] [review] Address Neil's comments >-[scriptable, uuid(f68136d6-1dd1-11b2-a184-a55a337e8507)] >+[scriptable, uuid(c987629e-6370-45f5-86ec-aa765fa861cd)] > interface nsIDOMXULLabelElement : nsIDOMXULDescriptionElement { >- attribute boolean accessKey; >+ attribute DOMString accessKey; > attribute DOMString control; >+ readonly attribute nsIDOMElement labeledControlElement; I don't see why you put this in the interface, it's only used internally. >-[scriptable, uuid(a457ea70-1dd1-11b2-9089-8fd894122084)] >+[scriptable, uuid(754ed576-2c0d-4bff-817a-3aa87102fa29)] > interface nsIDOMXULLabeledControlElement : nsIDOMXULControlElement { > attribute DOMString crop; > attribute DOMString image; > attribute DOMString label; >+ attribute nsIDOMElement labelElement; Should be an nsIDOMXULLabelElement to save on QI later. >+ onget="return this.labelElement ? this.labelElement.accessKey : this.getAttribute('accesskey');" >+ onset="this.setAttribute('accesskey', val); return val;"/> Hmm... this isn't very symmetric; if you have a label, then you can't get the access key you set. What was the idea? >+ <property name="labelElement" onget="return this._labelElement;" >+ onset="this._labelElement = val;"/> >+ <field name="_labelElement"/> Surprisingly <field name="labelElement"/> works just as well. Better in fact, because you forgot to return val from your setter :-P >+ <xhtml:span anonid="accessKeyParens"/> Neat idea, but I think the x is a typo. >+ accessKeyIndex = labelText.toLowerCase().search(accessKey.toLowerCase()); Missed an indexOf opportunity here. >+ labelNode.appendChild(document.createElementNS("http://www.w3.org/1999/xhtml", "span")). >+ style.textDecoration = "underline"; The word "style" is lined up nicely, but the trailing "." on the previous line belongs before it on the second line. >+ labelNode.lastChild.appendChild(document.createTextNode(labelText.substr(accessKeyIndex,1))); Opportunities for both .textContent and [] here. >+ var postText = labelText.substr(accessKeyIndex+1); Spaces around operators, please. And I'm sure I saw some other operators with only one space somewhere too. >+ <property name="accessKey"> >+ <getter> >+ <![CDATA[ >+ if (!this.hasChildNodes()) { >+ return null; >+ } Is that realistic? Won't this binding get used for labels with values rather than child text, that already display access keys? >+ var accessKey = this.getAttribute('accesskey')[0]; Sorry, it looks as if I was too enthusiasic with the []s - "".charAt(0) returns "" but ""[0] returns undefined :-[ >+ labelContent->GetAttr(kNameSpaceID_None, nsXULAtoms::accesskey, accessKey); >+ nsCOMPtr<nsIDOMXULLabelElement> labelElement(do_QueryInterface(labelContent)); Any reason why you couldn't have used labelElement->GetAccessKey(accessKey); ? >+ labelElement->SetAccessKey(accessKey);

>>+ onget="return this.labelElement ? this.labelElement.accessKey : this.getAttribute('accesskey');" >>+ onset="this.setAttribute('accesskey', val); return val;"/> > Hmm... this isn't very symmetric; if you have a label, then you can't get the > access key you set. What was the idea? If you have a label, it already has the logic to decide which accesskey to use, so we just ask the label. The label's accesskey attribute takes precedence (I had to choose one of them, but it's really an edge case to have two accesskeys). >>+ labelContent->GetAttr(kNameSpaceID_None, nsXULAtoms::accesskey, accessKey); >>+ nsCOMPtr<nsIDOMXULLabelElement> > labelElement(do_QueryInterface(labelContent)); > Any reason why you couldn't have used labelElement->GetAccessKey(accessKey); ? Yes, I've added a comment to explain in the patch. Otherwise we might get the accesskey from the control and set the attribute on the label and end up with 2 accesskey attributes when we previously had just one. >>+ labelNode.lastChild.appendChild(document.createTextNode(labelText.substr(accessKeyIndex,1))); > Opportunities for both .textContent and [] here. I changed it to use [] now, I don't see a place to use textContent, since we need to append after the span here: <label> / | text <span> | text

(In reply to comment #98) >If you have a label, it already has the logic to decide which accesskey >to use, so we just ask the label. The label's accesskey attribute takes >precedence (I had to choose one of them, but it's really an edge case to >have two accesskeys). OK, the point I'm trying to make is that if you have a control element with its own label then it's confusing to set the access key on the control but read it from the label. On the other hand, if the control has an access key but the label does not, then at when you set the access key on the label it takes precedence so you can read it back. >>Any reason why you couldn't have used labelElement->GetAccessKey(accessKey); >Yes, I've added a comment to explain in the patch. Otherwise we might get the >accesskey from the control and set the attribute on the label and end up with >2 accesskey attributes when we previously had just one. I might be way off track here, but if you did improve the setter for the control it might just make it possible to get and set the access key from the control without having to expose the label element on the interface (although you would still want to fix the label's interface for completeness). >>>+labelNode.lastChild.appendChild(document.createTextNode(labelText.substr(accessKeyIndex,1))); >> Opportunities for both .textContent and [] here. >I changed it to use [] now, I don't see a place to use textContent, since we >need to append after the span here: Not after the span, but as a child of the span... the following line covers the after the span case, which you still have to do longhand - perhaps .appendText will make DOM4 ;-)

I believe labelElement needs to stay in nsIDOMXULLabeledControlElement because it's used in text.xml One thing that doesn't work with this patch is dynamically updating the accesskey when the label has sub-elements (testcase items #8-11). I'm not sure why it doesn't work, for some reason setting the textContent isn't updating the view. Could be a bug in core gecko somewhere. Personally I think it's a really minor edgecase flaw.

Comment on attachment 159008 [details] [diff] [review] Another round of changes. Takes care of cases where there are other elements within <label> Neil, I haven't tried shortening the code that inserts the span by using innerHTML

Comment on attachment 159008 [details] [diff] [review] Another round of changes. Takes care of cases where there are other elements within <label> >+ attribute nsIDOMXULLabelElement labelElement; text.xml doesn't need this, XBL fields are always exposed to JS. >+ var oldAccessKey = labelNode.getElementsByAttribute('class', 'accesskey'); >+ if (oldAccessKey[0]) { // Clear old accesskey Should use .item(0) here to avoid JS strict warnings. It's safe to tack onto the previous line if you prefer, to save repeating it on the next line. >+ labelText = labelNode.textContent; labelNode is now known to be a text node at this point, so you could try switching to using .data to see if it helps with your issue. >+ span.style.textDecoration = "underline"; >+ span.className = "accesskey"; Need to ask a layout guru if it's better to add a CSS rule for html|span.accesskey instead.

I didn't change |labelText = labelNode.textContent| to use .data, the problems start before that line where the old accesskey can't get cleared by |oldAccessKey.textContent = oldAccessKey.textContent|. In fact in my tests I can't set oldAccessKey.textContent to anything -- it's not taking effect.

I fixed some of the code in the test cases, although 10 & 11 still have weird effects; they should all toggle now too, which makes them more interesting.

This patch requires that nsGenericDOMDataNode::CloneContent be made virtual (Neil's doing that in another bug). Will post crash test case next.

Hmm, it looks like bug 96108 has been touched to fix the assertion, so I'll see if updating to the trunk fixes the crash. Perhaps the last patch is the right one after all.

Comment on attachment 159135 [details] [diff] [review] Same patch but with xul.css change >+ try { >+ if (bindingParent.QueryInterface(Components.interfaces.nsIDOMXULLabeledControlElement)) { >+ control = bindingParent; // For controls that make the <label> an anon child >+ } >+ } catch(ex) {} Instead of try/catch, please use (bindingParent instanceof Components.interfaces.nsIDOMXULLabeledControlElement) as your condition. >+ if (control) >+ control.labelElement = this; I notice you're inconsistent with those braces - you have approximately equal numbers with and without.

Comment on attachment 159135 [details] [diff] [review] Same patch but with xul.css change After updating to trunk the crash problem goes away.

Comment on attachment 159153 [details] [diff] [review] Uses instanceOf and constent bracing {} I notice that the label whose value does not contain its access key writes the parenthesized access key without an extra space; I quite like your extra space so I suggest you file a bug on the label value version.

Will file a bug on the label value version to have space before parens.

Comment on attachment 159153 [details] [diff] [review] Uses instanceOf and constent bracing {} Woohoo! Mike, we're really close now. Now that Neil's already looked at it you know it's good :)

Neil, does your sr= also count torward changing the same files in Seamonkey? Or, do you want a separate patch posted for the Seamonkey changes?

Comment on attachment 159153 [details] [diff] [review] Uses instanceOf and constent bracing {} looks good!

Not sure why the radio button accesskeys aren't getting underlined in seamonkey prefs "Blank Page", "Navigator Startup", "Last Page Visited". It works elesewhere. Will have to file a separate bug on that, as well as the space before parens issue Neil pointed out. Checking in dom/public/idl/xul/nsIDOMXULLabelElement.idl; /cvsroot/mozilla/dom/public/idl/xul/nsIDOMXULLabelElement.idl,v <-- nsIDOMXULLabelElement.idl new revision: 1.3; previous revision: 1.2 done Checking in toolkit/content/widgets/general.xml; /cvsroot/mozilla/toolkit/content/widgets/general.xml,v <-- general.xml new revision: 1.5; previous revision: 1.4 done Checking in toolkit/content/widgets/text.xml; /cvsroot/mozilla/toolkit/content/widgets/text.xml,v <-- text.xml new revision: 1.5; previous revision: 1.4 done Checking in toolkit/content/xul.css; /cvsroot/mozilla/toolkit/content/xul.css,v <-- xul.css new revision: 1.40; previous revision: 1.39 done Checking in layout/xul/base/src/nsTextBoxFrame.cpp; /cvsroot/mozilla/layout/xul/base/src/nsTextBoxFrame.cpp,v <-- nsTextBoxFrame.cpp new revision: 1.74; previous revision: 1.73 done Checking in xpfe/global/resources/content/xul.css; /cvsroot/mozilla/xpfe/global/resources/content/xul.css,v <-- xul.css new revision: 1.134; previous revision: 1.133 done Checking in xpfe/global/resources/content/bindings/general.xml; /cvsroot/mozilla/xpfe/global/resources/content/bindings/general.xml,v <-- general.xml new revision: 1.31; previous revision: 1.30 done Checking in xpfe/global/resources/content/bindings/text.xml; /cvsroot/mozilla/xpfe/global/resources/content/bindings/text.xml,v <-- text.xml new revision: 1.11; previous revision: 1.10 done

Comment on attachment 159027 [details] [diff] [review] Yet another patch patch is obsolete

This seems to have caused bug 260657 (Txul / Ts regression)

The checkin at 2004-09-20 04:29 broke context menus in many parts of the app. They appear slowly or not at all. (It's particularly easy to reproduce on the bookmarks in the main tree in the bookmarks manager.)

The context menu regression is fixed by the patch in bug 260657.

If someone can make the case that there are no more lurking regressions related to these changes, feel free to request approval to land on the branch. We're not going to hold for this, though.

Relevant parts of patch relanded following landing of aviary branch

bad - the accesskeys are visible on mac.

(In reply to comment #124) > bad - the accesskeys are visible on mac. When did that regress?

(In reply to comment #125) > (In reply to comment #124) > > bad - the accesskeys are visible on mac. > > When did that regress? When the patch was landed, probably. (if i wasn't clear, the mnemonics are visible for radios/checks *only*)

Comment on attachment 159153 [details] [diff] [review] Uses instanceOf and constent bracing {} >+ <constructor> >+ <![CDATA[ >+ this.formatAccessKey(); >+ ]]> >+ </constructor> #ifdef it?

If you don't want the suite's access keys to be visible on the mac you could put an override rule into mac/platformXUL.css to remove the underline.

(In reply to comment #128) > If you don't want the suite's access keys to be visible on the mac you could put > an override rule into mac/platformXUL.css to remove the underline. filed bug 277463 for this

this bug doesn't really depend upon bug 56701. at least, it doesn't look like it does since it was RESOLVED FIXED. removing in order to clean up blocker list for 1.8 branch. /cb