1378427 - Context menus flicker when opened on edit/zoom controls in the panel menu

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[:Gijs (he/him)]

[Tracking Requested - why for this release]:

STR:

1. open old-style (non-photon) menu panel
2. right click the 'cut' button
3. hover over the first context menu entry

ER:
no flickering

AR:
flickering hover state

I expect I broke this in bug 1354078.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1378427 - move context menus around, fix flickering in dynamic portion of the overflow menu for photon,

Apparently the hover state of the combined buttons does not interact well with the menus unless they
share the same containing popup/panel. We broke that condition in bug 1354078. The expedient thing
is to simply move the popup back, and move it back and forth if/when the photon pref is flipped.
When removing the pref, we can simplify this by always putting the menu in the overflow panel.

I also noticed that we use the toolbar context menu in the dynamic portion of the overflow panel.
This has the same problem, and to fix it I switched us to using the same (panel) context menu in
the photon case. Doing this in the non-photon case won't help because the context menu will be in
a separate panel (namely the old hamburger panel) entirely.

Review commit: https://reviewboard.mozilla.org/r/155256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155256/

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking - sounds like we may want to uplift this once it lands.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8884321 [details]
Bug 1378427 - move context menus around, fix flickering in dynamic portion of the overflow menu for photon,

https://reviewboard.mozilla.org/r/155256/#review160954

::: browser/components/customizableui/content/panelUI.inc.xul:364
(Diff revision 1)
>      </panelview>
>  
>    </panelmultiview>
> +
> +  <!-- This menu is here because not having it in the menu in which it's used flickers
> +       when hover styles overlap. See https://bugzilla.mozilla.org/show_bug.cgi?id=1378427 .

Do we understand the root cause of the bug here? Why are :hover styles overlapping? The contextmenu shouldn't have pointer-events going through it. Does the restyle due to :hover cause the contextmenu to momentarily have something akin to pointer-events:none?

::: browser/components/customizableui/test/browser_880164_customization_context_menus.js:319
(Diff revision 1)
> -  contextMenu.childNodes[0].doCommand();
> +  let moveToToolbar = contextMenu.querySelector("menuitem:not([hidden])");
> +  moveToToolbar.doCommand();

Can you write a more specific selector? Finding the first non-hidden one and executing it opens this up to fail unexpectedly in the future.

::: browser/components/customizableui/test/browser_photon_customization_context_menus.js:330
(Diff revision 1)
> -  contextMenu.childNodes[0].doCommand();
> +  let moveToToolbar = contextMenu.querySelector("menuitem:not([hidden])");
> +  moveToToolbar.doCommand();

Ditto

Comment 4

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Comment on attachment 8884321 [details]
> Bug 1378427 - move context menus around, fix flickering in dynamic portion
> of the overflow menu for photon,
> 
> https://reviewboard.mozilla.org/r/155256/#review160954
> 
> ::: browser/components/customizableui/content/panelUI.inc.xul:364
> (Diff revision 1)
> >      </panelview>
> >  
> >    </panelmultiview>
> > +
> > +  <!-- This menu is here because not having it in the menu in which it's used flickers
> > +       when hover styles overlap. See https://bugzilla.mozilla.org/show_bug.cgi?id=1378427 .
> 
> Do we understand the root cause of the bug here?

Nope.

> Why are :hover styles
> overlapping? The contextmenu shouldn't have pointer-events going through it.
> Does the restyle due to :hover cause the contextmenu to momentarily have
> something akin to pointer-events:none?

How would I test this hypothesis / debug this? Given I'm basically reverting a change, are you OK landing this even without figuring this out 100%?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs from comment #4)
> > Why are :hover styles
> > overlapping? The contextmenu shouldn't have pointer-events going through it.
> > Does the restyle due to :hover cause the contextmenu to momentarily have
> > something akin to pointer-events:none?
> 
> How would I test this hypothesis / debug this? Given I'm basically reverting
> a change, are you OK landing this even without figuring this out 100%?

Yeah, I'm okay with landing this as-is. I imagine to test / debug this you would need to trace the mouse events code and I don't think this is high enough priority to spend the time working on it or asking others to look in to it. But it's worth noting in case we see some other shenanigans down the road.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8884321 [details]
Bug 1378427 - move context menus around, fix flickering in dynamic portion of the overflow menu for photon,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155256/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13302a007a78
move context menus around, fix flickering in dynamic portion of the overflow menu for photon, r=jaws

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/13302a007a78

Comment 9

[:Gijs (he/him)]

Comment on attachment 8884321 [details]
Bug 1378427 - move context menus around, fix flickering in dynamic portion of the overflow menu for photon,

Approval Request Comment
[Feature/Bug causing the regression]: bug 1354078
[User impact if declined]: flickering context menus in the overflow / hamburger panels
[Is this code covered by automated tests?]: yes, the context menus are covered by tests which had to be updated for these changes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
1. ensure the zoom and edit controls are in the hamburger panel (non-photon - use the cedar twig builds once this change has merged there) / beta) or overflow panel (photon + nightly)
2. right click them
3. hover over the items in the context menu that overlap with the zoom / edit controls

Expected: no flickering of the hover state
Pre-patch: hover state flickers on OS X and Windows

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not very
[Why is the change risky/not risky?]: There's still some beta runway left, the actual code is subject to decent automated test coverage and so I'm pretty confident this isn't causing behavioural regressions.
[String changes made/needed]: nope

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8884321 [details]
Bug 1378427 - move context menus around, fix flickering in dynamic portion of the overflow menu for photon,

fix a visual regression with context menus on hamburger panel, beta55+

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d85768302b22

Comment 12

[Saddam Hossain]

I have reproduced this Bug on Nightly 56.0a1 (2017-07-05)  on Windows 10, 64 bit!

The bug's fix is now verified on latest  Nightly 56.0a1

Build ID    :	20170715030206
User Agent  : 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0

Comment 13

[:Gijs (he/him)]

This needs verifying on OS X as well (and Linux, if it was reproducible there, which I'm not sure about) and needs checking on beta. Re-adding qe-verify+ and unmarking as VERIFIED.

Comment 14

[u554753]

Verified on Windows, Mac, and Linux.