Closed Bug 712871 Opened 12 years ago Closed 12 years ago

[SeaMonkey, Windows] mochitest-5: "test_contextmenu.html | checking expected number of menu entries - got 26, expected 24" and 30+ other errors

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
major

Tracking

(seamonkey2.6 wontfix, seamonkey2.7 wontfix, seamonkey2.8 wontfix)

RESOLVED FIXED
seamonkey2.9
Tracking Status
seamonkey2.6 --- wontfix
seamonkey2.7 --- wontfix
seamonkey2.8 --- wontfix

People

(Reporter: sgautherie, Assigned: sgautherie)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [perma-orange])

Attachments

(9 files, 3 obsolete files)

3.48 KB, patch
neil
: review+
Details | Diff | Splinter Review
5.64 KB, patch
neil
: review+
Details | Diff | Splinter Review
17.75 KB, patch
neil
: review+
Details | Diff | Splinter Review
2.93 KB, patch
neil
: review+
Details | Diff | Splinter Review
1.36 KB, patch
neil
: review+
Details | Diff | Splinter Review
3.33 KB, patch
neil
: review+
Details | Diff | Splinter Review
19.72 KB, patch
Details | Diff | Splinter Review
2.74 KB, patch
neil
: review+
Details | Diff | Splinter Review
24.05 KB, patch
neil
: review+
Details | Diff | Splinter Review
http://brasstacks.mozilla.com/topfails/test/SeaMonkey?name=/tests/suite/browser/test/test_contextmenu.html
Only one failure :-< ("2011-05-24 01:05 SeaMonkey WINNT 5.2   [f5cf5a444540]")
(Regression have happened after 2011-09-20+, but that can't be trusted.)


http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1321914049.1321914576.732.gz
OS X 10.5 comm-central-trunk debug test mochitests-5/5 on 2011/11/21 14:20:49
Already failing.


http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1324508264.1324509190.19095.gz&fulltext=1
WINNT 5.2 comm-central-trunk debug test mochitests-5/5 on 2011/12/21 14:57:44
{
2297 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_contextmenu.html | checking expected number of menu entries - got 26, expected 24
...
2621 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_contextmenu.html | checking expected number of menu entries - got 4, expected 6
2622 ERROR TEST-UNEXPECTED-FAIL | /tests/suite/browser/test/test_contextmenu.html | checking expected number of menu entries - got 26, expected 40
}


I can reproduce on my local Windows 2000 with an optimized build.

(I assume some bugs were not ported, at least their test part.)
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1321905260.1321905851.10136.gz
OS X 10.6 comm-beta debug test mochitests-5/5 on 2011/11/21 11:54:20
Already there on (SM 2.6) comm-beta too.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
Component: General → UI Design
Depends on: 697124
QA Contact: general → ui-design
Bug 697124 forgot to update test, though Firefox had.
Attachment #583731 - Flags: review?(neil)
Just an idle question. Why two patches instead of one?
(In reply to Serge Gautherie (:sgautherie) from comment #0)
> (I assume some bugs were not ported, at least their test part.)

Indeed: this test obviously misses many "recent" ports from FF :-<
(I wonder why people exclude/ignore that part when porting FF bugs :-/)


(In reply to Philip Chee from comment #4)
> Just an idle question. Why two patches instead of one?

Patch Av1 tracks port of FF bug 681550. Patch Bv1 tracks port of FF bug 669260. Though both were ported in SM bug 697124.
(This is easier, at least for me, to track resynchronization work.)
Depends on: 702019
Bug 702019 forgot to update test, though Firefox had.

(Though this patch, as is, doesn't help with remaining failures.)
Attachment #583752 - Flags: review?(neil)
(In reply to Serge Gautherie (:sgautherie) from comment #5)
> (I wonder why people exclude/ignore that part when porting FF bugs :-/)

Only speaking for myself here: I pretty much gave up and forgot about tests when perma-orange became the default state. I know this is not constructive so I'll try to help you sort out the existing issues and better keep tests in mind in the future. In case of the HTML5 context menu vs. this bug I have to admit that I didn't even know about the existence of this test on our side. My bad.

Feel free to request feedback and/or review on any test fixes you feel comfortable handing over to me, and CC me as you see fit. I can test both Win7 and Linux. No promises for the next two weeks, though. ;-)
Attachment #583729 - Flags: review?(neil) → review+
Attachment #583731 - Flags: review?(neil) → review+
Comment on attachment 583729 [details] [diff] [review]
(Av1) Update test after bug 697124 ('context-video-saveimage' part)
[Checked in: Comment 8]

http://hg.mozilla.org/comm-central/rev/84ba7b2c99f6
Attachment #583729 - Attachment description: (Av1) Update test after bug 697124 ('context-video-saveimage') → (Av1) Update test after bug 697124 ('context-video-saveimage') [Checked in: Comment 8]
Comment on attachment 583731 [details] [diff] [review]
(Bv1) Update test after bug 697124 ('controls' part)
[Checked in: Comment 9]

http://hg.mozilla.org/comm-central/rev/aeefba3ebe64
Attachment #583731 - Attachment description: (Bv1) Update test after bug 697124 ('controls' part) → (Bv1) Update test after bug 697124 ('controls' part) [Checked in: Comment 9]
Attachment #583729 - Attachment description: (Av1) Update test after bug 697124 ('context-video-saveimage') [Checked in: Comment 8] → (Av1) Update test after bug 697124 ('context-video-saveimage' part) [Checked in: Comment 8]
Attachment #583752 - Attachment filename: 712871-Cv1_bug697124.diff → 712871-Cv1_bug702019.diff
Depends on: 677463
Sync' test with Core/Firefox.
(Unrelated to remaining failures.)
Attachment #583768 - Flags: review?(neil)
Depends on: 338427
Sync' test with Core/Firefox.
(Unrelated to remaining failures.)
Attachment #583779 - Flags: review?(neil)
Depends on: 682618
Depends on: 46555
Sync' test with Core/Firefox.
Bug 682618 fixes some more failures,
but "reveals" bug 46555, so fix it at once.
Attachment #583806 - Flags: review?(neil)
Comment on attachment 583779 [details] [diff] [review]
(Ev1) Port bug 338427 ('contenteditable.focus()')

I found two core tests today that don't focus their contenteditable element :-(
Attachment #583779 - Flags: review?(neil) → review+
Comment on attachment 583806 [details] [diff] [review]
(Fv1) Port bug 682618 ('lastElement'), Port bug 46555 ('context-selectall' fix)
[Checked in: Comment 18]

Actually, doesn't this make the previous patch unnecessary?
Attachment #583752 - Flags: review?(neil) → review+
Attachment #583768 - Flags: review?(neil) → review+
(In reply to neil@parkwaycc.co.uk from comment #14)
> Comment on attachment 583806 [details] [diff] [review]
> (Fv1) Port bug 682618 ('lastElement'), Port bug 46555 ('context-selectall'
> fix)
> 
> Actually, doesn't this make the previous patch unnecessary?

Right: same question occurred to me at about the same time ;->
And answer should be "obviously yes".
I'll delay checking patch Ev1 in and (later) file a FF bug about that and some other nits.
Comment on attachment 583752 [details] [diff] [review]
(Cv1) Update test after bug 702019 (generated submenu)
[Checked in: Comment 16+22]

http://hg.mozilla.org/comm-central/rev/abb5e5bbbcd3
Attachment #583752 - Attachment description: (Cv1) Update test after bug 702019 (generated submenu) → (Cv1) Update test after bug 702019 (generated submenu) [Checked in: Comment 16]
Comment on attachment 583768 [details] [diff] [review]
(Dv1) Port bug 677463 ('Item w/ textContent')
[Checked in: Comment 17]

http://hg.mozilla.org/comm-central/rev/24729d88b962
Attachment #583768 - Attachment description: (Dv1) Port bug 677463 ('Item w/ textContent') → (Dv1) Port bug 677463 ('Item w/ textContent') [Checked in: Comment 17]
Attachment #583806 - Flags: review?(neil) → review+
Comment on attachment 583806 [details] [diff] [review]
(Fv1) Port bug 682618 ('lastElement'), Port bug 46555 ('context-selectall' fix)
[Checked in: Comment 18]

Actually, Debug OSX(_64) builds are already reporting bug 46555 (contrary to what an older Opt Win build did on my local Win2k):
patch Fv1 landing should confirm that patch Ev1 is not needed anymore ;-)


http://hg.mozilla.org/comm-central/rev/d4a15b0bd70e
Attachment #583806 - Attachment description: (Fv1) Port bug 682618 ('lastElement'), Port bug 46555 ('context-selectall' fix) → (Fv1) Port bug 682618 ('lastElement'), Port bug 46555 ('context-selectall' fix) [Checked in: Comment 18]
Blocks: 713173
Depends on: 364914
Sync' test with Core/Firefox.
Bug 364914 fixes remaining failures :-)
Attachment #584003 - Flags: review?(neil)
Sync' test with Core/Firefox.
Move SeaMonkey specific cases to the end.
Attachment #584009 - Flags: review?(neil)
Depends on: 696167
Depends on: 587134
Sync' test with Firefox,
even though this test code, as is, does nothing useful for SeaMonkey.
Attachment #584015 - Flags: review?(neil)
Comment on attachment 583752 [details] [diff] [review]
(Cv1) Update test after bug 702019 (generated submenu)
[Checked in: Comment 16+22]

(Jv1) Remove extra '+' from patch Cv1. (no review)
Attachment #583752 - Attachment description: (Cv1) Update test after bug 702019 (generated submenu) [Checked in: Comment 16] → (Cv1) Update test after bug 702019 (generated submenu) [Checked in: Comment 16+22]
Hv1, with subtst_contextmenu.html part.
Attachment #584009 - Attachment is obsolete: true
Attachment #584009 - Flags: review?(neil)
Attachment #584017 - Flags: review?(neil)
Blocks: 713192
Comment on attachment 584003 [details] [diff] [review]
(Gv1) Port bug 364914 ('shouldWaitForFocus')

>-    if (lastElement)
>-      lastElement.blur();
>-    element.focus();
There's no point duplicating this; just leave it here. r=me with that fixed.

>-    lastElement = element;
I'd like to believe that this can be left here too.

>+    //Some elements need time to focus and spellcheck before any tests are
>+    //run on them.
>+    if(shouldWaitForFocus)
Nit: space after // and before (

>+        var eventDetails = { type : "contextmenu", button : 2, shiftKey : shiftkey };
>+        synthesizeMouse(element, 2, 2, eventDetails, element.ownerDocument.defaultView);
[Theoretically this duplication can be avoided too but it's probably not worth it for only two lines of code.]
Attachment #584003 - Flags: review?(neil) → review+
Comment on attachment 584015 [details] [diff] [review]
(Iv1) Port (test part of) bug 587134 and bug 696167 (Firefox only, 'inspectItems' + pref)

I think this is pointless since our list of context menu items is already sufficiently different to that of Firefox.
Attachment #584015 - Flags: review?(neil)
Comment on attachment 584017 [details] [diff] [review]
(Hv1a) Port m-c changeset d957a1dd0894 (semicolon), Reoder cases to make things clearer

>+        // break;
>+
>+    case 12:
>+        todo(false, "[SeaMonkey] Case 12 will be ported in bug 713173.");
>+        // break;
>+
>+    case 13:
>+        todo(false, "[SeaMonkey] Case 13 will be ported in bug 713173.");
>+        overrideTestNum(13);
Don't have both the empty cases and the override. In fact, don't bother with either, just renumber the tests when you add the two missing tests. As there are only two SeaMonkey-specific tests, renumbering them won't be a big deal.
Attachment #584017 - Flags: review?(neil) → review-
Comment on attachment 584003 [details] [diff] [review]
(Gv1) Port bug 364914 ('shouldWaitForFocus')

(In reply to neil@parkwaycc.co.uk from comment #24)
> There's no point duplicating this; just leave it here. r=me with that fixed.
> 
> I'd like to believe that this can be left here too.
> 
> Nit: space after // and before (
> 
> [Theoretically this duplication can be avoided too but it's probably not
> worth it for only two lines of code.]

I fully agree, but would prefer to be as close to Firefox as possible.
My patch Av1-FF in bug 713192 should fix this (for FF first)...
Can I land my current patch here as is?
Attachment #584003 - Flags: feedback?(neil)
Comment on attachment 584003 [details] [diff] [review]
(Gv1) Port bug 364914 ('shouldWaitForFocus')

No, though feel tree to wait until you get Firefox patched first.
Attachment #584003 - Flags: feedback?(neil) → feedback-
Gv1, with comment 24 suggestion(s).
Attachment #584003 - Attachment is obsolete: true
Attachment #584393 - Flags: review?(neil)
Attachment #584003 - Flags: feedback-
Attachment #584393 - Flags: review?(neil) → review+
Comment on attachment 584393 [details] [diff] [review]
(Gv1a) Port bug 364914 ('shouldWaitForFocus')
[Checked in: Comment 30]

http://hg.mozilla.org/comm-central/rev/332abbd051eb
Attachment #584393 - Attachment description: (Gv1a) Port bug 364914 ('shouldWaitForFocus') → (Gv1a) Port bug 364914 ('shouldWaitForFocus') [Checked in: Comment 30]
(In reply to comment #26)
> Don't have both the empty cases and the override. In fact, don't bother with
> either, just renumber the tests when you add the two missing tests.
Sorry, I was unclear here. I think I meant that it would be better if bug 713173 moved those two tests to make room for the new tests, but I'll r+ the latest patch in case you prefer to do it the other way around.
Attachment #584422 - Flags: review?(neil) → review+
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1325007902.1325008824.4674.gz
WINNT 5.2 comm-central-trunk debug test mochitests-5/5 on 2011/12/27 09:45:02

"mochitest-plain-5: 9690/0/34": this test suite is now green :-)
Comment on attachment 584422 [details] [diff] [review]
(Hv1b) Port m-c changeset d957a1dd0894 (semicolon), Reoder cases and elements to make things clearer
[Checked in: Comment 34]

http://hg.mozilla.org/comm-central/rev/378ab7f9d3bb
Attachment #584422 - Attachment description: (Hv1b) Port m-c changeset d957a1dd0894 (semicolon), Reoder cases and elements to make things clearer → (Hv1b) Port m-c changeset d957a1dd0894 (semicolon), Reoder cases and elements to make things clearer [Checked in: Comment 34]
No longer blocks: 713173
Target Milestone: --- → seamonkey2.9
Serge, anything else to do here?
No reply in five months. Closing. Please file new bugs for new issues.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.