Closed Bug 1042092 Opened 5 years ago Closed 5 years ago

Write test for consumeanchor

Categories

(Core :: XUL, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
mozilla34
Iteration:
34.1

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file, 2 obsolete files)

Splitting this off from bug 987230 per feedback in the iteration meeting, last patch being attachment 8453368 [details] [diff] [review] .

(In reply to Neil Deakin from bug 987230 comment #27)
> Comment on attachment 8453368 [details] [diff] [review]
> test consumeanchor behaviour,
> 
> 
> This test passes on my Mac even though I don't have the rollupanchor patch
> applied. It should be written to fail.
> 
> 
> +  <popupset>
> +    <popup id="mypopup"
> +           onpopupshown="onMyPopupShown(event)"
> +           onpopuphidden="onMyPopupHidden(event)">This is a test
> popup</popup>
> +  </popupset>
> 
> 'popup' is deprecated so you should use menupopup instead. And popupset
> doesn't do anything so you should just remove it.
> 
> 
> +  let isMac = ("nsILocalFileMac" in Ci);
> +  let isLinux = ("@mozilla.org/gnome-gconf-service;1" in Cc);
> +  let isWindows = ("@mozilla.org/windows-registry-key;1" in Cc); 
> 
> You only use the isWindows variable, and you should just use
> navigator.platform rather than this weird component-checking thing.
> 
> +
> +  function synthesizeNativeMouseLDown(aElement, aOffsetX, aOffsetY) {
> +    let msg = isWindows ? 2 : 1;
> +    synthesizeNativeMouseEvent(aElement, aOffsetX, aOffsetY, msg);
> +  }
> +
> +  /**
> +  * Fires a synthetic 'mouseup' event on the current about:newtab page.
> +  * @param aElement The element used to determine the cursor position.
> +  */
> 
> Remove this comment which I assume you copied from somewhere else.
Status: NEW → ASSIGNED
QA Whiteboard: [qa?]
Hi Gijs, can you mark this bug as either [qa+] or [qa-] for verification.
Flags: needinfo?(gijskruitbosch+bugs)
QA Whiteboard: [qa?] → [qa-]
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch (Gone July 26 - August 3) from comment #0)
> Splitting this off from bug 987230 per feedback in the iteration meeting,
> last patch being attachment 8453368 [details] [diff] [review] .
> 
> (In reply to Neil Deakin from bug 987230 comment #27)
> > Comment on attachment 8453368 [details] [diff] [review]
> > test consumeanchor behaviour,
> > 
> > 
> > This test passes on my Mac even though I don't have the rollupanchor patch
> > applied. It should be written to fail.

FWIW, I think the behaviour that was flagged up by bug 987230 worked on mac before, too - it was only Linux and/or Windows that were broken. I expect this has to do with some other event mechanism deciding to close the popup. I know for sure that I used Linux to test before/after behaviour with the original patch. I'll doublecheck the test there to be sure.

> > +  <popupset>
> > +    <popup id="mypopup"
> > +           onpopupshown="onMyPopupShown(event)"
> > +           onpopuphidden="onMyPopupHidden(event)">This is a test
> > popup</popup>
> > +  </popupset>
> > 
> > 'popup' is deprecated so you should use menupopup instead. And popupset
> > doesn't do anything so you should just remove it.
> > 
> > 
> > +  let isMac = ("nsILocalFileMac" in Ci);
> > +  let isLinux = ("@mozilla.org/gnome-gconf-service;1" in Cc);
> > +  let isWindows = ("@mozilla.org/windows-registry-key;1" in Cc); 
> > 
> > You only use the isWindows variable, and you should just use
> > navigator.platform rather than this weird component-checking thing.
> > 
> > +
> > +  function synthesizeNativeMouseLDown(aElement, aOffsetX, aOffsetY) {
> > +    let msg = isWindows ? 2 : 1;
> > +    synthesizeNativeMouseEvent(aElement, aOffsetX, aOffsetY, msg);
> > +  }
> > +
> > +  /**
> > +  * Fires a synthetic 'mouseup' event on the current about:newtab page.
> > +  * @param aElement The element used to determine the cursor position.
> > +  */
> > 
> > Remove this comment which I assume you copied from somewhere else.

Thanks for all these suggestions, that'll clean things up a bit.
Attached patch test consumeanchor behaviour, (obsolete) — Splinter Review
Well, that took unnecessarily long... it took me a long time to realize that the fact that these were arrow panels mattered. This test fails if you remove the consumeanchor attributes (tested on Windows).
Attachment #8460980 - Flags: review?(enndeakin)
Flags: firefox-backlog+
Comment on attachment 8460980 [details] [diff] [review]
test consumeanchor behaviour,

>+  function onMyPopupHidden(e) {
>+    ok(true, "Popup hidden");
>+    if (outerAnchor.id == "toolbarbutton-anchor") {
>+      popupHasShown = false;
>+      outerAnchor = document.getElementById("hbox-anchor");
>+      anchor = document.getElementById("inner-anchor");
>+      SimpleTest.waitForFocus(onFocus, window);

You shouldn't need waitForFocus here. The window is already focused.


>+  function onMyPopupShown(e) {
>+    popupHasShown = true;
>+    SimpleTest.waitForFocus(function() {
>+      synthesizeNativeMouseClick(outerAnchor, 5, 5);
>+    }, window);

Same here.
Attachment #8460980 - Flags: review?(enndeakin) → review+
(In reply to Neil Deakin from comment #4)
> Comment on attachment 8460980 [details] [diff] [review]
> test consumeanchor behaviour,
> 
> >+  function onMyPopupHidden(e) {
> >+    ok(true, "Popup hidden");
> >+    if (outerAnchor.id == "toolbarbutton-anchor") {
> >+      popupHasShown = false;
> >+      outerAnchor = document.getElementById("hbox-anchor");
> >+      anchor = document.getElementById("inner-anchor");
> >+      SimpleTest.waitForFocus(onFocus, window);
> 
> You shouldn't need waitForFocus here. The window is already focused.
> 
> 
> >+  function onMyPopupShown(e) {
> >+    popupHasShown = true;
> >+    SimpleTest.waitForFocus(function() {
> >+      synthesizeNativeMouseClick(outerAnchor, 5, 5);
> >+    }, window);
> 
> Same here.

Done:

remote:   https://hg.mozilla.org/integration/fx-team/rev/ee1a53319c19
Whiteboard: [fixed-in-fx-team]
I had to back this out in https://hg.mozilla.org/integration/fx-team/rev/b16543ebbb48 because it apparently somehow made mochitest-other intermittently fail on OSX 10.6 with https://tbpl.mozilla.org/php/getParsedLog.php?id=44473505&tree=Fx-Team

I retriggered a bunch on the previous two pushes and they came back solid green.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Wes Kocher (:KWierso) from comment #6)
> I had to back this out in
> https://hg.mozilla.org/integration/fx-team/rev/b16543ebbb48 because it
> apparently somehow made mochitest-other intermittently fail on OSX 10.6 with
> https://tbpl.mozilla.org/php/getParsedLog.php?id=44473505&tree=Fx-Team
> 
> I retriggered a bunch on the previous two pushes and they came back solid
> green.

Hey look, it's another one for the annals of "wat". :-(

The cases in which it fails aren't even consistent.

I imagine it might be a question of moving the mouse pointer back to where it is before the test starts... but I don't know how to get that information.
Attached patch test consumeanchor behaviour, (obsolete) — Splinter Review
Trying to move the mouse out of the way based on the suspicion that's what's causing the failures here...: remote:   https://tbpl.mozilla.org/?tree=Try&rev=aebdb4c89b87
Yeah, the colorpicker changes the current colour when hovering over it, and the test failures above suggest a color a few cells down is being selected instead.
Comment on attachment 8461542 [details] [diff] [review]
test consumeanchor behaviour,

Review of attachment 8461542 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming the tests come back green post-retriggers, does that mean this looks OK to you, Neil? I couldn't find a way to track the previous mouse position, so I just picked one.

I looked at using mozMouseMoveX/Y and tallying everything up so as to move it back, but AFAICT they are wrong for the first mouse event in the test if the mouse is over the window when the browser starts up and the mouse isn't moved until that time (all of which I suspect is/might be true on our automation infra), so that wasn't reliable.
Attachment #8461542 - Flags: review?(enndeakin)
Flags: needinfo?(gijskruitbosch+bugs)
Updated per discussion on IRC to actually use the mouseMove constant...
Attachment #8461731 - Flags: review?(enndeakin)
Attachment #8461542 - Attachment is obsolete: true
Attachment #8461542 - Flags: review?(enndeakin)
Attachment #8460980 - Attachment is obsolete: true
Attachment #8461731 - Flags: review?(enndeakin) → review+
Let's try that again:

remote:   https://hg.mozilla.org/integration/fx-team/rev/d6a0fd7c2e93
https://hg.mozilla.org/mozilla-central/rev/d6a0fd7c2e93
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla34
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.