1555453 - Some TabContextMenu items are blank label at first time

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[Alice0775 White]

Attachment: screenshot

Reproducible: always

Steps To reproduce:

1. Right mousedown on empty space of content area
2. Keeping mousedown, move mouse pointer to tab
3. Then, mouseup

Actual Results:
Some TabContextMenu items are blank label

Expected Results:
No blank label

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a1cfb4874e277b523330f3595ac138897b49f41a&tochange=028c0eff90a7ad121827f45040699bbca433c5e7

Regressed by:
028c0eff90a7ad121827f45040699bbca433c5e7 Brian — Bug 1523763 - Move tab context menu strings to FTL file loaded on-demand. r=Gijs,flod

Comment 1

[Dão Gottwald [:dao]]

Gijs, thoughts on how to address this?

Comment 2

[:Gijs (he/him)]

I think we have a few options:

1. detect mousemoves/mouseovers as well, which is a little sad-making, perhaps, from a perf perspective
2. add fallback in onpopupshowing or oncontextmenu, which might cause a flash of unlocalized content, I guess?
3. move from mousedown to mouseup on windows (and/or linux? mac context menus appear onmousedown...) and/or use both
4. add some kind of complicated helper that intercepts an initial context menu showing, cancels it, and then re-opens it when l10n is ready

I'd be inclined to go for 2 or 3, but I'd like a second opinion. Jared, Dão, what do you think?

Comment 3

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

Can we add a capturing {once: true} mousedown event listener on the whole window and just initialize on that?

Comment 4

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)

Can we add a capturing {once: true} mousedown event listener on the whole window and just initialize on that?

Apparently I was confused and we already have a mouseover listener? I don't know off-hand why that wouldn't fire in the steps in comment #0...

Comment 5

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

The mouseover listener is only on the tabContainer. I assume holding down the right mousebutton while dragging it skips the mouseover event.

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1555453 - use contextmenu event as backup to ensure we always localize the context menu, r?jaws

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92923a205734
use contextmenu event as backup to ensure we always localize the context menu, r=jaws

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/92923a205734

Comment 9

[Julien Cristau [:jcristau]]

Should we uplift this to 68?

Comment 10

[:Gijs (he/him)]

Alice, can you confirm this is fixed for you?

Comment 11

[Alice0775 White]

Yes. It was fixed!.
No longer reproduced on latest Nightly69.0a1.
Build ID 20190603101337
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:69.0) Gecko/20100101 Firefox/69.0

Comment 12

[:Gijs (he/him)]

Comment on attachment 9069059 [details]
Bug 1555453 - use contextmenu event as backup to ensure we always localize the context menu, r?jaws

Beta/Release Uplift Approval Request

• User impact if declined: Broken context menu in some cases
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very small frontend-only patch, still got lots of beta runway.
• String changes made/needed: nope

Comment 13

[:Gijs (he/him)]

Marking verified per comment #11 - thanks Alice!

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9069059 [details]
Bug 1555453 - use contextmenu event as backup to ensure we always localize the context menu, r?jaws

approved for 68.0b8

Comment 15

[Oana Pop-Rus]

https://hg.mozilla.org/releases/mozilla-beta/rev/836d74518ab6