68841 - We don't underline accesskeys for XUL checkboxes or radiobuttons

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P4)

Description

[Blake Ross]

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.

Comment 1

[Peter Trudelle]

->saari

Comment 2

[Blake Ross]

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.

Comment 3

[Dan Rosen]

blake: is this windows-only or xp?

Comment 4

[saari (gone)]

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

Comment 5

[Peter Trudelle]

->dr

Comment 6

[Gervase Markham [:gerv]]

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

Gerv

Comment 7

[Dan Rosen]

Is this a priority for accessibility?

Comment 8

[Aaron Leventhal]

Is this not a dup of bug 959?

Comment 9

[Blake Ross]

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.

Comment 10

[Dan Rosen]

->1.0, depends on bug 959 per blake.

aaronl: kick me if this is the wrong thing to do!

Comment 11

[Aaron Leventhal]

-> aegis, who is handling 959 

Comment 12

[Chad Austin]

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

Comment 13

[Chad Austin]

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

Comment 14

[Chad Austin]

Attachment: proposed fix

Comment 15

[Chad Austin]

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

Comment 16

[Chad Austin]

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

Comment 17

[David Hyatt]

yes, they absolutely have to be multiline.

Comment 18

[Chad Austin]

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

Comment 19

[Chad Austin]

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

Comment 20

[Chad Austin]

I won't get to this.

Comment 21

[Peter Trudelle]

->jag

Comment 22

[jag (Peter Annema)]

-> 1.0

Comment 23

[David Hyatt]

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

Comment 24

[jag (Peter Annema)]

-> 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.

Comment 25

[jag (Peter Annema)]

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.

Comment 26

[jag (Peter Annema)]

-> 1.0

Comment 27

[Peter Trudelle]

Nav triage team needs info:

aaronl: how critical is this for section 508?  
jag: is this part of your patch in bug 959

Comment 28

[Aaron Leventhal]

Not necessary for Section 508 compliance

Comment 29

[jag (Peter Annema)]

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.

Comment 30

[Aaron Leventhal]

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

Comment 31

[Peter Trudelle]

nsbeta1- per Nav triage team, ->1.2

Comment 32

[:deanis74]

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.

Comment 33

[:deanis74]

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

Comment 34

[Aaron Leventhal]

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.

Comment 35

[Aaron Leventhal]

Taking, patch forthcoming.

Comment 36

[Aaron Leventhal]

Attachment: Draws underline in nsTextFrame.cpp

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.

Comment 37

[:deanis74]

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

Comment 38

[Aaron Leventhal]

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?

Comment 39

[Aaron Leventhal]

Attachment: Implemented using XBL, occasional formatting problem still needs resolution

Comment 40

[:deanis74]

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?

Comment 41

[:deanis74]

I, of course, meant bug 959 not 989.

Comment 42

[Aaron Leventhal]

Attachment: Total XBL fix for XUL radios and checkboxes, still needs layout blocker fixed (151930)

Comment 43

[Aaron Leventhal]

Attachment: Total XBL fix for XUL radios and checkboxes, still needs layout blocker fixed (151930)

Comment 44

[timeless]

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;"
+      />

Comment 45

[timeless]

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

Comment 46

[Aaron Leventhal]

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);

Comment 47

[Aaron Leventhal]

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 48

[timeless]

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 :-(

Comment 49

[Aaron Leventhal]

Attachment: Implements via extra inline and text frame construction, still bidi problem.

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.

Comment 50

[rbs]

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.

Comment 51

[Aaron Leventhal]

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

Comment 52

[rbs]

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?

Comment 53

[rbs]

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).

Comment 54

[Aaron Leventhal]

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." 

Comment 55

[rbs]

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.

Comment 56

[Henrik Gemal]

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

Comment 57

[sairuh (rarely reading bugmail)]

clearing milestone.

Comment 58

[Aaron Leventhal]

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

Comment 59

[Aaron Leventhal]

Attachment: New approach using nsIAnonymousContentCreator. Not ready for prime time.

Comment 60

[Samir Gehani]

adt: nsbeta1+/adt2

Comment 61

[Aaron Leventhal]

Attachment: Uses nsIAnonymousContentCreator to create new text nodes for node. Works great except for selection, which doesn't

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.

Comment 62

[Aaron Leventhal]

Attachment: Cleaned up diff, removed some wrong stuff

