66120 - Menu items should flash when selected

Status: RESOLVED WORKSFORME

(Core Graveyard :: Widget: Mac, defect)

Description

[Matthew T (active 1999-2002)]

Build: 2001011912, Mac OS 9.0

To reproduce:
* Select any item from an XP Toolkit menu (e.g. the Bookmarks menu in the Personal
  Toolbar, or any context menu).

What should happen:
* The selected item flashes three times, then the menu disappears.

What actually happens:
* The menu disappears.

Comment 1

[Håkan Waara]

It should only flash as many times as is specified in the preferences in MacOS.
Also, this shouldn't be Mac System 9.x but rather Mac System 7, since menu's
flashing there too.

OS -> Mac System 7

Comment 2

[Håkan Waara]

By the way, if this will be fixed. Then bug 59797 should also be fixed, which is
about common mac-menu behaviour (as this one).

Should we follow the mac menu behaviour in mac and thus being inconsistent in
the XPinterface behaviour?

Comment 3

[Mike Pinkerton (not reading bugmail)]

--> later

Comment 4

[Marcia Knous [:marcia]]

nominating for dogfood (from sdagley's list of bugs that are good candidates for 
our next release) 

Comment 5

[timeless]

see also Bug 50590

Comment 6

[:deanis74]

I was bored and the basketball game wasn't interesting me.  I've got something 
hooked up for this in my tree.  It does it everytime a menu item is executed, 
whether it be with the mouse or the keyboard.  Is that the correct behavior?

Comment 7

[:deanis74]

Oh, but just the basic flashing.  I don't have this hooked up to any system 
setting.  Ideally the number of times to flash and the delay would come from 
nsILookAndFeel instead of being encased in #ifdefs.  If there's no flashing or 
delay, the menu gets executed immediately.

Comment 8

[:deanis74]

I hooked this up to nsILookAndFeel.  Patches coming for critique.  Could be the 
complete wrong way of doing this, but hopefully it's at least a start.

Comment 9

[:deanis74]

Attachment: nsILookAndFeel.h

Comment 10

[:deanis74]

Attachment: nsMenuFrame.cpp and .h -- Note also that I changed Execute() to ExecuteMenu().

Comment 11

[Matthew T (active 1999-2002)]

Cool!

> It does it everytime a menu item is executed, whether it be with the mouse or
> the keyboard.  Is that the correct behavior?

Well ... Menu items other than those in the top-level menu bar generally don't 
respond to the keyboard -- and the top-level menu bar is native, so it has this 
behavior already. But I don't see the need to special-case the keyboard out of 
this behavior; if there are XP menu items anywhere in Mozilla which can be 
activated from the keyboard in Mac OS, that's probably a UI bug.

Comment 12

[:deanis74]

I'd be able to write code that's a lot more straight-forward if I had something 
like an xp Sleep() function, instead of using timers.  Is there such a thing?

Comment 13

[:deanis74]

Gee, maybe PR_Sleep()?  Whole new patch coming in a bit.

Comment 14

[:deanis74]

Hrm... maybe not.  SelectMenu() doesn't seem to have any effect when I'm using 
PR_Sleep().  It does when I do it in a timer, though.  Crappy.

Comment 15

[:deanis74]

Ugh.  Neither does mMenuParent->SetCurrentMenuItem(nsnull).  I must be missing 
something obvious.

Comment 16

[Mike Pinkerton (not reading bugmail)]

use a timer ;)

Comment 17

[:deanis74]

OK, then.  How's the code in my patch look, then?  I was thinking of adding code
to not execute the menu if it's on the menu bar, for that bug about flashing the
appropriate menu when a shortcut key is pressed.

Comment 18

[Aaron Leventhal]

Flashing menu items isn't good for access, it's actually a bad thing. Have an
option to turn it off please.

Comment 19

[Håkan Waara]

It should only flash on mac when that pref in the OS is set, so there is already
an "option to turn it off".

Comment 20

