355375 - Context menuitem doesn't blink when chosen

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Håkan Waara]

When a menuitem in a contextmenu (or popupmenu) is chosen, it should blink just like in the menubar.

Comment 1

[Håkan Waara]

Would this be fixed in XUL code rather than widget/cocoa?

Comment 3

[Markus Stange [:mstange]]

Attachment: v1

Comment 4

[Markus Stange [:mstange]]

Hmm, this also makes items in editable menulists blink. Neil, what would be a good way to prevent that? Checking for the "editable" attribute directly in nsMenuFrame.cpp feels a little hackish to me.

Comment 5

[Neil Deakin]

Comment on attachment 412400 [details] [diff] [review]
v1

>+static PRBool GetBlinkOffDelay()
>+{
>+  PRInt32 blinkOffDelay = 0;
>+  nsCOMPtr<nsILookAndFeel> lookAndFeel(do_GetService(kLookAndFeelCID));
>+  if (lookAndFeel) {
>+    lookAndFeel->GetMetric(nsILookAndFeel::eMetric_MenuItemBlinkOffDelay, blinkOffDelay);
>+  }
>+  return blinkOffDelay;
>+}
>+
>+static PRBool GetBlinkOnDelay()
>+{
>+  PRInt32 blinkOnDelay = 0;
>+  nsCOMPtr<nsILookAndFeel> lookAndFeel(do_GetService(kLookAndFeelCID));
>+  if (lookAndFeel) {
>+    lookAndFeel->GetMetric(nsILookAndFeel::eMetric_MenuItemBlinkOnDelay, blinkOnDelay);
>+  }
>+  return blinkOnDelay;
>+}

These should return PRInt32. But I don't think these functions are necessary. The blink delays
are hard-coded, so I see no reason to add more work by looking up the values in nsILookAndFeel;
just define some constants right in nsMenuFrame.cpp.


>+      case 1:
>+        // Turn the highlight back on and wait for a while before closing the menu.
>+        mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::menuactive,
>+                          NS_LITERAL_STRING("true"), PR_TRUE);
>+        aTimer->InitWithCallback(mTimerMediator, GetBlinkOnDelay(), nsITimer::TYPE_ONE_SHOT);

