Closed Bug 1555453 Opened 1 year ago Closed 1 year ago

Some TabContextMenu items are blank label at first time

Categories

(Firefox :: Tabbed Browser, defect, P3, minor)

68 Branch
x86_64
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 --- fixed
firefox69 --- verified

People

(Reporter: alice0775, Assigned: Gijs)

References

(Regression)

Details

(Keywords: regression, reproducible)

Attachments

(2 files)

Attached image 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

Has Regression Range: --- → yes
Has STR: --- → yes

Gijs, thoughts on how to address this?

Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P3

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?

Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dao+bmo)

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

Flags: needinfo?(jaws)

(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...

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

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/92923a205734
use contextmenu event as backup to ensure we always localize the context menu, r=jaws
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69
Flags: needinfo?(dao+bmo)

Should we uplift this to 68?

Flags: needinfo?(gijskruitbosch+bugs)

Alice, can you confirm this is fixed for you?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(alice0775)

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

Flags: needinfo?(alice0775)

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
Attachment #9069059 - Flags: approval-mozilla-beta?

Marking verified per comment #11 - thanks Alice!

Status: RESOLVED → VERIFIED

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

Attachment #9069059 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.