52895 - drop down menus > choice all show font sizing menu (Mac)

Status: VERIFIED FIXED

(SeaMonkey :: General, defect, P1)

Description

[John Morrison]

This is restart http://bugzilla.mozilla.org/show_bug.cgi?id=52809, which 
was incorrectly moved to Bugscape. [You can't reopen bugs that have been 
moved].

This bug is apparent in mozilla builds. Here are some comments from original 
bug and from the bugscape bug.

-----------------------------------------------------------------------

commercial build 2000-09-15-04-M18

- use drop down menus to do anything.
- got to > new menu 

tested results : opens with font sizing menu.
expected result...each menu should open with it's proper selections



------- Additional Comments From R.K.Aa 2000-09-15 10:45 -------

hmm not sure i understand this bug, but it doesn't seem to be in linux
2000-091506 (nightly)



------- Additional Comments From Tracy walker 2000-09-15 10:56 -------

it's just on Mac.  Open a menu drop dwon window...select any choice with > 
(little triangle)  to get furhter selections...when doing this on any >, the 
font scaling selections menu pops up.



------- Additional Comments From Peter Trudelle 2000-09-15 11:22 -------

Yes, the View->TextSize cascading menu is replacing nearly all other cascading
menus,  except for the ones that are coming up blank or munged with the 
contents
of the Help menu.  ->pinkerton.



   ------- Additional Comments From [e-mail] 2000-09-15 20:46 -----

   I am seeing this on the Mozilla 2000091508 build. It was said that this
   was commercial only, but not with definitive proof. I have reproduced this
   on two different Mac's with the _mozilla_ build.

   The submenus only begin to appear with the 'text zoom' menuitems, after
   the 'View' toplevel menu is opened in the browser. After that, all submenus
   are populated with this stuff. And, coincidentally, the code that jag 
   checked in last night to do this 'text zoom' is called 'oncreate' of 
   this 'View' menu. 

   I think someone should try backing out his changes on a Mac Mozilla build, 
   and if that cures the problem, then back this out of the mozilla tree until
   we understand how this is breaking Mac menus (and this is a dogfood bug, as 
   menus become unusable on the Mac right now).

Comment 1

[John Morrison]

Transferring decorative keywords. This is a smoketest blocker.

Comment 2

[Peter Trudelle]

cc'ing reviewer and approver for the suspect checkin.

Comment 3

[Peter "jag" Annema]

I'm looking at this, though I don't quite see how it could be doing this. It
looks like a Mac thing, since it (still) works fine here on Linux. I'm in
#mozilla in case you've got questions.

Comment 4

[Peter "jag" Annema]

And just tested this on Windows myself, works fine.

Has anyone tried this on a more recent Mac build? I'm starting to suspect this
problem lies elsewhere (menus empty? munged with the contents of the Help menu?)
and may have been fixed in the mean time, since the empty menu thing happened on
other platforms too and has subsequently been fixed.

Comment 5

[Martijn Pieters]

I suspect that the Mac menu implementation is broken in this respect; it seems
that when initTextZoomMenu() is called (navigator.js:1121), and the new menu
elements are inserted, the Mac menu goes haywire.

Comment 6

[John Morrison]

I just tried this with the 2000091520 Mozilla Mac build (Sorry, I don't have 
a debug build to test). The problem with Mac menus exists in this build too.

The primary symptom is that once you mouseover the menuitem 'View->Text Size',
all submenus in the browser will now display the submenu for 'Text Size'. That
is "Smaller, Larger, 50% ... 200%, Other".

As Martijn suggests, it may be that the Mac menus are having problems with 
something in the way that the 'Text Size' submenu is created, and the js/xul
is entirely legit (should just work XP), but I know nothing about the way that 
Mac menus are handled so I can't really say.

However, Mac menus are currently broken, whatever the cause. You cannot gain 
access to any submenu's true contents once the bug is triggered.

Comment 7

[John Morrison]

I should say that I don't _know_ that it is the 'Text Size' code that is the 
trigger/cause of these Mac menu problems. I'm just guessing, based on the 
observation that this js/xul was significantly changed last night, and 
coincidentally is now appearing as the content of Mac submenus in today's Mac 
builds.

