417483 - Double separator in context menu when selecting empty text & using iframes

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

Michael posted this in the dev-apps-firefox mailing list:
"
he Gmail context menu gets broken sometimes. No clue as to why! Check
out the following screenshot:
http://cv.schonfeld.org-a.googlepages.com/pic.png

I can't really file it in bugzilla, because I can't figure out how to
reproduce this bug... It just kinda happens form time to time, without
throwing any errors or anything like that...

This is what the context menu should look like:
http://cv.schonfeld.org-a.googlepages.com/pic2.png

Anyone else seen this?
"

I can reproduce this problem.

I've attached a testcase that also reproduces this and I think this is also what is happening at GMail.

It happens when some space for example is selected in a frame, in that case the Search context menu doesn't come up, but the seperator still remains.

Comment 1

[Michael Schonfeld]

Ahh, understandable.

So, should the search context menu be visible? or is it supposed to act like this, and we should just hide the extra separator?

Comment 2

[Martijn Wargers (dead)]

I think the extra separator should be hidden, since the Search context menu item is hidden, because searching for " " doesn't make sense.

Comment 3

[Michael Schonfeld]

Attachment: v1

Comment 4

[Martijn Wargers (dead)]

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

Michael, thanks for the patch!

The patch seems to make sense, but I'm not allowed to review it, so moving off review to someone else.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Is there any chance I could convince you to write some browser chrome tests for the context menu code? I find patches to this code (this one and bug 303898) very hard to review because there are some many combinations of elements to keep track of.

I'm thinking of a test that just sets document.popupNode to various nodes, and then calls the nsContextMenu constructor and checks the menupopup's children to see which appear. I can provide further detail or help you out with the test infrastructure if you're not familiar with it (and there's documentation at http://developer.mozilla.org/en/docs/Browser_chrome_tests ).

Comment 6

[Michael Schonfeld]

Attachment: v1 + browser chrome test

Browser Chrome test is based on Martijn's original test case (303264).

Comment 7

[Martijn Wargers (dead)]

Related to bug 303898?

Comment 8

[Michael Schonfeld]

Yes, definitely related. Applying this patch will solve 303898's problem too.  

Actually If I am not mistaken, 303898's patch became irrelevant when all the showItems were taken out from browser.js to nsContextMenu.js, right?

I think I'll get another browser chrome test, for 303898, while we're waiting for G#'s review..

Comment 9

[Michael Schonfeld]

Attachment: v1.2

Same patch, fixed typo in Makefile.in.

Comment 10

[Michael Schonfeld]

http://www.schonfeld.org-a.googlepages.com/pic.png
http://www.schonfeld.org-a.googlepages.com/pic2.png

the pics from the first post!

Comment 11

[(mostly gone) XtC4UaLL [:xtc4uall]]

ermm, can/will this be reviewed in 3.1 timeframe?
i'm just asking because the patch is "just" 9 months old.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 303855 [details] [diff] [review]
v1.2

>Index: browser/base/content/test/browser_bug417483.js

>+function onPageShow() {

>+    ok(document.getElementById("frame-sep").hidden, "'frame-sep' should be hidden if the selection contains only spaces");

It would be nice to test the opposite behavior too (that things are correct when non-spaces are selected).

Sorry for the delay here!

Comment 13

[Johnathan Nightingale [:johnath]]

Attachment: Unrotten v1.3

Michael, sorry this lingered so long. Gavin pointed it out to me while I was working on the similar-but-different bug 514520, so I figured I'd land them at the same time, once that one gets a final review.  In the meantime, I freshened this one up to current mozilla-central.

When I check in, I'll ensure that you get credit for the patch.

Comment 14

[Johnathan Nightingale [:johnath]]

Comment on attachment 398733 [details] [diff] [review]
Unrotten v1.3

Should have carried review forward, here.

Comment 15

[Johnathan Nightingale [:johnath]]

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

Thanks, Michael.

Comment 16

[Johnathan Nightingale [:johnath]]

Backed out - http://hg.mozilla.org/mozilla-central/rev/7116821311ca

Comment 17

[Johnathan Nightingale [:johnath]]

Re-landed - http://hg.mozilla.org/mozilla-central/rev/ecff3506755f

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on trunk with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090916 Minefield/3.7a1pre ID:20090916030909

The patch contains a test. Marking as in-testsuite+.

Comment 19

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/e5986c504c40

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed on 1.9.2 with builds on all platforms like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20090930 Namoroka/3.6b1pre ID:20090930033826