487692 - Fix context menu IDs and accesskeys and add a test for them

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

The discussion surrounding the context menu tests and fixes going along with it is becoming too long in bug 484187 to hold off landing of the infrastructure and the well-working tests, so I'm splitting this off into its own bug report here.

Comment 1

[Robert Kaiser]

Attachment: context menu fixes, v0

This is basically just the context menu part of bug 484187, for reference to build further work upon.

Comment 2

[Robert Kaiser]

Attachment: base on current Firefox test, fix all other known problems

INFO Passed: 646
INFO Failed: 0
INFO Todo:   0

This patch takes into account all comments from bug 484187 and updates this to the state the Firefox test is in right now, where it also checks activated/deactivated menuitems, as well as working and not working videos.

I'd be very much for going and getting this in with what it has, and then adding other possible states of the context menu if needed to the test.

Comment 3

[Robert Kaiser]

Oh, and sorry for that patch being that large in bytes, most of it is adding the video.ogg needed for testing that a context menu with a correct video has the right states. If that patch needs further iterations, I guess I'll split off the video into its own patch.

Comment 4

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/028eebe1606b

Comment 5

[Robert Kaiser]

Serge, didn't see you on IRC so CCing and commenting here instead.

I'll watch test results as much as I'm around today, but I have to leave for a bit, and it could be that Windows fails some part of this test (set image as wallpaper might be around there) and if so, I'll try to come up with a patch-blind fix for it, but I don't have Windows around to run the test myself.

