52969 - Need to not show 'View->Text Size' menu on the Mac

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P2)

Description

[John Morrison]

See (bug 39117). The text scaling functionality doesn't work _at_all_
on the Mac, and it looks like this will not be fixed for NS6 (that bug
is not [nsbeta3+] at this time).

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 suppose we need to use
platformGlobalOverlay.xul to conditionally overlay this menu depending
on mac/linux/win32.

So this bug is to rework the XUL/JS/DTD for this menuitem so that it
is available on a platform by platform basis. 

Peter 'jag' Annema ([e-mail]) has recently rewritten most of
this stuff so it would be best to coordinate this work with him.

nom. nsbeta3: there is no point in shipping a menu on Mac for a
feature that is known not to work on Mac.

Comment 1

[Peter "jag" Annema]

Okay, that can be done, but I suggest creating a new file called
platformViewOverlay.xul (.js, .dtd), or something like that. Doing that should
be fairly straightforward.

Can we put those files in xpfe/browser/resources/content/ (and locale/en-US/),
add them to unix/jar.mn and win/jar.mn, then put an "empty" version of those in
mac/ and refer to that in mac/jar.mn?

Comment 2

[John Morrison]

I'll leave the particulars of how to do it up to people who know better (i.e.,
not me :-]). It does seem like a good thing not to have two implementations
of the same overlay, when the code in this case is identical for win32 and 
linux (but an overlay with no elements on mac).

Comment 3

[Henri Sivonen (:hsivonen)]

Triage team: Please consider plussing bug 39117 instead of this one if at all possible.

Comment 4

[Phil Peterson]

nsbeta3+ to fix either 39117 or this one.

Comment 5

[don]

Well, I guess I wait on bug #39117 ...

Comment 6

[Peter "jag" Annema]

Another option, if bug 39117 doesn't get fixed in time, is to hide the menuitem
in initViewMenu() when we're on a Mac, or initially hide it and only turn it on
when we're not. See navigator.js, function EnableBusyCursor(doEnable) for an
example of "not a Mac" code. This is easier to undo than using platform
overlays, which also signal a "by design" solution, which this really isn't.

Comment 7

[Jim Roskind]

If this bug is waiting on 39117, please contact the bug owner and manager and
get that bug triaged.  We need to understand complexity of the bug that your
depending on, and that should be reflected in a triage marking.
Adding PDTP2

Comment 8

[John Morrison]

I have updated bug 39117 to ask for a nsbeta3(+|-) decision, but perhaps
Don would like to contact the Layout folks more directly.

Comment 9

[don]

Guess we wait until RTM for this ...

Comment 10

[johng]

nav triage team:
we don't ahve a patch yet, so [RTM NEED INFO] to get the patch.

Comment 11

[John Morrison]

Note that karnaze gave bug 39117 an rtm+ today. If that bug is fixed, then
this bug is moot.

Comment 12

[don]

This is apparently fixed on the branch, i.e. the menu is no longer there.  See
bug #53207 for my detailed rant on the subject.  And since we have bug #53207
now, do we want to close this one as FIXED?

Comment 13

[Peter "jag" Annema]

It is not, see bug 53207 for a recap/explanation :-)

Comment 14

[Michael La Guardia]

We just minused 39117.  Please let PDT know when you have a reviewed patch for
this bug.

Comment 15

[don]

Sinc the PDT has just "minused" bug #39117, we need to disable this menu on the
Mac.  Bug #53207 can deal with enabling for the other platforms.

Comment 16

[don]

Ben, we need a fix for this at the same time as the fix for bug #53207.

Comment 17

[Peter "jag" Annema]

I'll take this. Preferred solution? JS runtime check for Mac (minimal change),
or per platform version of textZoomOverlay.xul,.js (move xul and js out into per
platform files, with Mac having "do nothing, don't show" equivalents)?

Comment 18

[Matthew T (active 1999-2002)]

The first solution would seem to be the easiest, both to implement, and to 
reverse once bug 39117 is fixed.

-- mpt (oh, the irony of it all ...)

Comment 19

[don]

OK, jag.  That seems reasonable.  Run with it ...

Comment 20

[Peter "jag" Annema]

Attachment: [patch] Hide text size menu and only unhide it on platform != mac

Comment 21

[Peter "jag" Annema]

Patch attached, keywords added. Alternatively, I could remove the menu instead
of simply hiding it. Feel free to hack :-)

