1900757 - webext-panels.xhtml's contentAreaContextMenu doesn't call updateEditUIVisibility

Status: REOPENED

(Firefox :: Menus, task)

Description

[Tom Schuster]

Unlike its main-popupset.inc.xhtml counterpart, the contentAreaContextMenu popup event listeners in webext-panels.xhtml are missing the call to updateEditUIVisibility.

From https://phabricator.services.mozilla.com/D211561

Comment 1

[Rob Wu [:robwu]]

What's the impact of this inconsistency?

I can trace the origin of the updateEditUIVisibility addition to bug 404232. And somehow the logic was absent when it was introduced in bug 1208596.

Shane, long shot - do you recall why updateEditUIVisibility was missing when webext-panels.xhtml was added in bug 1208596?

Comment 2

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #1)

What's the impact of this inconsistency?

I expect that the enabled/disabled state of cut/copy/paste and similar edit/clipboard-related functionality can be wrong in the extension sidebar context menu.

Comment 3

[Rob Wu [:robwu]]

The logic appears to be no-op on macOS: https://searchfox.org/mozilla-central/rev/a26e972e97fbec860400d80df625193f4c88d64e/browser/base/content/browser.js#2902-2904

I tested on Linux, by installing an arbitrary sidebar extension (Tree Style Tabs) and adding an input field through about:debugging to the sidebar.
For good measure I also added the Edit controls from the customizable menu to the toolbar. When I focus the input field, type text/undo typing, modify the clipboard, etc., the menu controls update as expected. The context menu also shows the expected state. The Edit menu in the toolbar also shows the expected state when I open it. I also tried entering fullscreen, with no difference in observed behavior.

Wondering for the reason, I searched for cmd_paste in the codebase and found https://searchfox.org/mozilla-central/rev/8011b6325f7ce05d228a3cdefd45d74fb98ee7b4/toolkit/content/editMenuOverlay.js#35. When I put a logpoint there (new Error().stack), I see that the method is called whenever the focus changes, including focus on tab content/sidebar content, whenever the context menu is opened.

Since the logic is called that frequently, and is skipped on macOS, I wonder whether updateEditUIVisibility is necessary at all, in other contexts.

Since everything seems to be working fine in the extension sidebar, I am inclined to close this bug. Feel free to re-open if you think that there is a reason for keeping this open.

Comment 4

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #3)

The logic appears to be no-op on macOS: https://searchfox.org/mozilla-central/rev/a26e972e97fbec860400d80df625193f4c88d64e/browser/base/content/browser.js#2902-2904

I tested on Linux, by installing an arbitrary sidebar extension (Tree Style Tabs) and adding an input field through about:debugging to the sidebar.
For good measure I also added the Edit controls from the customizable menu to the toolbar. When I focus the input field, type text/undo typing, modify the clipboard, etc., the menu controls update as expected. The context menu also shows the expected state. The Edit menu in the toolbar also shows the expected state when I open it. I also tried entering fullscreen, with no difference in observed behavior.

Wondering for the reason, I searched for cmd_paste in the codebase and found https://searchfox.org/mozilla-central/rev/8011b6325f7ce05d228a3cdefd45d74fb98ee7b4/toolkit/content/editMenuOverlay.js#35. When I put a logpoint there (new Error().stack), I see that the method is called whenever the focus changes, including focus on tab content/sidebar content, whenever the context menu is opened.

Since the logic is called that frequently, and is skipped on macOS, I wonder whether updateEditUIVisibility is necessary at all, in other contexts.

AFAICT this is for paste, and for cut/copy/select/delete, whether we update commands depends on gEditUIVisibility, which is updated from updateEditUIVisibility, so it'd be needed for that, right? Or am I missing something else here?

Since everything seems to be working fine in the extension sidebar, I am inclined to close this bug. Feel free to re-open if you think that there is a reason for keeping this open.

I mean, either we should call it for the webpage context menu everywhere, or we should not call it anywhere. The half-and-half case is clearly buggy, it's just not clear in which direction (ie should we remove it or call it for webextensions, too).

Neil, can you help?

Comment 5

[Neil Deakin]

updateEditUIVisibility() is just an optimization to prevent updating the ui state when nothing is visible that would need updating. This function updates gEditUIVisible to that effect. As long as gEditUIVisible remains true, which is default value, then UI state updating should work ok.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

As long as gEditUIVisible remains true, which is default value, then UI state updating should work ok.

Are we sure it's the default value? Launching a new temporary profile and checking the value of that variable, it looks like the default to me is undefined...

So here's my understanding so far:

