303898 - Double context menu separator on highlighted image in a sub-frame

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[PikeUK]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b4) Gecko/20050807 Firefox/1.0+

A double separator appears in the context menu when an image from a *sub-frame*
is highlighted.

Steps to reproduce:
1. Visit testcase.
2. Highlight image of ant, be careful not to select any text, only the image.
3. Activate the context menu over the highlighted ant.

Result:
A double separator in the menu.

Regression window:
Works: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050613 Firefox/1.0+
Fails: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050614 Firefox/1.0+

I think it was caused by Bug 280284 but Bug 293758 and Bug 258901 were also
checked in within that timeframe.

Comment 1

[PikeUK]

Attachment: Testcase

Comment 2

[Jesse Ruderman]

Confirmed with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4)
Gecko/20050808 Firefox/1.0+.

Comment 3

[Martijn Wargers (dead)]

Attachment: patch

This fixes it, and I think it doesn't regress anything, but I'm not sure.

Comment 4

[Martijn Wargers (dead)]

I think the fix for bug 280284 regressed this (the most likely of the three).
Maybe there is a better solution, I don't particularly like my patch.
Also, there should be a menuseparator under the "This Frame" menuitem, which
doesn't happen currently with current builds with the testcase (and the patch
doesn't fix that).

Comment 5

[PikeUK]

Attachment: Updated Testcase

This appears to be fixed, can someone confirm that please.

Comment 6

[PikeUK]

This hasn't been a problem for sometime now, closing.

Comment 7

[Martijn Wargers (dead)]

Attachment: testcase

The bug is still there, see testcase.
Google apparently added an alt attribute to the img, that's why you don't see it anymore with the previous testcase.

Comment 9

[Arjan]

Attachment 192017 [details] [diff] seems to fix the double separator issue with a frame 4 without regressing the issues mentioned in bug 280284

Could this get a request review?

Comment 10

[Arjan]

Attachment: Extended Patch

This patch is an extended version of the patch by Martijn Wargers.

What it does is the following:
- Shows only 1 seperator when right clicking with a selection in a frameset 
- Shows a separator below "This frame" when text/objects is selected

I made sure the following stayed the same:
- Shows only 1 separator when right clicking in a frameset 
- Shows a separator below "This frame" when no text/objects is selected

Comment 11

[Mike Schroepfer]

Comment on attachment 292037 [details] [diff] [review]
Extended Patch

Please request approval only after reviews complete

Comment 12

[Martijn Wargers (dead)]

Gavin, can you review the patch?

Comment 13

[Michael Schonfeld]

This bug is actually very similar to bug 417483. Maybe even a dupe.  Only in this solution, the upper separator is hidden, while is bug 417483, the solution hides the lower separator.

Comment 14

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

Comment on attachment 292037 [details] [diff] [review]
Extended Patch

Unfortunately this has bitrotted - I think it might be taken care of by the patch in bug 417483, though?

Comment 15

[Johnathan Nightingale [:johnath]]

Fixed by bug 417483 in changeset:

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

Comment 16

[Johnathan Nightingale [:johnath]]

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

Comment 17

[Johnathan Nightingale [:johnath]]

Relanded. 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

Is this regression covered by the test on bug 417483?

Comment 19

[Martijn Wargers (dead)]

Yes, I think so, since this bug was fixed by that bug. But a separate test for this bug might not hurt.