Closed Bug 630830 Opened 9 years ago Closed 9 years ago

"key" attribute changes to menuitems are not handled

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
blocking2.0 --- -

People

(Reporter: mano, Assigned: enndeakin)

References

Details

(Keywords: regression, Whiteboard: required for polishing bug 580638 on mac)

Attachments

(1 file, 7 obsolete files)

Some code in nsMenuFrame.cpp suggests that it takes care of changes to the "key" attribute of menuitems:

http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuFrame.cpp#174

But while working on bug 580638, I found that after opening the menu for the first time, changes to the key attribute aren't handled at all.

I'm not sure if it's a regression. It might be that it never worked, and  it might be dead code.

Surprisingly enough,  Cocoa menus suffer from the same issue.  However, I'm sure that's not a regression.


P.S. What's the right component for layout/xul bugs?
Component: Layout → XP Toolkit/Widgets: Menus
OS: Mac OS X → All
QA Contact: layout → xptoolkit.menus
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
Need to write a test as well.

Also fixes a regression from 2003 from bug 190735 by switching to RemoveStateBits.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Neil, should I file a separate bug for mac menus?
blocking2.0: --- → ?
Drivers, this blocks bug 580638 in the sense of "the workaround is ugly". However, if Neil thinks the fix is safe enough, I think we should take it for FF4.
Blocks: 580638
Attached patch mac patch (obsolete) — Splinter Review
Attachment #509841 - Attachment is obsolete: true
Attachment #510323 - Flags: review?(joshmoz)
Attachment #509841 - Attachment is obsolete: false
Neil: with the mac patch, does changing the key of a menuitem update the highlight-menuitem behavior? For example, if Cmd+W is removed from File->Close, would the File menu highlight when Cmd+W is pressed (before the menu is reopened)?
Comment on attachment 510323 [details] [diff] [review]
mac patch

OK, let me test removing a key as well.
Attachment #510323 - Flags: review?(joshmoz)
Drivers, with the mac patch Neil provided, fixing bug 630839 isn't necessary for polshiing bug 580638.  On very-current-trunk, you can reproduce the bug by hitting cmd+w when an app tab is selected. The file menu will highlight even though this key binding is disabled.
Whiteboard: required for polishing bug 580638 on mac
Attached patch update menu when key is changed (obsolete) — Splinter Review
Fix up case when removing a key and add a test
Attachment #509841 - Attachment is obsolete: true
Attachment #510323 - Attachment is obsolete: true
Attachment #510346 - Flags: review?(joshmoz)
Attachment #510346 - Flags: review?(neil)
Comment on attachment 510346 [details] [diff] [review]
update menu when key is changed

Found a bug in the non-mac part so I'll look into this again.
Attachment #510346 - Flags: review?(neil)
Don't think this blocks, it can be put up for approval, though.
blocking2.0: ? → -
Attachment #510877 - Flags: review?(neil)
Isn't this a regression from bug 190735?
Part of it is, yes.
So why don't we just land bug 592424?
Because it doesn't fix this bug.

As SetAttr is called without notification, the content is correct but the rendering is not. I should probably add a reftest which tests this more specifically.
Comment on attachment 510877 [details] [diff] [review]
fix by making sure acceltext is updated properly

This seems (possibly intentionally) to change the behaviour that setting the acceltext and then the key will overwrite the acceltext, but setting the key and then the acceltext is OK. What if the content has both acceltext and key attributes before the frame is created?

>+  mIgnoreAccelTextChange = PR_TRUE;
>+  mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::acceltext, accelText, aNotify);
>+  mIgnoreAccelTextChange = PR_FALSE;
Isn't this going to get ignored anyway, because the acceltext isn't empty?
Attached patch new patch (obsolete) — Splinter Review
This patch is more like Gavin's patch in the other bug, but with the needed fixes for this one. A reftest is also included.
Attachment #510877 - Attachment is obsolete: true
Attachment #511371 - Flags: review?(neil)
Attachment #510877 - Flags: review?(neil)
Comment on attachment 510346 [details] [diff] [review]
update menu when key is changed

>-    // Set key shortcut and modifiers
>     if (doc) {
>-      nsAutoString keyValue;
>-      mContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::key, keyValue);
>-      if (!keyValue.IsEmpty()) {
>-        nsIContent *keyContent = doc->GetElementById(keyValue);
>-        if (keyContent) {
>-          nsAutoString keyChar(NS_LITERAL_STRING(" "));
>-          keyContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::key, keyChar);
>-
>-          nsAutoString modifiersStr;
>-          keyContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::modifiers, modifiersStr);
>-          PRUint8 modifiers = nsMenuUtilsX::GeckoModifiersForNodeAttribute(modifiersStr);
>-
>-          SetKeyEquiv(modifiers, keyChar);
>-        }
>-      }
>+      SetKeyEquiv(PR_FALSE);
>     }