Linux should work fine, I tested that locally, Mac just passed successfully (I hope, as I couldn't detect what chunk it runs in).

Comment 6

[Robert Kaiser]

(In reply to comment #5)
> Mac just passed successfully (I
> hope, as I couldn't detect what chunk it runs in).

Ah, found it at last: mochitest-plain-5 as I expected, and yes, passed fine on Mac.

Comment 7

[Robert Kaiser]

1095 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_contextmenu.html | menuitem context-savepage has same accesskey as context-setWallpaper
1119 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_contextmenu.html | checking item #9 (---) name - got "context-setWallpaper", expected "---"

All others are just confusion by the latter one.

For the accesskey, the good thing is that "Select All" is always hidden on images, so we can re-use "A" there (label is "Set As Wallpaper") from what I see.

For the item itself, we need to detect either if the shell service is present or if we're on Windows.

Comment 8

[Robert Kaiser]

Attachment: fix up Windows

It's not particularly pretty, but this patch does the job and somewhat leaves the flow and logic of those arrays intact for a reader.

Comment 9

[[e-mail]]

Comment on attachment 440887 [details] [diff] [review]
fix up Windows

Ideally you would use the same hasShell logic as nsContextMenu.js so that you don't have to fix the test should someone implement the shell service on Mac or Linux.

"A" is sadly used by "Save Page"...

Comment 10

[[e-mail]]

Sorry, I was looking at an unpatched build. It actually conflicts with Copy Email Address.

Comment 11

[[e-mail]]

Sigh, View Image and Open Link in New Window both use W...

Comment 12

[Robert Kaiser]

Attachment: test images with links, fix Windows

OK, I decided to just add tests for images with links as well, so that we know we don't conflict there. I found a solution that should pass all those tests, and I found a better solution for the shellservice check, I hope.

Comment 13

[Robert Kaiser]

Oh, and reopening this as long as we have something to do in here.

Comment 14

[[e-mail]]

Attachment: More juggling

I didn't like your use of y as the access key to Copy Image, so I moved Copy Image to C, which meant I couldn't move Block Image to c, so I moved that to o, but that meant moving Reject popup windows to u, which meant moving View Source to o. But I then had to tweak nsContextMenu.js so that View Selection Source didn't appear on an image, Reject popup windows didn't appear on a textbox and Copy didn't appear on an image, and also I made it so Search web for... didn't appear on a link (they both use S). So the test might need changing ;-)

Comment 15

[Robert Kaiser]

Sorry, now we are at the point where I start to cry and almost want to throw away everything.

Deciding what to show for what items just because of access keys is not only wrong but plain wrong and - ok, let's leave too strong words out of this.

I now am at the point where I refuise to look at any patch unless it contains a decsription of what is _useful_ to show on what context menu, and I'm about to either back out the patch I already checked in or mark this an Alpha blocker and nag you daily until we have a decent plan of what are useful items to show in which case - and ONLY THEN assign accesskeys, perhaps from scratch.

I can't believe we're even thinking about changing what's available in an accesskey based on the mere needs of a single of the languages we're supporting!

Comment 16

[[e-mail]]

Comment on attachment 441117 [details] [diff] [review]
More juggling

Please read comment 14 while I repeatedly thwap Bugzilla for its stupidity...

Comment 17

[Robert Kaiser]

OK, as I said, I'm completely disagreeing with "I didn't like your use of <x> as the access key to <foo>, so [...] I then had to tweak nsContextMenu.js so that <bar> didn't appear". That's coming from the wrong way, it's better to first decide which context menu entries we should show where (I agree it's longer than needed in a number of cases) and only then attack the accesskey problem again.

Because of that, I'm now going through all cases the patched test after my attachment 440918 [details] [diff] [review] would cover, and try to reason about all entries we're showing there at the moment, listing all IMHO questionable ones here.

So, here's my result and questions:

// Context menu for plain text

Looks reasonable.

// Context menu for text link

"context-bookmarkpage", "context-savepage", "context-sendpage"

Are those reasonable here? Shouldn't people always easily be able to click outside of a link to do those instead? Can't they even be confusing due to their similarity to the same actions on the link target?

// Context menu for text mailto-link

Same as for text links, though less confusion.

// Context menu for text input field

Looks reasonable.

// Context menu for an image

"context-bookmarkpage", "context-savepage", "context-sendpage" are again somewhat questionable, but thinking about popups having an image (even a linked one?) over their whole size and no top-level menu, I can see some reasoning for wanting this.
I also disagree with removing "View Selection Source" from images, as I tend to use that point to see the markup behind certain elements on screen, and that includes images.

Note: No selections in the test so far, so "View Selection Source" is UNTESTED right now.

// Context menu for an image with a link

Do we really need the whole list of image items here? I wonder if it would be OK to just leave "View Image" here and remove all other image-related items (context-*image, contest-setWallpaper). People wanting those actions can always "view image" and do those things from there.

// Context menu for an image with a mailto: link

Same as above, can we remove all image items except "View Image"?

// Context menu for a canvas

Looks reasonable (mostly, after comparison with FF).

// Context menu for a video

Looks reasonable.

// Context menu for an iframe

Looks reasonable.

Comment 18

[Robert Kaiser]

Comment on attachment 441117 [details] [diff] [review]
More juggling

As said in comment #17, I don't agree with the change to "context-viewpartialsource-selection", I'm also not convinced by the "context-searchselect" change as I can easily come up with use cases where I'd want that.
The "popupwindow-reject/allow" change seems reasonable, though, but "context-sep-popup" needs to be hidden on text inputs as well, then.
I'm also not sure about removing "context-copy" from images, I don't see why it would be less useful there than elsewhere.

We definitely need to add tests for selections, I see that now.

And as I said above, if we go for thinking about removing some items, let's think that through and decide it before fixing any other accesskeys. Not liking an accesskey can't be the reason for removing items, we need to think about use-cases instead.

Comment 19

[[e-mail]]

(In reply to comment #18)
> The "popupwindow-reject/allow" change seems reasonable, though, but
> "context-sep-popup" needs to be hidden on text inputs as well, then.
Good point.

> I'm also not sure about removing "context-copy" from images, I don't see why it
> would be less useful there than elsewhere.
I thought it was justified on the basis that "Copy Image" already copies it in text and HTML format. (I can't justify the other changes.)

> We definitely need to add tests for selections, I see that now.
In that case you'll want to figure out what to do for a context menu on a selected image. (Block vs. Copy)

Comment 20

[[e-mail]]

Comment on attachment 440918 [details] [diff] [review]
test images with links, fix Windows

We got away with using y for Copy Image Location on Linux for 2.0.x so I guess it's better than nothing, and we can at least move forward...

Comment 21

[[e-mail]]

Also, sorry for not noticing comment #15 - when I hit submit I didn't hang around and as a result I didn't see Bugzilla's stupid multiple requestee message until later and I didn't know you had already commented when I then went and completed the action I had originally intended to make.

(In reply to comment #17)
> Shouldn't people always easily be able to click
> outside of a link to do those instead?
But then again this is almost justification for not allowing Search Web on a link or View Partial Source on an image ;-)

Comment 22

[Robert Kaiser]

Pushed Windows fixup and linked image test patch as http://hg.mozilla.org/comm-central/rev/87899a3eea57

Will file followups in a moment.