484187 - Port plain mochitests from browser/base/content to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Robert Kaiser]

I did some work to port tests from browser/base/content to SeaMonkey, the plain mochitests will be done in this bug, browser tests get split to a different bug as we need some actual XUL fixes to get the context menu test working correctly.

Comment 1

[Robert Kaiser]

Attachment: add plain mochitests for browser

This patch ports test_feed_discovery.html, test_bug395533.html and test_contextmenu.html from Firefox to SeaMonkey, the only plain mochitest I left out is test_offlineNotification.html as we don't have notifications for offline apps implemented in SeaMonkey (yet).

The contextmenu test did uncover some bugs with missing IDs and duplicate accesskeys, the fixes for those are included in this patch as well.

The popupwindow-reject item is shown in all context menus with this patch and also has no accesskey, but it's hard to find one that always fits, so I special-cased this to a todo(), we can file a followup on this is wanted.

Entry #11 in the feed discovery test (the test is using XULBrowserWindow.feeds as we don't have selectedBrowser.feeds) fails even though it should detect the feed, I made it TODO as well, we can handle this edge-case in a followup.

With that, this is the result of running the tests here:

*** 392 INFO Passed: 376
*** 393 INFO Failed: 0
*** 394 INFO Todo:   9

8 of the TODOs are popupwindow-reject missing an accesskey, 1 is the feed detection case.

Comment 2

[Justin Wood (:Callek)]

Comment on attachment 368255 [details] [diff] [review]
add plain mochitests for browser

<!-- type can have leading and trailing whitespace -->

:/ Uh-Oh.... that needs a followup!

I'm also surprised we pass test_bug395533.html. From the read-through of that test it requires about:feeds to be present along with feed sniffing. KaiRo do you have that patch locally applied? and/or are you sure this passes?

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 368255 [details] [diff] [review]
add plain mochitests for browser

I assume these tests have to be ported and could not be moved from Firefox to a shared place, could they ?


>diff --git a/suite/browser/test/Makefile.in b/suite/browser/test/Makefile.in
>+_TEST_FILES =	test_feed_discovery.html \

Nits:
Could you use
>+_TEST_FILES =	\
>+   test_feed_discovery.html \
style ?
And +/- sort the tests alphabetically ?

Comment 4

[Justin Wood (:Callek)]