1. We want to update cut/copy/paste/select all/change text direction commands on focus changes ONLY if UI is exposed that might trigger the cut/copy/paste/select all/change text decoration commands. Seems reasonable, considering how often focus changes occur and how expensive updating the commands was (though it might be worth seeing if this optimization even makes sense anymore - maybe we're faster now?)
2. If the optimization still makes sense, it means that any chunk of UI that might expose those commands needs to know to update the visibility state. That seems fragile - it looks like we've already forgotten to do that at least once.

So I think we have two things to do here:

1. We need to determine whether or not this optimization still has value. If not, we should remove it to simplify everybody's lives.
2. If it does have value, we need some way of making sure that current (and future) surfaces that expose these commands update the state accordingly.

Am I missing anything, Neil?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Unless there are serious objections, I'm going to re-open this. It seems like there's still some investigation work to do here.

Comment 8

[Neil Deakin]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #6)

Are we sure it's the default value? Launching a new temporary profile and checking the value of that variable, it looks like the default to me is undefined...

It is set to true or false for me. On Mac it will be undefined, as the menu bar is always visible and gEditUIVisible is unused:
https://searchfox.org/mozilla-central/rev/dea459eb01a4c38b696ae3d31c1540e86365a937/browser/base/content/browser.js#650

Comment 9

[Tomislav Jovanovic :zombie]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #6)

1. We need to determine whether or not this optimization still has value. If not, we should remove it to simplify everybody's lives.
2. If it does have value, we need some way of making sure that current (and future) surfaces that expose these commands update the state accordingly.

I suppose this be a firefox-wide change? If so, should this bug be moved to fronted (probably Firefox::Menus)?

Comment 10

[:Gijs (he/him)]

I did some digging here.

As already discussed, gEditUIVisibility is undefined on macOS and it defaults to true elsewhere.

Both browser.xhtml and the subframe webext-panels.xhtml end up loading both editMenuOverlay.js and browser.js, and the context menu code. browser.js has the updateEditUIVisibility helper; editMenuOverlay.js has the code that updates cut/copy and other command states if and only if gEditUIVisibility is undefined or true (or it's passed a "force update" flag).

In the browser.xhtml case, a bunch of things can trigger changes to gEditUIVisibility - customization changes, showing and hiding the hamburger panel, etc. etc. None of those things appear to apply in the webext-panels.xhtml frame itself.

Focus changes cause commandupdate events to fire... but only in the parent document (browser.xhtml), as far as I can tell.

So the only time we update the command state in the webext-panels.xhtml case is when showing and hiding the context menu - and because we never update gEditUIVisibility, it stays true because that's what it is to start with, and so we "really" update the command states rather than bailing out early because gEditUIVisibility is false. If we ever changed the initial value, things would break (the cut/copy command states would be "stuck" in whatever disabled state they start off with).

Conversely, nothing would break if we ran updateEditUIVisibility consistently in the webext-panels case as well, and that'd make the code resilient to any changes to the default value, and more likely to behave correctly if we added other copy/paste UI in the same frame.

The thing that still confuses me a bit is that ISTM that disabling these commands in the webext-panels.xhtml frame doesn't really do anything - the keyboard shortcuts still work fine (presumably because they're not disabled in the same-process parent frame?). Neil, is that expected? Should we just... not have these commands exist in the child frame at all, and have the context menu use top or whatever so it looks for the relevant command in the parent frame? That might solve some level of confusion here...

(In reply to Mike Conley (:mconley) (:⚙️) from comment #6)

Seems reasonable, considering how often focus changes occur and how expensive updating the commands was (though it might be worth seeing if this optimization even makes sense anymore - maybe we're faster now?)

I think we have recent evidence that e.g. clipboard consultation is still expensive. Hopefully just "is there something there" is cheaper, but it seems reasonable to keep the optimization for now...

2. If it does have value, we need some way of making sure that current (and future) surfaces that expose these commands update the state accordingly.

A "thorough" fix would be refactoring a bunch of this stuff so it was self-contained and worked in any window, rather than requiring a motley collection of script files. But at least initially, I think we should just remove the conditionals that make webext-panels behave differently, here. I'll file a follow-up to rethink how some of these bits interact with each other.

(In reply to Tomislav Jovanovic :zombie from comment #9)

(In reply to Mike Conley (:mconley) (:⚙️) from comment #6)

1. We need to determine whether or not this optimization still has value. If not, we should remove it to simplify everybody's lives.
2. If it does have value, we need some way of making sure that current (and future) surfaces that expose these commands update the state accordingly.

I suppose this be a firefox-wide change? If so, should this bug be moved to fronted (probably Firefox::Menus)?

Right now, webextensions are the "odd one out", and the 4-line fix I'm suggesting is specific to them - just in browser code... I can move it if that's helpful.

Comment 11

[Neil Deakin]

The thing that still confuses me a bit is that ISTM that disabling these commands in the webext-panels.xhtml frame doesn't really do anything - the keyboard shortcuts still work fine (presumably because they're not disabled in the same-process parent frame?). Neil, is that expected?

Is webext-panels.xhtml a child frame or a top-level window?

Commands and command updating only need to exist at the root level. That will also handle any subframes that are in the same process. The clipboard commands will execute using whatever is focused.

I seem to recall that keyboard shortcuts don't always check the enabled state, but you can use the edit menu to verify the command state or customize the toolbar to have the clipboard buttons as an alternative.

I think we have recent evidence that e.g. clipboard consultation is still expensive. Hopefully just "is there something there" is cheaper, but it seems reasonable to keep the optimization for now...

That bug looks like the code is trying the read the clipboard contents to determine if a url is present? That can be quite slow as it needs to get the data from the other application. Perhaps it should just be checking the available formats instead which shouldn't have this issue?

Comment 12

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #11)

The thing that still confuses me a bit is that ISTM that disabling these commands in the webext-panels.xhtml frame doesn't really do anything - the keyboard shortcuts still work fine (presumably because they're not disabled in the same-process parent frame?). Neil, is that expected?

Is webext-panels.xhtml a child frame or a top-level window?

A child frame.

Commands and command updating only need to exist at the root level. That will also handle any subframes that are in the same process. The clipboard commands will execute using whatever is focused.

I seem to recall that keyboard shortcuts don't always check the enabled state, but you can use the edit menu to verify the command state or customize the toolbar to have the clipboard buttons as an alternative.

Presumably this doesn't work in terms of assessing the enabled state inside the child frame as they don't have these buttons nor a menubar. But then it also sounds like the state at the child frame level shouldn't matter at all?

Comment 13

[Neil Deakin]

If it is a child frame in the same process, then updateEditUIVisibility() would need to be called on the top level window since that function doesn't make sense running anywhere else.

I wonder if it would be easier to just call topWindow.goUpdateGlobalEditMenuItems(true) when opening the context menu and not bothering with the updateEditUIVisibility() call at all in there.