1042092 - Write test for consumeanchor

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[:Gijs (he/him)]

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.

Comment 1

[Marco Mucci [:MarcoM]]

Hi Gijs, can you mark this bug as either [qa+] or [qa-] for verification.

Comment 2

[:Gijs (he/him)]

(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.

Comment 3

[:Gijs (he/him)]

Attachment: test consumeanchor behaviour,

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).

Comment 4

[Neil Deakin]

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.

Comment 5

[:Gijs (he/him)]

(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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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.

Comment 7

[:Gijs (he/him)]

(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.

Comment 8

[:Gijs (he/him)]

Attachment: test consumeanchor behaviour,

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

Comment 9

[Neil Deakin]

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 10

[:Gijs (he/him)]

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.

Comment 11

[:Gijs (he/him)]

Attachment: test consumeanchor behaviour,

Updated per discussion on IRC to actually use the mouseMove constant...

Comment 12

[:Gijs (he/him)]

Let's try that again:

remote:   https://hg.mozilla.org/integration/fx-team/rev/d6a0fd7c2e93

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6a0fd7c2e93