Comment 22

[don]

jag, Cool.  Now pester people to get this reviewed and approved.  Once that's
done, I'll "rtm+" it again and talk to the PDT about making it "rtm++".  So you
can check it in.

Comment 23

[Peter "jag" Annema]

Btw, if you're going to test this, you'll need to have the menu enabled again
(doh), see bug 53207 for that patch.

Comment 24

[Peter "jag" Annema]

r=pinkerton, asking ben for a=.

Comment 25

[don]

Not to worry. :-)  Ben will approve this shortly. ...

Comment 26

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

a=ben

Comment 27

[Peter "jag" Annema]

Fix checked in on trunk. gramps, your turn :-)

Comment 28

[don]

PDT, please approve.

(Sorry Peter, I actually didn't work on Saturday :-)

Comment 29

[Michael La Guardia]

Does this relate to 40340?  If that lands does that change the disposition of
this bug?  Marking need info

Comment 30

[John Morrison]

I do not believe that this will change when bug 40340 lands. bug 40340 appears
to be high-level XP code dealing with the style system and pref. values.

This bug is motivated by a mac-specific problem with the implementation of
TextZoom on the Mac (bug 39117), which requires a platform-specific fix at the
device context level (if I'm not mistaken). Marc, can you confirm that your fix
for bug 40340 will not fix bug 39117.

So, assuming that bug 39117 is not going to be fixed, this bug exists to remove
the menu item on the Mac, where it will not (unfortunately) work. (Removing need
info).

Comment 31

[Phil Peterson]

PDT marking [rtm-] because it's time to focus on P1 crash/data loss bugs.

Comment 32

[Jason Eager]

*ahem*. This patch has been available for days now. Marking this rtm- blocks
bug# 53207 which has already been marked rtm++ by the PDT and ALSO has had a
patch available for days now. Allowing the checkin of these two patches would
not require any further work by these developers, and both patches have been
checked in and verified on the trunk.

Not changing this to rtm++ will create a menu for a feature that doesn't work on
mac. Changing 53207 from rtm++ to rtm- would deprive netscape users of yet
ANOTHER useful feature that works well and is highly requested by most users.
Most forums talking about PR3 have several disappointed comments about
View->Text Size disappearing.

This patch has been reviewed and approved and verified in the trunk. The
dependant bug has also been reviewed, approved and verified in the trunk, AND it
has been rtm++'ed. The only reason that it hasn't been checked into the branch
is because the manager would prefer that THIS patch be checked in at the same
time, so macintosh users don't get a menu item that they can't actually use. The
only reason that THIS item didn't get rtm++'ed as well is because another member
of the PDT wanted to know if we could get View->Text Size working on the mac as
well by fixing another bug that was already (I assume) rtm++'ed. The answer came
back negative; fixing that bug (40340) would not make View->Text Size work on
mac, therefore making this patch necessary.

No further work needs to be done on EITHER of these patches, they just need to
be CHECKED IN. If the PDT didn't have a question about 40340, then this bug
would have been rtm++'ed to be consistant with the decision made on 53207.

Therefore, marking this rtm- is horribly inconsistant, and I'm removing the rtm-
to ask for reconsideration.

Comment 33

[Blake Ross]

[e-mail]: PDT won't get to this again unless it's rtm+.  readding that

Comment 34

[Phil Peterson]

pdt:  OK, one day only.  Check in by 4pm Wednesday or the product lockdown cycle
will leave it behind. The standard for fixes gets higher as we get closer to
shipping, so you should not expect the same bug to get the same evaluation,
separated by several days.

Comment 35

[Michael La Guardia]

per phil's comment marking rtm++

Comment 36

[Peter "jag" Annema]

Checked in on branch too, marking fixed.

Comment 37

[sairuh (rarely reading bugmail)]

vrfy: View > Text Size is no longer in the mac branch bits [using
2000.10.11.09-n6 opt comm bits on os 9.0]. although i haven't seen that menu
item on the mac for some time, i contrast this with the fact that it *is* back
in the linux and winnt branch [using 2000.10.11.09-n6 opt comm bits] --but that
of course is the subject of bug 53207, which am gonna vrfy rsn...

Comment 38

[Asa Dotzler [:asa]]

Verified Fixed on trunk builds. Menu is gone.
mac 101804 Mac OS9
Setting bug to Verified and removing vtrunk keyword