Comment 63

[rbs]

>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.

Comment 64

[Aaron Leventhal]

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"

Comment 65

[Aaron Leventhal]

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.

Comment 66

[rbs]

>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.]

Comment 67

[Aaron Leventhal]

> 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.

Comment 68

[Aaron Leventhal]

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?

Comment 69

[rbs]

> 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.

Comment 70

[Aaron Leventhal]

> 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.

Comment 71

[rbs]

>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).

Comment 72

[Aaron Leventhal]

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.

Comment 73

[rbs]

>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).

Comment 74

[Aaron Leventhal]

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 75

[Aaron Leventhal]

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?

Comment 76

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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...

Comment 77

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 78

[rbs]

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.

Comment 79

[rbs]

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.]

Comment 80

[Aaron Leventhal]

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?

Comment 81

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 82

[rbs]

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.

Comment 83

[Blake Ross]

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

Comment 84

[Simon Paquet [:sipaq]]

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

Comment 85

[Mike Kaply [:mkaply]]

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?

Comment 86

[u60234]

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

Comment 87

[Jesiah S]

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

Comment 88

[u60234]

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

Comment 89

[Brendan Eich [:brendan]]

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

Comment 90

[Aaron Leventhal]

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)

Comment 91

[Aaron Leventhal]

Attachment: Work in progress. nsBoxFrame::AttributeChanged needs work to invalidate <label>

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 92

[Aaron Leventhal]

Attachment: XBL fix only for XUL -- relatively safe, and that's where we actually need it. Implements both underlined & paren types

Comment 93

[Aaron Leventhal]

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 94

[[e-mail]]

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.

Comment 95

[Aaron Leventhal]

(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.

Comment 96

[Aaron Leventhal]

Attachment: Address Neil's comments

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 97

[[e-mail]]

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);

Comment 98

[Aaron Leventhal]

Attachment: Addresses Neil's 2nd set of comments

>>+		   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

Comment 99

[[e-mail]]

(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 ;-)

Comment 100

[Aaron Leventhal]

Attachment: Assortment of test cases

Comment 101

[Aaron Leventhal]

Attachment: Another round of changes. Takes care of cases where there are other elements within <label>

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 102

[Aaron Leventhal]

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 103

[[e-mail]]

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.

Comment 104

[Aaron Leventhal]

Attachment: Yet another patch

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.

Comment 105

[[e-mail]]

Attachment: Fixed test cases

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.

Comment 106

[Aaron Leventhal]

Attachment: Works with testcase, but crashes with a testcase based on only item #11

This patch requires that nsGenericDOMDataNode::CloneContent be made virtual
(Neil's doing that in another bug).

Will post crash test case next.

Comment 107

[Aaron Leventhal]

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 108

[Aaron Leventhal]

Attachment: Same patch but with xul.css change

Comment 109

[[e-mail]]

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 110

[Aaron Leventhal]

Comment on attachment 159135 [details] [diff] [review]
Same patch but with xul.css change

After updating to trunk the crash problem goes away.

Comment 111

[Aaron Leventhal]

Attachment: Uses instanceOf and constent bracing {}

Comment 112

[[e-mail]]

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.

Comment 113

[Aaron Leventhal]

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

Comment 114

[Aaron Leventhal]

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 :)

Comment 115

[Aaron Leventhal]

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 116

[Mike Connor [:mconnor]]

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

looks good!

Comment 117

[Aaron Leventhal]

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 118

[Simon Paquet [:sipaq]]

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

patch is obsolete

Comment 119

[Robert Kaiser]

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

Comment 120

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 121

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 122

[Asa Dotzler [:asa]]

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.

Comment 123

[Ian Neal]

Relevant parts of patch relanded following landing of aviary branch

Comment 124

[Asaf Romano (gone)]

bad - the accesskeys are visible on mac.

Comment 125

[Aaron Leventhal]

(In reply to comment #124)
> bad - the accesskeys are visible on mac.

When did that regress?

Comment 126

[Asaf Romano (gone)]

(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 127

[Asaf Romano (gone)]

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


>+      <constructor>
>+        <![CDATA[
>+          this.formatAccessKey();
>+        ]]>
>+      </constructor>

#ifdef it?

Comment 128

[[e-mail]]

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.

Comment 129

[Jo Hermans]

(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

Comment 130

[Chris Beard]

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