Why are you leaving the check for "doc" here?

>+void nsMenuItemX::SetKeyEquiv(PRBool aRemoveExisting)

I don't like this argument name much, it isn't very clear what it does. I'm also not sure that it is necessary. If the argument is true then it'll remove the native key equiv if there is no key equiv in the DOM - but wouldn't we always want to do that? I don't see the point of ever calling SetKeyEquiv(PR_FALSE).
(In reply to comment #18)
> Why are you leaving the check for "doc" here?

I'll move this to where GetCurrentDoc is called.

> I don't like this argument name much, it isn't very clear what it does. I'm
> also not sure that it is necessary.

It was a minor optimization. But I'll just remove the argument.
Comment on attachment 511371 [details] [diff] [review]
new patch

>   // Now we're going to compute the accelerator text, so remember that we did.
>   AddStateBits(NS_STATE_ACCELTEXT_IS_DERIVED);
> 
>   // If anything below fails, just leave the accelerator text blank.
>   nsWeakFrame weakFrame(this);
>-  mContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::acceltext, PR_FALSE);
>+  mContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::acceltext, aNotify);
>   ENSURE_TRUE(weakFrame.IsAlive());
...
>+  mIgnoreAccelTextChange = PR_TRUE;
>+  mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::acceltext, accelText, aNotify);
>+  mIgnoreAccelTextChange = PR_FALSE;
You need to double-check the weak frame here first. r=me (on the non-cocoa parts, obviously) with that fixed. But since mIgnoreAccelTextChange exists to avoid resetting the acceltext state bit, I thought another approach would be to defer setting the bit until after setting the acceltext (still with a weak frame check, of course!) But then I noticed that either way it's possible to game the system by setting the attribute in a mutation event listener, as the menu frame still thinks it's setting its acceltext. So would it be better to reset mIgnoreAccelTextChange in AttributeChanged()? That way you could be sure you were only ignoring exactly one attribute change.
Attachment #511371 - Flags: review?(neil) → review+
Attached patch address comments (obsolete) — Splinter Review
Attachment #510346 - Attachment is obsolete: true
Attachment #511371 - Attachment is obsolete: true
Attachment #511883 - Flags: review?(joshmoz)
Attachment #510346 - Flags: review?(joshmoz)
Comment on attachment 511883 [details] [diff] [review]
address comments

Is this the right patch? I'm not sure this would even compile.

>+  void SetKeyEquiv(PRBool aRemoveExisting);

You forgot to remove the argument from the header.

>+  if (aRemoveExisting) {

This variable doesn't exist.
Attachment #511883 - Flags: review?(joshmoz) → review-
Attached patch the right patch (obsolete) — Splinter Review
Attachment #511883 - Attachment is obsolete: true
Attachment #511999 - Flags: review?(joshmoz)
Comment on attachment 511999 [details] [diff] [review]
the right patch

I don't think this is the right patch either, this lacks the Cocoa changes entirely.
Attachment #511999 - Flags: review?(joshmoz) → review-
Attachment #511999 - Attachment is obsolete: true
Attachment #512029 - Flags: review?(joshmoz)
Attachment #512029 - Flags: review?(joshmoz) → review+
Attachment #512029 - Flags: approval2.0?
Comment on attachment 512029 [details] [diff] [review]
really the right patch, hopefully

My understand is that we have a (hacky) workaround for this that landed with the original fix, I don't think we need to land the cleanup for 4.0
Attachment #512029 - Flags: approval2.0? → approval2.0-
(In reply to comment #26)
> Comment on attachment 512029 [details] [diff] [review]
> really the right patch, hopefully
> 
> My understand is that we have a (hacky) workaround for this that landed with
> the original fix, I don't think we need to land the cleanup for 4.0

The workaround introduces a pretty ugly bug on mac: the file menu highlights for cmd+w when it's disabled. Re-nominating.
Attachment #512029 - Flags: approval2.0- → approval2.0?
Keywords: regression
Summary: "key" attribute changes to menuitems are not handled (anymore?) → "key" attribute changes to menuitems are not handled
That's a pretty minor issue. I don't think we need to rework all the code this patch touches to address it this late in the game.
Comment on attachment 512029 [details] [diff] [review]
really the right patch, hopefully

agree w/gavin
Attachment #512029 - Flags: approval2.0? → approval2.0-
http://hg.mozilla.org/mozilla-central/rev/561a1d421cec
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.