[:deanis74]

Correct.  That's where nsILookAndFeel comes in.

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

where does widget obtain a value from the OS for this particular metric? I don't
see code that does it...

Comment 22

[:deanis74]

No idea.  I was going to leave that up to someone with a make to add
eMetric_MenuSelectionFlashCount and eMetric_MenuSelectionFlashDelay to Mac's
nsLookAndFeel.cpp.

Comment 23

[:deanis74]

I've tried to get an answer from hyatt as to why SelectMenu() doesn't update
when it's not called within a loop.  Anyone have any ideas?  Here's my thoughts...

I need SetAttribute to take effect, because I need to use GetAttribute to
retrieve it the next time through my loop.  I tried using both
nsFrame::Invalidate and nsIPresShell::FlushPendingNotifications to force the
SetAttribute to take effect, but it's not working.  Is there a different
FlushPendingNotifications that I should be calling?  nsIPresShell is used
elsewhere in nsMenuFrame.cpp.

  for (i = 0; i < aFlashCount * 2; i++) {
    // aFlashCount is in milliseconds
    PR_Sleep(aFlashDelay);

    mContent->GetAttribute(kNameSpaceID_None, nsXULAtoms::menuactive,
active);
    if (active.Equals(NS_LITERAL_STRING("true")))
        ^^^^^^^^^^^^^^^^^^^^^ this is "true" everytime through the loop
      // Menu is highlighted - unhighlight it
      SelectMenu(PR_FALSE);
      ^^^^^^^^^^^^^^^^^^^^^ this should set the attribute to "false"
    else
      // Menu is not hightlighted - highlight it
      SelectMenu(PR_TRUE);

    // force the SetAttribute(), in SelectMenu(), to take effect
    // XXX this doesn't work
    nsCOMPtr<nsIPresShell> shell;
    mPresContext->GetShell(getter_AddRefs(shell));
    shell->FlushPendingNotifications();
  }

Comment 24

[timeless]

[the first version of macos that mozilla supports is 8.5]

Comment 25

[Mike Pinkerton (not reading bugmail)]

fffffffuture

Comment 26

[Jo Hermans]

Since we don't do development for Mac OS 9 or earlier anymore, and this
behaviour is not valid anymore in Mac OS X (there are no falshes anymore),
should we change this to WONTFIX or WORKSFORME ?

Comment 27

[Jo Hermans]

Sorry, I was a bit wrong. What I wanted to say is that it isn't possible anymore
to configure them in OS X, so people might be less annoyed when the flashing
isn't exactly like they configured themselves..

There are still flashes, but much faster. But they're often invisible, for
instance when trying to switch between the different views in the System
Preferences. It looks to me that it's related to the /size/ of the pulldownmenu.
Dissolving the menu in the background takes a long time for me (iBook 300 Mhz
with simple Rage video), so it's possible that this makes the flashing invisible
(when flashing is faster than the actual menu dissolve). Since I've never read
about this, I don't think that anyone cares anymore.

Comment 28

[Simon Paquet [:sipaq]]

This bug is targeted at a Mac classic platform/OS, which is no longer supported
by mozilla.org. Please re-target it to another platform/OS if this bug applies
there as well or resolve this bug.

I will resolve this bug as WONTFIX in four weeks if no action has been taken.
To filter this and similar messages out, please filter for "mac_cla_reorg".

Comment 29

[Mark Mentovai]

*** Bug 341179 has been marked as a duplicate of this bug. ***

Comment 30

[Mark Mentovai]

mumble SetMenuFlashCount mumble

Comment 31

[Mark Mentovai]

oops, this isn't about native menus

Comment 32

[Samuel Sidler (old account; do not CC)]

Has this been fixed with the move to Cocoa?

Comment 33

[Håkan Waara]

(In reply to comment #32)
> Has this been fixed with the move to Cocoa?

This bug was pretty OS 9-specific, but yeah - menuitems definitely flash as they should AFAICS in the new Cocoa world.