Comment 8

[Peter "jag" Annema]

Hmmm, well, the code I'm using is used elsewhere in Mozilla so you should be
seeing it there too. In the bookmarks window, open the View menu. This is
created with the same kind of code. Can you see what that does?

Comment 9

[Peter Trudelle]

cc saari.  jag found some suspicious code/comments, from yesterday:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/widget/src/mac&command=DIFF_FRAMESET&file=nsMenu.cpp&rev1=1.81&rev2=1.82&root=/cvsroot

Comment 10

[John Morrison]

This is only in the browser (navigator.xul), and view source windows, as they
are the only ones to pull in navigatorOverlay.xul, and (on the Mac) have a
View menu item.

I agree that nothing leaps out at me about why this bit of code is doing this, 
but it really is. I tried removing the oncreate for the menupopup, and that
meant that "50, ... 200" were not created for every other submenu.

I then just commented out "<menu id="menu_TextZoom" ... </menu>" in navOverlay
and after that, this bug on the Mac went away. 

I moved "<menu id="menu_TextZoom" into the Edit menu, and uncommented it, and
this seems to work fine. So, seems like flakiness with that Menu. 
(This may also be a temporary workaround -- move it to the Edit Menu, but 
someone more Mac-like than me needs to check this out). 

Comment 11

[John Morrison]

Scratch what I just said. Putting it in the Edit menu does not work as a 
workaround (I was on (lack of) drugs).

Comment 12

[John Morrison]

Commenting out looks like the workaround until people can have a deeper look
at where this is going wrong.

Comment 13

[Peter Trudelle]

That sounds perfectly acceptable to me for now.  We need all the other submenus
a hell of a lot more than we need that one.  If we have to, we'll ship N6
without it.

Comment 14

[Mike Pinkerton (not reading bugmail)]

backed out jag's menu until we know why this is a problem. removing all those 
nasty keywords.

Comment 15

[Peter "jag" Annema]

John, can you try removing just the |oncreate="initTextZoomMenu()"| and see what
that does for you?

If that works, could you put that back and then comment out all |setAttribute|s
in function initTextZoomMenu(), except for the "value" one, and see what that
does?

Comment 16

[Doron Rosenberg (IBM)]

making asa qa contact, as he has a mac.

Comment 17

[hirata masakazu]

There may be a workaround since switching to another window resets the menu to 
normal.

Comment 18

[Peter "jag" Annema]

Thanks to lordpixel for being my proxy to a Mac with a recent build.

It turns out that Mac doesn't like it when a <menu> has no value attribute, or
an empty one. So both <menu> and <menu value=""> mess up the UI. <menu value="
"> works though.

Next problem:
Mac doesn't like it when you change a <menu>'s value attribute from JS code. It
places the text in all menu items. Example:
<menu value="View">
  <menupopup
oncreate="document.getElementById('subMenu').setAttribute('value','foo');">
    <menu id="subMenu" value="sub">
      <menupopup>
        <menuitem value="bar"/>
      </menupopup>
    </menu>
  </menupopup>
</menu>

Comment 19

[John Morrison]

