1551412 - Editor context menu is broken in the Saved Logins window

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P1)

Description

[nayinain]

Attachment: Screenshots.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

1. Open the Saved Logins window.
2. Double click to make the Username cell editable.
3. Right click the editor.

Actual results:

The Editor context menu is broken.

Expected results:

---

Sorry for my bad English.

Comment 1

[nayinain]

regression range:

2019-05-14T13:49:41: INFO : Narrowed inbound regression window from [ff204728, a21eae6d] (3 builds) to [ff204728, 37d758a9] (2 builds) (~1 steps left)
2019-05-14T13:49:41: DEBUG : Starting merge handling...
2019-05-14T13:49:41: DEBUG : Using url: https://hg.mozilla.org/integration/autoland/json-pushes?changeset=37d758a90ed9fc26c1d09993ddf73823295ea5d0&full=1
2019-05-14T13:49:42: DEBUG : Found commit message:
Bug 1500626 - Convert <menuitem> bindings to a Custom Element r=surkov

Differential Revision: https://phabricator.services.mozilla.com/D9322

2019-05-14T13:49:42: DEBUG : Did not find a branch, checking all integration branches
2019-05-14T13:49:42: INFO : The bisection is done.
2019-05-14T13:49:42: INFO : Stopped

Comment 2

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Regression in important UI.

Brian, can you take a look?

Comment 3

[Brian Grinstead [:bgrins]]

