Closed Bug 417483 Opened 12 years ago Closed 10 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: martijn.martijn, Assigned: dev)

References

(Depends on 1 open bug)

Details

(Keywords: polish, testcase, verified1.9.2)

Attachments

(2 files, 3 obsolete files)

Attached file 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.
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?
I think the extra separator should be hidden, since the Search context menu item is hidden, because searching for " " doesn't make sense.
Attached patch v1 (obsolete) — Splinter Review
Attachment #303276 - Flags: review?(martijn.martijn)
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.
Attachment #303276 - Flags: review?(martijn.martijn) → review?(gavin.sharp)
Assignee: nobody → dev
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: PC → All
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 ).
Attached patch v1 + browser chrome test (obsolete) — Splinter Review
Browser Chrome test is based on Martijn's original test case (303264).
Attachment #303276 - Attachment is obsolete: true
Attachment #303536 - Flags: review?(gavin.sharp)
Attachment #303276 - Flags: review?(gavin.sharp)
Related to bug 303898?
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..
Attached patch v1.2 (obsolete) — Splinter Review
Same patch, fixed typo in Makefile.in.
Attachment #303536 - Attachment is obsolete: true
Attachment #303855 - Flags: review?(gavin.sharp)
Attachment #303536 - Flags: review?(gavin.sharp)
Flags: blocking-firefox3?
Summary: (Weird) Gmail Broken Context Menu Bug! → Double separator in context menu when selecting empty text & using iframes
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Keywords: polish
Whiteboard: [has patch][needs review gavin][fixes bug 303898]
Flags: wanted-firefox3.1?
Flags: blocking-firefox3.1?
Flags: wanted-firefox3.1?
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
ermm, can/will this be reviewed in 3.1 timeframe?
i'm just asking because the patch is "just" 9 months old.
Attachment #303855 - Flags: review?(mconnor)
Attachment #303855 - Flags: review?(gavin.sharp)
Attachment #303855 - Flags: review?(gavin.sharp)
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!
Attachment #303855 - Flags: review?(mconnor)
Attachment #303855 - Flags: review?(gavin.sharp)
Attachment #303855 - Flags: review+
Attached patch Unrotten v1.3Splinter Review
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.
Attachment #303855 - Attachment is obsolete: true
Whiteboard: [has patch][needs review gavin][fixes bug 303898] → [has patch][fixes bug 303898]
Comment on attachment 398733 [details] [diff] [review]
Unrotten v1.3

Should have carried review forward, here.
Attachment #398733 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/e03b6c34cee3

Thanks, Michael.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][fixes bug 303898] → [fixes bug 303898]
Target Milestone: --- → Firefox 3.7a1
Backed out - http://hg.mozilla.org/mozilla-central/rev/7116821311ca
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Re-landed - http://hg.mozilla.org/mozilla-central/rev/ecff3506755f
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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+.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Whiteboard: [fixes bug 303898]
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
Keywords: verified1.9.2
Blocks: 655842
Depends on: 1093155
You need to log in before you can comment on or make changes to this bug.