Okay, so the problem lies with doing .setAttribute on a <menu>'s 'value'.
The 'View->Text Size' is modified dynamically (at least in the oncreate of
the View menu, and I'm not sure if this is updated in other places (?)).

function updateTextSizeMenuLabel() {
   var textSizeMenu = document.getElementById("menu_TextZoom");
   if (textSizeMenu) {
     textSizeMenu.setAttribute("value", 
                               zoomMenuLabel.replace(/%zoom%/, zoomFactor));
   }
}

If I disable that function and just give <menu id="menu_TextZoom"...></menu>
a static 'value' attribute, then this problem of submenus getting hosed goes
away. 

So, that is what this bug is about for Mike -- the effect of the .setAttribute
on the Mac menus. (Re-adding nsbeta3 for now, but I think that we may just say
"don't do that" should someone try to do this again before NS6. In the longer
run, though, this should be fixed).

Jag. I think that if you reworked your code to not do this update, then we 
could turn this menu back on in the interim (see below).
---



However, I then found out that this text scaling functionality doesn't work
_at_all_ on the Mac (bug 39117). Since that bug is not [nsbeta3+], there is
pretty much zero chance that it will be fixed anytime soon. [I find it sad
that this is broken on Mac, but that's the way it is].

That begs the question: why do we even have 'View->Text Size' in the menu on
Mac if it is known to be broken?  I've filed bug 52969 to address this (I
suppose we need to use platformGlobalOverlay.xul to conditionally overlay this
menu depending on mac/linux/win32).
---



And on a final note, I have filed bug 52968. The askViewZoom.(xul|js|dtd)
files were not included in the most recent win32 build available from 
mozilla.org (2000-09-15-08). I think they were not added to 'jar.mn'.

As an interesting(?) side-effect of the missing XUL (which is supposed to come
up for the menuitem 'View->Text Size->Other...), if you call .openDialog with
a path to a non-existent file and a modal flag, you wind up hanging the whole
window (filed bug 52967, and futured it)).
---

Comment 20

[John Morrison]

Ooops. Didn't see jag's note before I posted mine.

Comment 21

[Peter "jag" Annema]

I can change that... You'll upset mpt though! ;-)

But if we're going to remove this feature for the Mac, I'd rather leave it in.

Comment 22

[John Morrison]

I guess it just depends on the timing between the two options, and I can't 
predict when my suggestion for platform overlays would be done (if at all).

I think, as an interim measure, you can just add a 'value' attribute to the 
<menu> and just disable any call that previously used to update that attribute,
and leave the rest of the code as is, but the choice is yours.

Comment 23

[Peter "jag" Annema]

Easy enough. Patch coming up.

Comment 24

[Peter "jag" Annema]

Attachment: [patch] interim fix

Comment 25

[John Morrison]

The patch looks correct to me. (Sorry no patch on Mac, I am told, but hand
rolling the change works correctly -- the submenu problem no longer occurs
as long as the value attribute is left alone). 

Comment 26

[Tracy Walker [:tracy]]

works fine on mac mozilla build 2000-09-18-04  

marking works for me

Comment 27

[Peter Trudelle]

This is not wfm; changes were explicitly made to the tree for this bug, and more
may be necessary. Please wait for someone to resolve as fixed. reopening.

Comment 28

[:deanis74]

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

Comment 29

[saari (gone)]

I have the correct fix for this. The original xul/js code exposed a case where we 
would delete one of the four golden child menus on mac (mac dynamic menu 
implementation specific things). They should never be deleted.

Taking the bug, need to find a reviewer.

Comment 30

[Brendan Eich [:brendan]]

jag, a=[e-mail] on the interim fix unless saari can get the real fix 
in fast enough.  Saari, what's the outlook?

/be

Comment 31

[saari (gone)]

I have the proper fix. I just need to get it reviewed by pink and a super 
reviewer.

Comment 32

[Peter Trudelle]

nsbeta3+, p2 for M18

Comment 33

[selmer (gone)]

We're assuming this means that no submenus work at all.  If that's the case, 
then we believe this is a P1 stop ship.  If that's not the case, please restate 
the effects of this bug.

Comment 34

[Peter Trudelle]

Restating: current status is that the menu that exposed this bug (and others)
doesn't work on the Mac anyway, and has been pulled, so this defect currently
doesn't show up in the product. This should probably not be P1/++.

Comment 35

[saari (gone)]

Fixed

Comment 36

[saari (gone)]

oops, setting fixed

Comment 37

[Blake Ross]

saari, you checked in the proper fix meaning that the menu can be re-enabled 
again without causing problems on mac?  Or am I confused?

Comment 38

[saari (gone)]

Yes, it would work on the Mac now. I wasn't sure what the plans for the feature 
were, so I left it out for now.

Comment 39

[John Morrison]

verified fixed. When I uncomment the menu entry, with 2000092212 mac os9.04,
I no longer get the submenu populating all the other submenus. 

I have filed bug 53879 for this menu to be re-enabled, and if we are planning
to ship with this feature enabled (and we are) then we should get it out there
in the beta to see if any problems arise with the layout back-end code for this
feature. (Pros/cons to that bug).