1692673 - "Cut" and "Copy" shouldn't be enabled when opening the page context menu on an input/textarea without a selection

Status: VERIFIED FIXED

(Firefox :: Menus, defect, P3)

Description

[:Gijs (he/him)]

UX has pointed out that we enable Cut and Copy even when there's no selection. This seems wrong.

Comment 1

[Neil Deakin]

It is odd, but this was intentional. See bug 1159490 for more details. The gist is that rather than come up with a way for a page to indicate that cut/copy/paste should be enabled, they are just always enabled. This only applies to web pages though; our chrome ui should be correct.

Chrome (the browser) seems to have a bug here. It disables the context menu commands, but the menubar/keyboard shortcuts continue to be enabled.

Comment 2

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #1)

It is odd, but this was intentional. See bug 1159490 for more details. The gist is that rather than come up with a way for a page to indicate that cut/copy/paste should be enabled, they are just always enabled. This only applies to web pages though; our chrome ui should be correct.

Chrome (the browser) seems to have a bug here. It disables the context menu commands, but the menubar/keyboard shortcuts continue to be enabled.

Shouldn't we still disable them when context-clicking inside a plaintext <input> or <textarea>? How would the webpage do anything meaningful there based on cut/copy events? :-\

Comment 3

[Neil Deakin]

I don't think I could answer that. Maybe nika might?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to :Gijs (he/him) from comment #2)

Shouldn't we still disable them when context-clicking inside a plaintext <input> or <textarea>? How would the webpage do anything meaningful there based on cut/copy events? :-\

We still fire clipboard events like the "copy" or "paste" event even if focus is in a normal plaintext <input> or <textarea>, and the webpage can intercept those and change what value you're copying, so the webpage can still meaningfully interact in this scenario.

As an example, if you load the data URI data:text/html,<input oncopy="event.clipboardData.setData('text/plain', 'hello, world!'); event.preventDefault();">, right click in the empty textarea, and select "Copy", you'll notice that the string "hello, world!" has been copied to your clipboard.

Comment 5

