Closed
Bug 54710
Opened 24 years ago
Closed 24 years ago
menuitem sometimes has incorrect accesskey underlining if value set after accesskey
Categories
(Core :: XUL, defect, P3)
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: jag+mozbugs, Assigned: mikepinkerton)
Details
Attachments
(3 files)
35.70 KB,
patch
|
Details | Diff | Splinter Review | |
22.46 KB,
patch
|
Details | Diff | Splinter Review | |
22.45 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 2•24 years ago
|
||
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).
Comment 4•24 years ago
|
||
Pink, is it unthinkable to try for an rtm fix?
/be
Assignee | ||
Comment 5•24 years ago
|
||
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.
Reporter | ||
Comment 6•24 years ago
|
||
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.
Reporter | ||
Comment 7•24 years ago
|
||
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.
Reporter | ||
Comment 8•24 years ago
|
||
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.
Reporter | ||
Comment 9•24 years ago
|
||
Reporter | ||
Comment 10•24 years ago
|
||
Reporter | ||
Comment 11•24 years ago
|
||
Adding keywords, and I'll be back in a few hours, so give this some pounding if
you can.
Reporter | ||
Comment 12•24 years ago
|
||
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.
Reporter | ||
Comment 13•24 years ago
|
||
cc'ing evaughan, it's his code.
Comment 14•24 years ago
|
||
Yep the smallest rect is the best to intersect with. Looks good.
Comment 15•24 years ago
|
||
a=hyatt on the super-review.
Reporter | ||
Comment 16•24 years ago
|
||
Reporter | ||
Comment 17•24 years ago
|
||
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.
Comment 18•24 years ago
|
||
r=evaughan
Reporter | ||
Comment 19•24 years ago
|
||
Checked in on trunk. Feel free to mark resolved fixed unless you wanna get this on the branch.
Reporter | ||
Comment 20•24 years ago
|
||
No way this is going in on the branch, resolving fixed.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in
before you can comment on or make changes to this bug.
Description
•