The call to SetAttr could destroy the frame, and accessing mTimerMediator would crash.

 
>+
>+  if (!ShouldBlink()) {
>+    PassMenuCommandEventToPopupManager();
>+    return;

This early return never deletes mDelayedMenuCommandEvent.


>+  }
>+
>+  // Blink off.
>+  mContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::menuactive, PR_TRUE);
>+
>+  if (mMenuParent) {

Similarly, here, as this could crash.


>+
>+  // Returns true between a call to LockMenuUntilClosed() and the next time it's
>+  // opened.
>+  virtual PRBool IsMenuLocked() = 0;

did you mean "until it's closed?"

Not sure I like the term 'lock'. It may be clearer to just call this IsMenuBlinking and similar names for other functions.


>@@ -1951,18 +1929,21 @@ nsXULPopupManager::KeyPress(nsIDOMEvent*
> 
>   if (!trustedEvent)
>     return NS_OK;
> 
>   nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
>   PRUint32 theChar;
>   keyEvent->GetKeyCode(&theChar);
> 
>+  nsMenuChainItem* item = GetTopVisibleMenu();
>+  if (item && item->Frame()->IsMenuLocked())
>+    return NS_OK;
>+

This could be done before the GetKeyCode check.

Do you need to check the lock/blink state during the KeyDown event as well?


>+    case eMetric_ChosenMenuItemsShouldBlink:
>+      aMetric = 1;
>+      break;
>+    case eMetric_MenuItemBlinkOffDelay:
>+      aMetric = 4 * 1000 / 60; // 4 frames
>+      break;
>+    case eMetric_MenuItemBlinkOnDelay:
>+      aMetric = 6 * 1000 / 60; // 6 frames

what is 'frames' referring to here?


Menu locking only seems to handle some popup/menu interactions. For instance, a direct call could occur to
open or close the popup during the blink. What happens in these cases?

What happens when a menuitem is activated, and the mouse moves over a parent menu which could close the child menu? Only the child menu is locked, so things could happen to the parent, no?

I think there's a lot of potential odd interactions that could happen that need to be examined, if you haven't done already. One more drastic possibility is to just lock the entire menu ui during a blink.

But the blinking itself under normal circumstances works great.


> Checking for the "editable" attribute directly in nsMenuFrame.cpp feels a little hackish to me.

It should call nsIDOMXULMenuListElement::GetEditable on the parent menu.

Comment 6

[Markus Stange [:mstange]]

Attachment: v2

(In reply to comment #5)
> (From update of attachment 412400 [details] [diff] [review])
> But I don't think these functions are necessary.
> The blink delays
> are hard-coded, so I see no reason to add more work by looking up the values in
> nsILookAndFeel;
> just define some constants right in nsMenuFrame.cpp.

Done.

> >+      case 1:
> >+        // Turn the highlight back on and wait for a while before closing the menu.
> >+        mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::menuactive,
> >+                          NS_LITERAL_STRING("true"), PR_TRUE);
> >+        aTimer->InitWithCallback(mTimerMediator, GetBlinkOnDelay(), nsITimer::TYPE_ONE_SHOT);
> 
> The call to SetAttr could destroy the frame, and accessing mTimerMediator would
> crash.

fixed + added test

> >+
> >+  if (!ShouldBlink()) {
> >+    PassMenuCommandEventToPopupManager();
> >+    return;
> 
> This early return never deletes mDelayedMenuCommandEvent.

Are you sure? I think it gets deleted by the nsCOMPtr that takes over ownership in nsXULPopupManager::ExecuteMenu.
If that's not obvious enough, what would be a better way to do this?


> >+  }
> >+
> >+  // Blink off.
> >+  mContent->UnsetAttr(kNameSpaceID_None, nsGkAtoms::menuactive, PR_TRUE);
> >+
> >+  if (mMenuParent) {
> 
> Similarly, here, as this could crash.

fixed + added test

> >+
> >+  // Returns true between a call to LockMenuUntilClosed() and the next time it's
> >+  // opened.
> >+  virtual PRBool IsMenuLocked() = 0;
> 
> did you mean "until it's closed?"

Well, I wanted to make the comment reflect reality, since I was resetting mIsMenuLocked in nsMenuPopupFrame::ShowPopup... but doing so in nsMenuPopupFrame::HidePopup makes more sense, so I'm doing that now and I've changed the comment.

> Not sure I like the term 'lock'. It may be clearer to just call this
> IsMenuBlinking and similar names for other functions.

I'd like to make use of this locking functionality when I implement fading in bug 406997, too, and I don't really like IsMenuBlinkingOrFading or IsMenuDoingCloseAnimation.

> >@@ -1951,18 +1929,21 @@ nsXULPopupManager::KeyPress(nsIDOMEvent*
> > 
> >   if (!trustedEvent)
> >     return NS_OK;
> > 
> >   nsCOMPtr<nsIDOMKeyEvent> keyEvent = do_QueryInterface(aKeyEvent);
> >   PRUint32 theChar;
> >   keyEvent->GetKeyCode(&theChar);
> > 
> >+  nsMenuChainItem* item = GetTopVisibleMenu();
> >+  if (item && item->Frame()->IsMenuLocked())
> >+    return NS_OK;
> >+
> 
> This could be done before the GetKeyCode check.

Done.

> Do you need to check the lock/blink state during the KeyDown event as well?

Probably; I've added it, just to be safe.

> >+    case eMetric_ChosenMenuItemsShouldBlink:
> >+      aMetric = 1;
> >+      break;
> >+    case eMetric_MenuItemBlinkOffDelay:
> >+      aMetric = 4 * 1000 / 60; // 4 frames
> >+      break;
> >+    case eMetric_MenuItemBlinkOnDelay:
> >+      aMetric = 6 * 1000 / 60; // 6 frames
> 
> what is 'frames' referring to here?

as in 60 fps (frames per second, refresh rate)... which I admit doesn't make any sense.

> Menu locking only seems to handle some popup/menu interactions. For instance, a
> direct call could occur to
> open or close the popup during the blink. What happens in these cases?

The menu closes as it was told and the menu item is executed sometime later, at the time when the blinking would have finished.
This means that the command might be executed after popuphiding and popuphidden have fired. But that's not really a new problem, since the nsXULMenuCommandEvent is already asynchronous.

> What happens when a menuitem is activated, and the mouse moves over a parent
> menu which could close the child menu? Only the child menu is locked, so things
> could happen to the parent, no?

Right.
Therefore I've made changes to LockMenuUntilClosed(): It now locks all its parent menus, too. I've tested that this doesn't break context menus opened on top of menus (by disabling bug 503639).

> I think there's a lot of potential odd interactions that could happen that need
> to be examined, if you haven't done already.

I've thought about some, but I can't really think of anything serious. I don't see the need to lock the whole menu system.
The only thing that (I think) could and indeed does cause problems is the fact that the menu no longer closes synchronously in response to a mouseup event.
Mac context menus on background windows depend on this in a broken way; I'm going to fix it in another bug.

> It should call nsIDOMXULMenuListElement::GetEditable on the parent menu.

Done.

Comment 7

[Markus Stange [:mstange]]

(In reply to comment #6)
> Mac context menus on background windows depend on this in a broken way; I'm
> going to fix it in another bug.

bug 529891

Comment 8

[Markus Stange [:mstange]]

Attachment: v2

updated to trunk

Comment 9

[Neil Deakin]

Comment on attachment 425965 [details] [diff] [review]
v2

>@@ -892,16 +900,36 @@ nsMenuFrame::Notify(nsITimer* aTimer)
...
>+  } else if (aTimer == mBlinkTimer) {
>+    nsWeakFrame weakFrame(this);

Move this declaration into the 'case 1' block below it.

>+function test_crash(when, andThen) {
...
>+          if (e.attrChange == e[when]) {
>+            menuitem.style.setProperty("display", "none", "");

menuitem.style.display = 'none' or menuitem.hidden = true, are clearer. Same with the removal below.


>+  }, false);
>+  menupopup.openPopup(null, "after_start", 10, 10, false, false);

menulist.open = true is simpler of course.


> Are you sure? I think it gets deleted by the nsCOMPtr that takes over ownership
> in nsXULPopupManager::ExecuteMenu.

But ExecuteMenu isn't always called.


Nothing seems to clear the popup locked state when a popup ends up not being closed. When the closemenu attribute prevents it for instance, or a popuphiding event that later fires is cancelled?

Comment 10

[Markus Stange [:mstange]]

Attachment: v3

(In reply to comment #9)
> (From update of attachment 425965 [details] [diff] [review])
> >@@ -892,16 +900,36 @@ nsMenuFrame::Notify(nsITimer* aTimer)
> ...
> >+  } else if (aTimer == mBlinkTimer) {
> >+    nsWeakFrame weakFrame(this);
> 
> Move this declaration into the 'case 1' block below it.

Done. I hadn't done it because I wanted to avoid one more level of indentation and braces.

> >+function test_crash(when, andThen) {
> ...
> >+          if (e.attrChange == e[when]) {
> >+            menuitem.style.setProperty("display", "none", "");
> 
> menuitem.style.display = 'none' or menuitem.hidden = true, are clearer. Same
> with the removal below.

changed to .hidden = true

> >+  }, false);
> >+  menupopup.openPopup(null, "after_start", 10, 10, false, false);
> 
> menulist.open = true is simpler of course.

Done.

> > Are you sure? I think it gets deleted by the nsCOMPtr that takes over ownership
> > in nsXULPopupManager::ExecuteMenu.
> 
> But ExecuteMenu isn't always called.

I've put mDelayedMenuCommandEvent into an nsRefPtr and now also null it when not calling ExecuteMenu.

> Nothing seems to clear the popup locked state when a popup ends up not being
> closed. When the closemenu attribute prevents it for instance, or a popuphiding
> event that later fires is cancelled?

I've added a boolean param to LockMenuUntilClosed and unlock it after blinking.

Comment 11

[Neil Deakin]

Comment on attachment 438977 [details] [diff] [review]
v3

It would be nice to also have a test for when the popup hiding is cancelled, but that can be a followup bug.

Comment 12

[Markus Stange [:mstange]]

Note to self: I need to disable the test on Win / Linux before landing.

Comment 13

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/4acb7242eb1a