99853 - Changing the 'key' attribute in a XUL menu item doesn't work

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

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

Using the DOM to change the 'key' attribute of a XUL menu item doesn't change
the displayed key text if the menu frame has already been created.

This is because when the frame is first created, the 'acceltext' attribute on
the XUL menu item is set to the fancy name of the key defined by the 'key'
attribute, and after the 'acceltext' attribute is set, the 'key' attribute is
always ignored.

To fix this, I suggest changing the XBL bindings to inherit the value of a new
attribute, 'computedacceltext', and modify nsMenuFrame::BuildAcceleratorText to
update this attribute instead of 'acceltext'. nsMenuFrame::AttributeChanged
needs to trap changes to 'key' and 'acceltext' and rebuild the accelerator text
into 'computedacceltext' when necessary.

BTW, nsMenuFrame::mMenuText and nsMenuFrame::mAccelText are never used and we
should get rid of them.

Comment 1

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

Attachment: Proposed fix

Comment 2

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

Attachment: Sorry --- missed a file

Comment 3

[David Hyatt]

I don't want to add a whole extra attribute here that pollutes the content model
of menus.  It seems like the frame could just cache in a bit whether or not its
acceltext comes from its key or not.  Then if the key changes, and if the
acceltext came from the key, then it allows the acceltext to be replaced by the
new key.  If the acceltext did not come from the previous key, then you leave it
alone when the new key is set.   You could even use the frame state bits to
store the bit.

Comment 4

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

I considered that, but... How do we keep that bit current in the face of
AttributeChanged()-notified changes to 'acceltext'? Can we distinguish a
client's DOM changes to 'acceltext' from the changes performed by nsMenuFrame
itself?

BTW, if it's space you're worried about, very few menu items have the
'acceltext' attribute set by the client, so with this patch almost all menu
items just have the 'key' and 'computedacceltext' attributes (no worse than the
'key' and 'acceltext' attributes they have now).

Comment 5

[David Hyatt]

Another problem is that someone should be able to query for the acceltext
property/attribute via the DOM and have that work.  Changing to
"computedacceltext" makes the acceltext property no longer work.

There is a very specific code path used when the presence of a key causes the
acceltext to be set.  In that particular path, you can set the acceltext
attribute without notifying the front end (using PR_FALSE in SetAttr), and then
just do directly what would have been done in the AttributeChanged method for
acceltext.  You can then set the special bit saying that a key is responsible
for this acceltext.

Then you'll know that in the AttributeChanged case that someone really changed
acceltext, so you can clear the special bit (if it's set) and update accordingly.

In the AttributeChanged case for key, you can check the bit and only update the
acceltext if the bit is set or if the current acceltext is empty.

Comment 6

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

OK, that sounds fine. I will produce another patch along those lines when I get
home tonight.

Comment 7

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

Attachment: Patch (v2) --- use frame state bit instead

Comment 8

[David Hyatt]

sr=hyatt on v2 patch.

Comment 9

[David Hyatt]

Roc, feel free to take this bug and land the patch.

Comment 10

[David Hyatt]

--> roc.

Comment 11

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

Thanks. I asked jag for a review. Jag??

Comment 12

[jag (Peter Annema)]

Comment on attachment 51629 [details] [diff] [review]
Patch (v2) --- use frame state bit instead

r=jag

Comment 13

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

Checked in.