1403965 - [OOP] The context menu is displayed in a wrong position after moving extension sidebar to right / left

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P2)

Description

[Marius Coman [:mcoman], Ecosystem QA]

Attachment: context menu.gif

[Affected versions]:
- Firefox Notes 1.8.0 rc2
- Firefox 56 and up

[Affected Platforms]:
- All Windows

[Prerequisites]:
- Have a Firefox profile with the latest "Firefox Notes" add-on version (1.8.0 rc2) installed.

[Steps to reproduce]:
1. Open the browser with the profiles from prerequisites.
2. Right click in the "Notes" sidebar.
3. Click on the "Notes" button from the sidebar.
4. Choose "Move sidebar to right / left" from the sidebar's dropdown menu.
5. Right click in the "Notes" sidebar and observe the context menu position.

[Expected result]:
- The context menu is displayed under the mouse cursor.

[Actual result]:
- The context menu is wrongly displayed in the other side of the screen.

[Regression Window]:
Last good revision: 392ed89ec2730a48d10b1cec741e86a242d28aa3
First bad revision: a625a2e9b3333a8e76982ea65f077cfded6ac224
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=392ed89ec2730a48d10b1cec741e86a242d28aa3&tochange=a625a2e9b3333a8e76982ea65f077cfded6ac224

[Notes]:
- If you move the sidebar back to the initial side the issue is no longer reproducible.
- The issue is no longer reproducible if you switch the sidebars.
- Attached a screen recording of the issue:

Comment 1

[Marius Coman [:mcoman], Ecosystem QA]

Kris, can you please take a look at this?

Comment 2

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

Too late to fix this in 56.

Comment 3

[Rémy Hubscher (:natim)]

Is it too late for 60 too? We started having user feedback about that one.

Comment 4

[Jim Mathies [:jimm]]

Hey David, should we up the priority of this / find an owner?

Comment 5

[Shane Caraveo (:mixedpuppy)]

I don't think this is a webextension issue.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Correcting my last comment.  Some positioning used for the context menu event is setup when the webext panel is loaded, so the context menu has incorrect coordinates.  The bookmark/webpanel doesn't have the same issue, so I'll need to look into what is different between the two (I suspect though that the webpanel is not remote).

Comment 7

[Shane Caraveo (:mixedpuppy)]

Switching sides of the sidebar is done with the ordinal attribute.  Once the sidebar is switched, the context menu has the prior screenX/Y values until something is done to the window (moving or resizing the window or resizing the sidebar fixes this).  The issue here is lower level, gfx layer or dom.

Kris, any thoughts on this, or who should I talk to?

Comment 8

[Shane Caraveo (:mixedpuppy)]

Verifying again that event.screenX/Y are wrong as received in ContextMenu.jsm _setContext

- open sidebar on left, context click
**** coords 230 271
- move sidebar to right, context click
**** coords 212 293
- resize sidebar on right, context click
**** coords 1184 314

This is on osx (59 and nightly).

browser.tabs.remote.autostart = true causes the problem, if set to false context menus show in the correct location.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Fix for similar screen coordinate issues in Bug 1390445

https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/toolkit/mozapps/extensions/content/extensions.js#3521-3523

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1403965 fix context menu position in extension sidebar,

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

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969363 [details]
Bug 1403965 fix context menu position in extension sidebar,

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

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8969363 [details]
Bug 1403965 fix context menu position in extension sidebar,

https://reviewboard.mozilla.org/r/238108/#review243874

This looks ok to me but I'm not that familiar with this code. Somebody who knows the frontend code would be a better reviewer. Sorry!

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8969363 [details]
Bug 1403965 fix context menu position in extension sidebar,

https://reviewboard.mozilla.org/r/238108/#review243880

::: browser/base/content/browser-sidebar.js:197
(Diff revision 2)
> +
> +    let content = SidebarUI.browser.contentWindow;
> +    if (content && content.updatePosition) {
> +      setTimeout(() => {
> +        content.updatePosition();
> +      }, 1);

Why `1`? What happens if we use `0` or `Services.tm.dispatchToMainThread`?

Comment 14

[Julien Cristau [:jcristau]]

What was the rationale for tracking this for 60?

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to Julien Cristau [:jcristau] from comment #14)
> What was the rationale for tracking this for 60?

It's fairly broken behavior in the ui, a fix should be put into 60.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969363 [details]
Bug 1403965 fix context menu position in extension sidebar,

https://reviewboard.mozilla.org/r/238108/#review243880

> Why `1`? What happens if we use `0` or `Services.tm.dispatchToMainThread`?

I just went and tried both, neither work.  In fact, 1 didn't work, also tried 10 and it was intermittent.  I bumped it to 50 and that works.  

As I recall, I initially tried with zero, it didn't work, tried it with 100 and that worked, then I bumped it down to 1.  I probably forgot to build again (this file doesn't track well) when I tested that value.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969363 [details]
Bug 1403965 fix context menu position in extension sidebar,

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2573db9d14e
fix context menu position in extension sidebar, r=kmag

Comment 19

[Shane Caraveo (:mixedpuppy)]

STR:

1. load any extension with a sidebar
2. right click to open context menu
3. use sidebar dropdown to move sidebar to other side
3. right click to open context menu
4. repeat a few times

pass:

context menu always appears in sidebar where click occurred.

failure:

context menu appears on the opposite side from where the sidebar is.

Comment 20

[Andreea Pavel [:apavel]]

Backed out for failing browser chrome at browser/components/extensions/test/browser/test-oop-extensions/browser_ext_sidebarAction_runtime.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=e2573db9d14e05ccc90c80a5bab27a8db0d3c489

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175203576&repo=autoland&lineNumber=22986

Backout: https://hg.mozilla.org/integration/autoland/rev/681479cecc1f135ec182cd1ee8df3c53028d2934

Comment 21

[Julien Cristau [:jcristau]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> (In reply to Julien Cristau [:jcristau] from comment #14)
> > What was the rationale for tracking this for 60?
> 
> It's fairly broken behavior in the ui, a fix should be put into 60.

OK, I guess, but if this has been there since 56 the urgency right now is unclear to me.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969363 [details]
Bug 1403965 fix context menu position in extension sidebar,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238108/diff/3-4/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93d49d1a2688
fix context menu position in extension sidebar, r=kmag

Comment 24

[Shane Caraveo (:mixedpuppy)]

patch updated and run through try the re-landed.

Comment 25

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/93d49d1a2688

Comment 26

[Victor Carciu]

Verified as fixed in Firefox 62.0a1 and Firefox 61.0b3. I will attach a postfix video.

Comment 27

[Victor Carciu]

Attachment: Postfix video

Comment 28

[Julien Cristau [:jcristau]]

Please request uplift to esr60 if you think we should fix it there.

Comment 29

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8969363 [details]
Bug 1403965 fix context menu position in extension sidebar,

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Since esr is longer lived, it would be nice to fix this ux item.
User impact if declined: context menus appear in wrong location when users switch sidebar from side to side
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 30

[Julien Cristau [:jcristau]]

Comment on attachment 8969363 [details]
Bug 1403965 fix context menu position in extension sidebar,

ui glitch, fix verified in nightly and beta, approved for 60.1esr

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/788dd61d4625

Comment 32

[Victor Carciu]

Verified as fixed in ESR60