It seems that when a menuitem's value is set separately from the accesskey (e.g. accesskey defined in xul, value set from js), the wrong or no letter gets underlined.

okay

I notice you futured this, so I'll just go through the motions for completeness' sake: cc'ing hyatt. Seeing this in recent nightlies and trunk builds on linux, and on windows nightlies after uncommenting that menu (branch build apparantly).

Can you whip up a test case?

Pink, is it unthinkable to try for an rtm fix? /be

brendan, given all the other things we've futured that i'd rather fix, i think this is so trivial, that we shouldn't even bother for 6.01.

So, to clarify, in ``View -> Text Si_z_e (xx %)'' (1), and in ``View -> Text Size (xx %) -> _O_ther (xx %)...'' (2), the accesskeys sometimes don't get underlined correctly. Switching themes usually "fixes" that. In (1) I've seen ``T_e_xt Size (xx %)'' (a) or no underline at all (b). In (2) so far I've seen no underline at all (b). Both these menuitem values are set from javascript, but start out empty. Now, for (a), after studying the code (nsTextBoxFrame) a bit, what I think happens is that since we don't have a value to start with, but do have an accesskey: - In |Init| the value attribute is fetched and the accesskey index determined - Since value is empty, index = -1 and ``(Z)'' is appended (*) to the value, but the accesskey index pointing at the spot to underline is left at -1 - We're drawn (|Paint|), which causes the cropped title to be determined (``(Z)'') and ``(Z)'' to be drawn, no underline - From JS oncreate, value gets set which triggers |AttributeChanged| in which the title is updated, the accesskey index determined (from the cropped title, which still has ``(Z)''!!!) to be 2, and we're Marked Dirty and thus get |Paint|ed, at which point the second char will be underlined Now (b) happens in a similar manner, except we don't get drawn before the value gets set from JS, so our accesskey index is still -1, thus nothing gets drawn. I'm rewriting the code, stay tuned.

For the previous comment: (*) If the accesskey can't be found in the value (title), it is appended between parens, or inserted between parens before an ellipsis (``...'') Upon further analysis, it looks like events are slightly different. Say we |Init| then |Paint|. Say our mState doesn't have NS_STATE_NEED_LAYOUT set, so we exit |LayoutTitle| early and don't determine our cropped title. Accesskey index was -1, so in |PaintTitle| we don't draw the underline. Now, the value gets set from JS, accesskey index determined (from an empty cropped title!) -> -1, the NS_STATE_NEED_LAYOUT bit is set, and we |Paint|, |LayoutTitle| which now does determine the cropped title, so next time we set a value from JS, our accesskey index will "correctly" be determined (only because the part containing the accesskey didn't change). This is case (b). I'm still trying to understand case (a). It would need the NS_STATE_NEED_LAYOUT bit to be set, in which case after |Init|, |Paint|, in |LayoutTitle| we do determine the cropped title, ``(Z)'', then when setting the value from JS, accesskey index is 1 and when we paint, that's what gets underlined (thus, the e). Now, we ourselves don't seem to be setting the NS_STATE_NEED_LAYOUT bit, but that bit clashes with NS_STATE_IS_HORIZONTAL in nsBoxFrame, both are 0x00400000. So if that bit is set from nsBoxFrame, we'd get (a). However, it doesn't seem to explain why (2) is always (b), and (1) is mostly (a), sometimes (b). Anyway, what needs to change is that when the value is set (initially, or later), the "append accesskey between parens?" test needs to be done. Then, after the title is cropped in LayoutTitle, the accesskey index needs to be determined (for the cropped title), after all, there might not be enough room to show that key, nor its underlining. I believe I've got this done in my code, I'll go test that now.

Okay, I think I've got this. Someone want to take a look and see if this is the right direction? Please be critical and tell me which changes I need to improve. Besides the above suggested changes, I've also fixed two bug in CalculateTitleForWidth, one in case CropLeft, one in case CropCenter. Further I've removed unused parameters from functions, put things in a (to me) more logical order, did elipsis -> ellipsis, and move the NS_STATE_NEED_LAYOUT flag up a few bits. Oh, and fixed whitespace. I'll attach both -u and -u -w.

Adding keywords, and I'll be back in a few hours, so give this some pounding if you can.

So, one minor enhancement would be to use kNotFound instead of -1. Another thing I'm wondering about... In PaintTitle, we check if aRect intersects with aDirtyRect. Above that, textRect was created based on aRect, then width was set to the cropped title width. I moved that down to where textRect was actually used, but shouldn't the intersection test be on textRect instead of aRect? It would seem logical to only redraw the title if it itself was dirty, instead of the whole surrounding box.

cc'ing evaughan, it's his code.

Yep the smallest rect is the best to intersect with. Looks good.

a=hyatt on the super-review.

Just to be on the safe side, r=evaughan, a=hyatt on this new patch? I'll apply the whitespace fix-up version of this one, btw.

r=evaughan

Checked in on trunk. Feel free to mark resolved fixed unless you wanna get this on the branch.

No way this is going in on the branch, resolving fixed.