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

VERIFIED FIXED in Firefox 55

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks 2 bugs, {regression})

55 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55+ fixed, firefox56+ verified)

Details

(Whiteboard: [photon-structure])

Attachments

(1 attachment)

[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.
Whiteboard: [photon-structure] [triage]
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon-structure] [triage] → [photon-structure]
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.3 - Jul 24
Priority: P2 → P1
Iteration: 56.3 - Jul 24 → 56.2 - Jul 10
Tracking - sounds like we may want to uplift this once it lands.
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
Attachment #8884321 - Flags: review?(jaws) → review+
(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%?
Flags: needinfo?(jaws)
(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.
Flags: needinfo?(jaws)
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Pushed by gijskruitbosch@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/13302a007a78
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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
Attachment #8884321 - Flags: approval-mozilla-beta?
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+
Attachment #8884321 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
QA Whiteboard: [bugday-20170712]
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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.
Status: VERIFIED → RESOLVED
Closed: 2 years ago2 years ago
Flags: qe-verify+
Verified on Windows, Mac, and Linux.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.