(In reply to comment #2)
> (From update of attachment 368255 [details] [diff] [review])
> I'm also surprised we pass test_bug395533.html. ...

Err looks like this was testing that we do _not_ sniff that text file as a feed. Which surely is the case pre-asrail's patch. And adding it now will catch a regression early, so good!

(In reply to comment #3)
> (From update of attachment 368255 [details] [diff] [review])
> I assume these tests have to be ported and could not be moved from Firefox to a
> shared place, could they ?

Sadly you're correct, cannot be shared.

Comment 5

[Robert Kaiser]

(In reply to comment #2)
> (From update of attachment 368255 [details] [diff] [review])
> <!-- type can have leading and trailing whitespace -->
> 
> :/ Uh-Oh.... that needs a followup!

Yes, as I said on IRC, I'll file the todo()s I needed to mark as followups, this is one of them ;-)

> I'm also surprised we pass test_bug395533.html. From the read-through of that
> test it requires about:feeds to be present along with feed sniffing. KaiRo do
> you have that patch locally applied? and/or are you sure this passes?

I just was about to reply that I have that patch applied when I realized that's on a different computer, I don't have it applied here.

(In reply to comment #3)
> (From update of attachment 368255 [details] [diff] [review])
> I assume these tests have to be ported and could not be moved from Firefox to a
> shared place, could they ?

They test things that are specific to Firefox or SeaMonkey UI, the only reason I could port them over is that we have ported a number of features from there or implemented similar ones. I also needed to heavily modify some of the tests to fit SeaMonkey specifically. So, as they test app-specific code/UI, it's only right that they are app-specific tests.

> >diff --git a/suite/browser/test/Makefile.in b/suite/browser/test/Makefile.in
> >+_TEST_FILES =	test_feed_discovery.html \
> 
> Nits:
> Could you use
> >+_TEST_FILES =	\
> >+   test_feed_discovery.html \
> style ?
> And +/- sort the tests alphabetically ?

Might be a good idea in the end, I for now left the style very much the way Firefox has it, I'll wait for what else comes up in review and I'll like reviewers to comment on what style they prefer - more similar to what FF has or more consistent and logical internally.

Comment 6

[Robert Kaiser]

(In reply to comment #1)
> *** 394 INFO Todo:   9
> 
> 8 of the TODOs are popupwindow-reject missing an accesskey,

Filed as bug 484381.

> 1 is the feed detection case.

Filed as bug 484379.

Comment 7

[[e-mail]]

(In reply to comment #1)
> The popupwindow-reject item is shown in all context menus with this patch and
> also has no accesskey, but it's hard to find one that always fits, so I
> special-cased this to a todo(), we can file a followup on this is wanted.
In fact the only "free" letter is j (things are so bad we already use g...)

Comment 8

[Robert Kaiser]

(In reply to comment #7)
> (In reply to comment #1)
> > The popupwindow-reject item is shown in all context menus with this patch and
> > also has no accesskey, but it's hard to find one that always fits, so I
> > special-cased this to a todo(), we can file a followup on this is wanted.
> In fact the only "free" letter is j (things are so bad we already use g...)

j is _really_ bad, but I feared that, and all that trouble is why I ended up marking it todo() and filing bug 484381, it should not distract us from getting the tests in (and the other accesskeys fixed where I found clashes with the test). :)

Comment 9

[[e-mail]]

OK, so this was the set of access key changes I came up with. Note that this does not include Bookmark This Frame... was there a reason to change that?

Select All uses A so change Save Page (As) to e
... and change View Selection/MathML Source to u
... and change Copy Email Address to A
... and change Reload Image to R
Switch Text Direction (needs bidi pref) uses w so change Ignore Word to I
Copy use C so change Copy Link Location to L
... and change Bookmark Link to B ... and change (Un)Block Image to k
Open Link in New Window uses W so change Search Web for to f
... and change Fit Image to Window to W

As for the todo, we can set Allow/Reject popup windows to o
... and change Copy Media Location to L
... and change Copy Image to I ... and change View Image to w

Comment 10

[Robert Kaiser]

(In reply to comment #9)
> OK, so this was the set of access key changes I came up with. Note that this
> does not include Bookmark This Frame... was there a reason to change that?

That was mainly for consistency, IIRC.

> Switch Text Direction (needs bidi pref) uses w so change Ignore Word to I

I is a bad accesskey, as it's very short and almost invisible with many fonts.

> Open Link in New Window uses W so change Search Web for to f

f is a bad accesskey, we should avoid it if possible.

> ... and change Copy Image to I ... and change View Image to w

I again.

We should at least try to avoid bad accesskeys for items that are probably used more often. Do we show all those items where you proposed changes in the same context?

Comment 11

[[e-mail]]

(In reply to comment #10)
> (In reply to comment #9)
> > OK, so this was the set of access key changes I came up with. Note that this
> > does not include Bookmark This Frame... was there a reason to change that?
> That was mainly for consistency, IIRC.
Actually I now see that it clashes with Save Frame (As) in the submenu, so we need to change it anyway.

> > Switch Text Direction (needs bidi pref) uses w so change Ignore Word to I
> I is a bad accesskey, as it's very short and almost invisible with many fonts.
OK, so maybe switch (pun not intended) Switch Text Direction to x?

> > Open Link in New Window uses W so change Search Web for to f
> f is a bad accesskey, we should avoid it if possible.
Search Web for ... appears whenever text is selected, but maybe we could change it to "S" and change "Set As Wallpaper" to e.

> > ... and change Copy Image to I ... and change View Image to w
> I again.
How about changing Copy Image to m but changing View Image to w anyway?

> We should at least try to avoid bad accesskeys for items that are probably used
> more often. Do we show all those items where you proposed changes in the same
> context?
Yes, the checks have been the cause of the delay!

Note: Unlike the previous set, I haven't run these proposals past the tests.

Comment 12

[Robert Kaiser]

(In reply to comment #11)
> How about changing Copy Image to m but changing View Image to w anyway?

"Bookmark This Page" has "m" already.

> Note: Unlike the previous set, I haven't run these proposals past the tests.

Would have caught this one ;-)

Comment 13

[Robert Kaiser]

Attachment: plain mochitests for browser, v1.1

OK, I used "g" for "Bookmark This Page", even though it's somewhat lame, but it works.

Comment 14

[[e-mail]]

(In reply to comment #13)
> I used "g" for "Bookmark This Page", even though it's somewhat lame, but it
works.

Sadly not; for bidi users, "g" is Switch Page Direction, which exists whenever there isn't a text field (when you get Switch Text Direction instead).

I've just noticed that we can't change Reload Image to R either...

Comment 15

[[e-mail]]

OK, so I think we can move Save Link Target to S and also we should perhaps move Add To Dictionary to n otherwise it conflicts with Reject Popup Windows, but I'm afraid I'm going to have to ask you to move Bookmark This Page back to m and Copy Image back to I (which we already use for View Page Info, so there).

Comment 16

[[e-mail]]

Comment on attachment 370658 [details] [diff] [review]
plain mochitests for browser, v1.1

> <!ENTITY bookmarkPageCmd.label        "Bookmark This Page">
>-<!ENTITY bookmarkPageCmd.accesskey    "m">
>+<!ENTITY bookmarkPageCmd.accesskey    "g">
We can't make this change because "Switch Page Direction" already uses "g".

> <!ENTITY bookmarkFrameCmd.label       "Bookmark This Frame">
>-<!ENTITY bookmarkFrameCmd.accesskey   "m">
>+<!ENTITY bookmarkFrameCmd.accesskey   "B">
[You might or might not want to revert this change too.]

> <!ENTITY saveLinkAsCmd.label          "Save Link Target As…">
> <!ENTITY saveLinkCmd.label            "Save Link Target">
> <!ENTITY saveLinkCmd.accesskey        "r">
We need to move this to "S" as it can appear with "Reload Image".

> <!ENTITY copyImageCmd.label           "Copy Image">
>-<!ENTITY copyImageCmd.accesskey       "o">
>+<!ENTITY copyImageCmd.accesskey       "m">
We need to move this to "I" so it can appear with "Bookmark This Page".

> <!ENTITY spellAddToDictionary.label     "Add to Dictionary">
> <!ENTITY spellAddToDictionary.accesskey "o">
We need to move this to "n" as it can appear with "Reject popup windows".

Comment 17

[Robert Kaiser]

Attachment: plain mochitests for browser, v1.2

This patch addresses the further accesskey conflicts mentioned in the last review/comment.

Comment 18

[[e-mail]]

(In reply to comment #16)
> (From update of attachment 370658 [details] [diff] [review])
> > <!ENTITY bookmarkFrameCmd.label       "Bookmark This Frame">
> >-<!ENTITY bookmarkFrameCmd.accesskey   "m">
> >+<!ENTITY bookmarkFrameCmd.accesskey   "B">
Actually this change is right, since it conflicts with Save Frame...
(even Netscape 7.2 has the bug!)

Comment 19

[Robert Kaiser]

Attachment: plain mochitests for browser, v2

I have split off the contextmenu stuff into bug 487692, this is getting too long and complex to hold off adding the infrastructure and the passing tests.
The patch here does just that, I'll attach the current state of contextmenu unchanged to the other bug as a base for further work.

Comment 20

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/6310cc51872b