Closed Bug 1403965 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: General, defect, P2)

All
Windows
defect

Tracking

(firefox-esr52 unaffected, firefox-esr60 verified, firefox55 unaffected, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 wontfix, firefox60 wontfix, firefox61 verified, firefox62 verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- verified
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified
firefox62 --- verified

People

(Reporter: mcoman, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

Attached image 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:
Kris, can you please take a look at this?
Flags: needinfo?(kmaglione+bmo)
Blocks: webext-oop
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P2
Is it too late for 60 too? We started having user feedback about that one.
Hey David, should we up the priority of this / find an owner?
Flags: needinfo?(ddurst)
I don't think this is a webextension issue.
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).
Assignee: nobody → mixedpuppy
Flags: needinfo?(ddurst)
Blocks: 1355331
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?
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
Summary: The context menu is displayed in a wrong position after moving the "Notes" sidebar to right / left → [OOP] The context menu is displayed in a wrong position after moving extension sidebar to right / left
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!
Attachment #8969363 - Flags: review?(bugmail)
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`?
Attachment #8969363 - Flags: review+
What was the rationale for tracking this for 60?
Flags: needinfo?(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.
Flags: needinfo?(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.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e2573db9d14e
fix context menu position in extension sidebar, r=kmag
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.
Flags: qe-verify+
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
Flags: needinfo?(mixedpuppy)
(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.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/93d49d1a2688
fix context menu position in extension sidebar, r=kmag
patch updated and run through try the re-landed.
Flags: needinfo?(mixedpuppy)
https://hg.mozilla.org/mozilla-central/rev/93d49d1a2688
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Verified as fixed in Firefox 62.0a1 and Firefox 61.0b3. I will attach a postfix video.
Status: RESOLVED → VERIFIED
Attached image Postfix video
Please request uplift to esr60 if you think we should fix it there.
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy)
Attachment #8969363 - Flags: approval-mozilla-esr60?
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
Attachment #8969363 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Verified as fixed in ESR60
Flags: qe-verify+
Product: Toolkit → WebExtensions
See Also: → 1535515
You need to log in before you can comment on or make changes to this bug.