466234 - Content popup menu is overly long

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Peter Weilbacher]

Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.1b2pre) Gecko/20081118 SeaMonkey/2.0a2pre

We haven't had an OS/2 nightly in a while so I cannot tell when this started, but this build shows an overly long popup menu on most content. Seems like 40 entries to me and it even shows scroll arrows at top and bottom to show them all.

When right-clicking on the content area to get this menu, I see the following three errors on the JS console:

Error: HTMLVideoElement is not defined
Source File: chrome://communicator/content/nsContextMenu.js
Line: 403
Error: gContextMenu is null
Source File: chrome://navigator/content/mailNavigatorOverlay.xul
Line: 138
Error: gContextMenu is null
Source File: chrome://editor/content/editorApplicationOverlay.js
Line: 42

The first reminds me that the OS/2 builds are currently done with
   --disable-ogg
   --disable-wave
so MOZ_MEDIA is deactivated. Perhaps that needs to be tested for somewhere?

Comment 1

[Peter Weilbacher]

I get an additional error when selecting "Open in new tab" from the context menu:

Error: gContextMenu is null
Source File: chrome://navigator/content/navigator.xul
Line: 1

I suspect that this is linked to the problem.

Comment 2

[Peter Weilbacher]

Firefox actually has the same problem and I suspect Thunderbird, too.

Preprocessing contextMenu.js to exclude the lines about the HTMLVideo and HTMLAudio classes using #ifdef MOZ_MEDIA should help.

Comment 3

[Peter Weilbacher]

Attachment: fix for SeaMonkey

Of course I was wrong before, TB doesn't need a fix because it doesn't use HTML{Video,Audio} in its code (yet). So this is the whole fix for comm-central.

Comment 4

[Peter Weilbacher]

Attachment: fix for Firefox

Same fix for Firefox.

Comment 5

[[e-mail]]

roc, should these --disable options in comment #0 stop the audio and video elements from existing too, or is that a bug?

Comment 6

[Peter Weilbacher]

That building without MOZ_MEDIA also disables compiling of nsHTML{Audio,Video,Media}Element.cpp seems to be on purpose, see <http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/Makefile.in#138>.
It's just weird that there is no --disable-media flag.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #5)
> roc, should these --disable options in comment #0 stop the audio and video
> elements from existing too, or is that a bug?

That's intentional, although I'm open to arguments that it should be changed.

Comment 8

[[e-mail]]

Actually would it be enough to provide the audio and video interfaces, even when not actually supporting the elements?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In this case, perhaps yes.

Comment 10

[[e-mail]]

Attachment: Like this?

Peter, I haven't actually tried building without media so I hope it works...

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 349719 [details] [diff] [review]
Like this?

Something like that, if it builds

Comment 12

[Peter Weilbacher]

Comment on attachment 349719 [details] [diff] [review]
Like this?

Yes, it does compile and also gives me back the working menu. Thanks, Neil.

Comment 13

[[e-mail]]

Comment on attachment 349719 [details] [diff] [review]
Like this?

This stops people building without A/V for whatever reason from getting a broken UI in Firefox or SeaMonkey (or even, given bug 463155, Thunderbird).

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 349719 [details] [diff] [review]
Like this?

a191=beltzner

Comment 15

[Peter Weilbacher]

Attachment: for metered checkin

Because I want this in soon, I made this version with the header to comply with the metered checkin rules.

Comment 16

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/b32dc52a37ff

Comment 17

[[e-mail]]

Looks like I pushed this as changeset b32dc52a37ff on releases/mozila-1.9.1 a few months ago; sorry for not updating the bug.

Comment 18

[Peter Weilbacher]

And in the meantime I verified that it is fixed on trunk and 1.9.1 branch.