514520 - Double separator in context menu

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Dão Gottwald [:dao]]

STR:

1. open data:text/html,<img src="about:logo">
2. select all
3. open the context menu by clicking next to the image

expected result:

Copy
Select All
---------------------
View Selection Source


actual result:

Copy
Select All
---------------------
---------------------
View Selection Source

Comment 1

[Johnathan Nightingale [:johnath]]

Attachment: Don't show properites separator on isContentSelected

Since context-sep-selectall is shown on this.isContentSelected, it doesn't make sense to show context-sep-properties as well. It *should* still be shown on shouldShow, since it will then act as a separator for View Source/Page Info.

When I land this, I'll also land the fix for bug 417483 which has been waiting on checkin for some time, and fixes a related problem in frames code which I thought I had caused until gavin pointed out that was known (and fixed, just not landed!)

Comment 2

[Johnathan Nightingale [:johnath]]

That, in turn, should fix the similar seeming, though not strictly identical, bug 303898.

Comment 3

[Dão Gottwald [:dao]]

Does "context-sep-properties" refer to the removed item? If so, can we find a better name?

Comment 4

[Johnathan Nightingale [:johnath]]

Attachment: Better name for the separator

Good catch - should have done that in the original removal.

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 398729 [details] [diff] [review]
Better name for the separator

It would be clearer if the context-sep-viewsource element was moved between context-viewpartialsource-mathml and context-viewsource, as it will never be shown together with context-viewpartialsource-selection and context-viewpartialsource-mathml, as far as I can tell.

Comment 6

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/mozilla-central/rev/3655f5de6392

Comment 7

[Johnathan Nightingale [:johnath]]

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

Comment 8

[Johnathan Nightingale [:johnath]]

Attachment: Update test_contextmenu.html as well

I really don't know why my pre-checkin testing didn't catch this - all I can figure is think-o. In any event, this one fixes the problem locally, but I'll be holding off until the tryserver finishes with it, for what I hope are obvious reasons.

Comment 9

[Johnathan Nightingale [:johnath]]

Relanded with test fix - http://hg.mozilla.org/mozilla-central/rev/e35bb64a2af3

Comment 10

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

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

Comment 11

[Johnathan Nightingale [:johnath]]

Attachment: Combined "patch" for 192

Here's a concatenated export of the fix for this bug and bug 417483. It won't cleanly apply as a patch, since it's two patches glued together, but is otherwise exactly what I'd push. This is not a blocker, but a near-trivial fix for a couple duplicated separators in our context menus, one of which is a direct result of the properties dialog deletion in bug 513147.

Comment 12

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

Comment on attachment 401042 [details] [diff] [review]
Combined "patch" for 192

a192=beltzner

Comment 13

[Johnathan Nightingale [:johnath]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/93d992dbf533

Comment 14

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

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