860994 - Story - Fixup intermittent context menu failing tests

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Tests, defect, P2)

Comment 1

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v1

Occasionally context menu popup hide would not trigger a transitionend event and would end up in a zombie state where it's there but with opacity 0. After that, trying to pop up or hide a new context menu would fail. I was able to reproduce it very frequently by reducing the transition time to 0.02s.

I'm still not 100% sure why this was happening, my feeling is that having a separate transition for opacity was confusing the engine somehow - may be worth investigating further.

After adding the hiding attribute to trigger the hiding transition, it never happened again (on my machine™).

Comment 2

[Matt Brubeck (:mbrubeck)]

Comment on attachment 736576 [details] [diff] [review]
Patch v1

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

::: browser/metro/base/content/helperui/MenuUI.js
@@ -380,2 @@
>        self._panel.hidden = true;
> -      self._popupState = null;

Why is this line removed?  Is _popupState nulled out elsewhere, or do we want it to remain set?

::: browser/metro/base/tests/mochitest/browser_context_menu_tests.js
@@ -34,5 @@
>      "Top position is " + aElement.top + ", expected between " + aMinTop + " and " + aMaxTop);
>  }
>  
>  gTests.push({
> -  desc: "text context menu",

I don't think this was a typo -- this is testing the context menus for selected *text*

Comment 3

[Rodrigo Silveira [:rsilveira] [:rodms]]

(In reply to Matt Brubeck (:mbrubeck) from comment #2)
> Comment on attachment 736576 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 736576 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/metro/base/content/helperui/MenuUI.js
> @@ -380,2 @@
> >        self._panel.hidden = true;
> > -      self._popupState = null;
> 
> Why is this line removed?  Is _popupState nulled out elsewhere, or do we
> want it to remain set?
> 

I removed it because it was never used. This was the only reference.

> ::: browser/metro/base/tests/mochitest/browser_context_menu_tests.js
> @@ -34,5 @@
> >      "Top position is " + aElement.top + ", expected between " + aMinTop + " and " + aMaxTop);
> >  }
> >  
> >  gTests.push({
> > -  desc: "text context menu",
> 
> I don't think this was a typo -- this is testing the context menus for
> selected *text*

Duh! Reverted it :)

Comment 4

[Rodrigo Silveira [:rsilveira] [:rodms]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37fd7284be3a

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37fd7284be3a

Comment 6

[Marco Mucci [:MarcoM]]

For Kamil to test and verify.

Comment 7

[Marco Mucci [:MarcoM]]

For Raymond to test and verify.

Comment 8

[raymond [:retornam] (inactive)]

verified fixed http://hg.mozilla.org/mozilla-central/rev/fef5f202b2dc

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

No need to verify this manually anymore; if the test starts breaking we'll find out about it when the test breaks.