[:Gijs (he/him)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #4)

(In reply to :Gijs (he/him) from comment #2)

Shouldn't we still disable them when context-clicking inside a plaintext <input> or <textarea>? How would the webpage do anything meaningful there based on cut/copy events? :-\

We still fire clipboard events like the "copy" or "paste" event even if focus is in a normal plaintext <input> or <textarea>, and the webpage can intercept those and change what value you're copying, so the webpage can still meaningfully interact in this scenario.

As an example, if you load the data URI data:text/html,<input oncopy="event.clipboardData.setData('text/plain', 'hello, world!'); event.preventDefault();">, right click in the empty textarea, and select "Copy", you'll notice that the string "hello, world!" has been copied to your clipboard.

I understand that this is technically possible. My question was more about what would be a meaningful/reasonable use of this mechanism, if there is no selection anyway, and we're inside an inputfield.

It seems to me that the common case here is that there is no event handler, and so creating weird UX (enabled items that do nothing and aren't useful) for the uncommon/implausible case feels like the wrong decision.

If we're convinced this is important, could we use the event listener service to detect if a cut/copy listener exists, and disable the items when there's no selection and such a listener does not exist?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Yeah, we could potentially do something like that. I made the original change to force the options to always be available when I was an intern because we were not getting the option sometimes when we should, but if it's creating a bad UX we could definitely change it.

Something like checking for a cut/copy listener seems like it could be a reasonable approach if it's not too painful to implement.

Comment 7

[Jonathan Kew [:jfkthame]]

I tried a small patch to handle this in TextEditor::IsCut[Copy]CommandEnabled(), which seems to work OK in simple testing, at least. It checks for an event listener on the editor, but will not enable the commands if no text is selected but there's a cut/copy listener somewhere on an ancestor of the input element. Yet keyboard shortcuts will still send the command and the listener gets to handle it.

So this is arguably a bit inconsistent, but seems to me that it's a fairly obscure edge case, and probably doing the "right thing" in the simple case is worthwhile.

Anyhow, I'll post the patch for consideration.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1692673 - Avoid enabling Cut/Copy menu commands in text editor context when there's no selection, unless a listener is present. r=masayuki

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1692673 - Update tests for new Cut/Copy command-enabling behavior. r=masayuki

Depends on D107480

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

smaug: Do you think that event listener should be check only editing host or its inclusive ancestors? See https://phabricator.services.mozilla.com/D107480#inline-600709

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b1ba8dbfc8a
Avoid enabling Cut/Copy menu commands in text editor context when there's no selection, unless a listener is present. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/988b919adc5c
Update tests for new Cut/Copy command-enabling behavior. r=masayuki

Comment 12

[Atila Butkovits]

Backed out for causing build bustages on EditorBase.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/ba7a0e46f822029f67ba53fc9adc670134656c0b

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=GwrYHXI7Spuzv4G4Km7bLw.0&revision=988b919adc5c7078da290ba4da71771f0b0d685f&searchStr=linux%2Cx64%2Copt%2Cbuild-linux64%2Fopt%2Cb

Failure log: https://treeherder.mozilla.org/logviewer?job_id=332827226&repo=autoland&lineNumber=13667

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b88ab8c6dd39
Avoid enabling Cut/Copy menu commands in text editor context when there's no selection, unless a listener is present. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/dd24e610d956
Update tests for new Cut/Copy command-enabling behavior. r=masayuki

Comment 14

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_contextmenu.js

backout: https://hg.mozilla.org/integration/autoland/rev/40869e2eef251908f942c9a35158e54579f0399d

push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=dd24e610d956db094bf1c0c9975ebbb54fab028a&group_state=expanded&selectedTaskRun=La-vWwTARDmJy649BmXsYQ.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=332888122&repo=autoland&lineNumber=2262

[task 2021-03-11T21:03:40.474Z] 21:03:40 INFO - TEST-PASS | browser/base/content/test/contextMenu/browser_contextmenu.js | checking item #0 (context-copy) name -
[task 2021-03-11T21:03:40.475Z] 21:03:40 INFO - Buffered messages finished
[task 2021-03-11T21:03:40.476Z] 21:03:40 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/contextMenu/browser_contextmenu.js | checking item #0 (context-copy) enabled state - Got false, expected true
[task 2021-03-11T21:03:40.476Z] 21:03:40 INFO - Stack trace:
[task 2021-03-11T21:03:40.476Z] 21:03:40 INFO - chrome://mochikit/content/browser-test.js:test_is:1359
[task 2021-03-11T21:03:40.476Z] 21:03:40 INFO - chrome://mochitests/content/browser/browser/base/content/test/contextMenu/contextmenu_common.js:checkMenuItem:241
[task 2021-03-11T21:03:40.476Z] 21:03:40 INFO - chrome://mochitests/content/browser/browser/base/content/test/contextMenu/contextmenu_common.js:checkMenu:305
[task 2021-03-11T21:03:40.476Z] 21:03:40 INFO - chrome://mochitests/content/browser/browser/base/content/test/contextMenu/contextmenu_common.js:checkContextMenu:175
[task 2021-03-11T21:03:40.476Z] 21:03:40 INFO - chrome://mochitests/content/browser/browser/base/content/test/contextMenu/contextmenu_common.js:test_contextmenu:444
[task 2021-03-11T21:03:40.478Z] 21:03:40 INFO - TEST-PASS | browser/base/content/test/contextMenu/browser_contextmenu.js | checking item #1 (context-selectall) name -

Comment 15

[Natalia Csoregi [:nataliaCs]]

Also failing: https://treeherder.mozilla.org/logviewer?job_id=332898050&repo=autoland&lineNumber=1531

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to Natalia Csoregi [:nataliaCs] from comment #14)

Backed out for failures on browser_contextmenu.js

Huh, this was a bit odd; the test is supposed to select the contents of a text div, so the Copy command would be enabled, but running it with added delays to make it easier to watch, it appears that the <input> element that's used earlier to check clipboard contents holds on to focus, and as none of its content is selected, it disables the Copy command.

Explicitly adding input.blur() after we're finished with that element, so that it relinquishes focus before we continue with opening context menus elsewhere, results in the rest of the tests behaving as expected.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8148b9e5dfba
Avoid enabling Cut/Copy menu commands in text editor context when there's no selection, unless a listener is present. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/0f6717c26ae3
Update tests for new Cut/Copy command-enabling behavior. r=masayuki

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6060fa16bc3e
Fix eslint prettier failures in browser_controller.js a=lint-fix

Comment 19

[Cristian Brindusan [:cbrindusan]]

Backed out 3 changesets (bug 1692673) for bc failures in browser_contextmenu_input.js.
https://hg.mozilla.org/integration/autoland/rev/749a8350e2489721dfdbe9850ea4d17116eae66f

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=0f6717c26ae397929cc55f45a386913273cc49fc&selectedTaskRun=T-PVCFP0Q5uIw58HOiLJyg.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=333003367&repo=autoland&lineNumber=3324

And also, the eslint fix:
https://treeherder.mozilla.org/logviewer?job_id=332998134&repo=autoland&lineNumber=472

Comment 20

[Jonathan Kew [:jfkthame]]

Sigh... another test that assumes the old behavior where the commands are unconditionally enabled in edit fields. Not sure why that didn't show up on try; maybe it's different on Linux. Pushing a try job with more platforms to try and get it right next time....

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/128681c5a081
Avoid enabling Cut/Copy menu commands in text editor context when there's no selection, unless a listener is present. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/06d6db9466df
Update tests for new Cut/Copy command-enabling behavior. r=masayuki

Comment 22

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/128681c5a081
https://hg.mozilla.org/mozilla-central/rev/06d6db9466df

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jfkthame, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 24

[Jonathan Kew [:jfkthame]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #23)

The patch landed in nightly and beta is affected.
:jfkthame, is this bug important enough to require an uplift?

I don't think so; this is a longstanding issue, and the fix is basically a cosmetic improvement to get the state of the menuitems to better reflect whether they're relevant.

Comment 26

[Emil Ghitta, QA [:emilghitta]]

This issue is verified fixed using Firefox 89.0a1 (BuildId:20210322174641) and Firefox 88.0b1 (BuildId:20210322185611) on Windows 10 64bit, macOS 10.14 and Ubuntu 20.04.