Closed Bug 1551412 Opened 6 months ago Closed 6 months ago

Editor context menu is broken in the Saved Logins window

Categories

(Firefox :: Menus, defect, P1)

68 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- unaffected
firefox67.0.1 --- unaffected
firefox68 + verified
firefox69 --- verified

People

(Reporter: nayinain, Assigned: bgrins)

References

(Regression)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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

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

Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Menus
Keywords: regression
Regressed by: 1500626

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

Brian, can you take a look?

Flags: needinfo?(bgrinstead)
Priority: -- → P1

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

Assignee: nobody → bgrinstead
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(bgrinstead)

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

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.

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
Flags: needinfo?(emilio)
See Also: → 1551781

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?

Flags: needinfo?(emilio) → needinfo?(bgrinstead)

(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

Flags: needinfo?(bgrinstead)

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.

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.

Duplicate of this bug: 1551781

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

Flags: needinfo?(emilio)
Attached file 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:

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?).

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

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.

Flags: needinfo?(emilio)
Flags: needinfo?(bgrinstead)

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

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

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.

Flags: needinfo?(bgrinstead)

needinfo for Comment 20

Flags: needinfo?(emilio)
Flags: needinfo?(emilio)

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

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

Attachment #9065108 - Attachment is obsolete: true
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2968b165e90c
Make XUL popup events composed. r=smaug
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Flags: qe-verify+

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.