Thanks for filing, will take a look. At first glance, it seems the "popupshowing" listener isn't firing for this menu - maybe that's because the <textbox> is inside the tree shadow DOM (https://searchfox.org/mozilla-central/rev/cc280c4be94ff8cf64a27cc9b3d6831ffa49fa45/toolkit/content/widgets/tree.js#523). This means we don't end up running the code to render the menuitem labels (https://searchfox.org/mozilla-central/rev/cc280c4be94ff8cf64a27cc9b3d6831ffa49fa45/toolkit/content/widgets/menu.js#144).

Comment 4

[Brian Grinstead [:bgrins]]

Had some discussion on #content about this: https://mozilla.logbot.info/content/20190514#c16309562

you can make the event composed and it should go outside of the shadow dom IIRC
but wheres the listener that cares about what happens inside the shadow dom?

this is a perf optimization to avoid rendering menuitem labels etc until they are going to be visible

there's a chrome-only event handler that you could use, but making the event composed should work as well since it's internal
DocShell::chromeEventHandler
but marking the event composed might be better

Comment 5

[Brian Grinstead [:bgrins]]

Oddly, the popupshowing listener does fire on the capture listener on window in browser.xul but not on window in passwordManager.xul (where the input lives). However we early return in that case because it's not in the same document: https://searchfox.org/mozilla-central/rev/cc280c4be94ff8cf64a27cc9b3d6831ffa49fa45/toolkit/content/widgets/menu.js#145-147.

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: menupopups-shadow-dom.gif

This is a demo of what I mean in Comment 5. This includes a console.log("Captured popupshowing", e.originalTarget, e.originalTarget.ownerDocument.documentURI, document.documentURI); call right before https://searchfox.org/mozilla-central/rev/cc280c4be94ff8cf64a27cc9b3d6831ffa49fa45/toolkit/content/widgets/menu.js#145 when the popupshowing event is captured.

When the first menupopup (the context menu on the tree row) opens I see the event captured in both documents, which is expected.

I was expecting the second menupopup (the broken edit context menu inside of Shadow DOM) to not be captured by any of the handlers. But instead I see it only captured in browser.xul. Does this look like a bug, or is it expected?

Sidenote that I'm confused why we aren't seeing any of the events preferences.xul as well since menu.js is loaded in it, and the DOM looks like is:

browser.xul
  <browser> preferences.xul
     <browser> passwordManager.xul <- menupopups in here are opening

Comment 7

[Emilio Cobos Álvarez (:emilio)]

I'm a bit confused though, the textbox is in Shadow DOM, but the menupopup does not seem to be in Shadow DOM, right?

We should firing the popupshowing event at the popup, not the textbox (as proved by originaltarget). If so there should be no change as to how the event gets propagated, as far as I can tell.

I see there could be problems if stashing XUL popups in Shadow DOM (all the GetUncomposedDoc() usage in layout/xul/nsXULPopupManager.cpp is probably bogus), but as long as that's not the case I'm confused about what the difference is.

If you trigger the event with menu.dispatchEvent(new MouseEvent("popupshowing")), do you see the same behavior?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

I'm a bit confused though, the textbox is in Shadow DOM, but the menupopup does not seem to be in Shadow DOM, right?

The menupopup is inside the Shadow DOM (since it's inside of the <textbox>). The structure looks like this:

<tree>
  # Shadow Root
     <textbox> (Custom Element w/ non-anonymous content)
        <menupopup>

The <textbox> is appended as a child of shadow DOM in <tree> at https://searchfox.org/mozilla-central/rev/116bd975c30746ddefc3d20e6947d1871469354f/toolkit/content/widgets/tree.js#523

The menupopup is appended as a child of <textbox> at https://searchfox.org/mozilla-central/rev/116bd975c30746ddefc3d20e6947d1871469354f/toolkit/content/widgets/textbox.js#79

Comment 9

[Brian Grinstead [:bgrins]]

If you trigger the event with menu.dispatchEvent(new MouseEvent("popupshowing")), do you see the same behavior?

Just tested that and I see the same behavior as when the popup gets shown due to the right click context menu. I get the captured event from the listener in browser.xul, but don't get it from the listener in passwordManager.xul.

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: Bug 1551412 - Workaround popupshowing event not being captured outside of Shadow DOM for rendering <menuitem>

Just in case we don't come up with a more robust fix before the merge, this fixes
the saved logins and edit bookmark textbox context menus.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I'll take a closer look at this, no worries.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: event-target-chain.txt

Yeah, so I think this is behaving as expected. If you take a look at the target chain, the shadow root is the last target in that document, then we go look at the message manager, and all the parent scopes, which happen to have the parent document's window by chance.

I confirmed that we end up here:

• https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/dom/base/ShadowRoot.cpp#448

So possible fix is either:

• Mark popupshowing as composed (a bit sketchy, but fine), or...
• Add the event listener to the window's message manager (not sure about whether it's sketchy or not, but that's where we handle the contextmenu events as well, so maybe fine?).

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1551412 - Make XUL popup events composed. r=smaug

The front-end relies on getting the event for nodes in a ShadowRoot.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

I'll send a patch to make the popup events composed, but let me know if you want to try out either the other solution, or reworking the thing. Is saw a couple other window.addEventListener("popupshowing", ..) that looked like needing similar changes.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

For comment 15.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

• Add the event listener to the window's message manager (not sure about whether it's sketchy or not, but that's where we handle the contextmenu events as well, so maybe fine?).

Where do we do this with contextmenu events? I'd be happy to try this, but not sure how to add an event listener onto the messagemanger (like, is this just on the window.messageManager object?).

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Yeah: https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/browser/base/content/tabbrowser.js#47

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #18)

Yeah: https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/browser/base/content/tabbrowser.js#47

OK, I tried adding that for popupshowing but there are 2 problems. (1) it doesn't seem to fire even in browser.xul when popups open and (2) when opening about:preferences I get an error window.messageManager is undefined.

I think that line is only receiving a message from content (likely this one https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/browser/actors/ContextMenuChild.jsm#597) and won't work for our case.

Comment 20

[Brian Grinstead [:bgrins]]

I applied your patch locally and confirmed the fix. Unless if you have ideas on Comment 19 it seems like this might be the simplest solution for now.

Could changing the event to be composed have other side effects? I'm wondering if it's safe to land during soft freeze, or if we should aim for a frontend workaround fix for 68 and land the event change after merge.

Comment 21

[Brian Grinstead [:bgrins]]

needinfo for Comment 20

Comment 22

[Emilio Cobos Álvarez (:emilio)]

I think it should be pretty safe, the composed flag is only read here: https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/dom/base/ShadowRoot.cpp#439

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

ok, conceptually those events shouldn't be composed, but I guess it doesn't matter too much.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #22)

I think it should be pretty safe, the composed flag is only read here: https://searchfox.org/mozilla-central/rev/11cfa0462a6b5d8c5e2111b8cfddcf78098f0141/dom/base/ShadowRoot.cpp#439

Great! I'll suggest we go with your patch then. Thanks for looking into this.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2968b165e90c
Make XUL popup events composed. r=smaug

Comment 26

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2968b165e90c

Comment 27

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using an older version of Nightly (2019-05-13) on Windows 10 x64.
I retested everything using latest Nightly 69.0a1 and beta 68.0b6 on Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x32 and the bug is not reproducing anymore.