Closed Bug 860994 Opened 8 years ago Closed 8 years ago

Story - Fixup intermittent context menu failing tests

Categories

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

x86_64
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 23

People

(Reporter: rsilveira, Assigned: rsilveira)

References

Details

(Whiteboard: feature=story c=testing u=developer p=1)

Attachments

(1 file)

No description provided.
Attached patch Patch v1Splinter Review
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™).
Attachment #736576 - Flags: review?(mbrubeck)
Attachment #736576 - Flags: review?(jmathies)
QA Contact: jbecerra
Whiteboard: p=1 → feature=story c=testing u=developer p=1
Summary: Fixup intermittent context menu failing tests → Story - Fixup intermittent context menu failing tests
Blocks: metrov1it6
No longer blocks: metrov1it5
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*
Attachment #736576 - Flags: review?(mbrubeck) → review+
(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 :)
https://hg.mozilla.org/mozilla-central/rev/37fd7284be3a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
For Kamil to test and verify.
Flags: needinfo?(kamiljoz)
Attachment #736576 - Flags: review?(jmathies)
For Raymond to test and verify.
Flags: needinfo?(kamiljoz) → needinfo?(mozbugs.retornam)
verified fixed http://hg.mozilla.org/mozilla-central/rev/fef5f202b2dc
Status: RESOLVED → VERIFIED
Flags: needinfo?(mozbugs.retornam)
No need to verify this manually anymore; if the test starts breaking we'll find out about it when the test breaks.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.