1170531 - Clipboard menu commands not disabled properly

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Neil Deakin]

Steps:

1. Focus a tab by tab navigating to it.
2. Open the edit menu

Actual:
 The cut and copy commands are enabled.

Expected:
 The cut and copy commands are not enabled.

I assume that this is a regression from bug 1159490 or 1012662.

In fact, it seems that the cut/copy commands never get disabled.

Comment 1

[Neil Deakin]

[Tracking Requested - why for this release]: the cut/copy edit menu commands don't ever update their disabled state.

I verified that 1159490 causes this bug, but the fix for that bug should only apply to web pages, and not to xul documents/chrome windows. The fix is to back out the patch in bug 1159490 and instead modify the controllers as I described in comment 9 of that bug ( https://bugzilla.mozilla.org/show_bug.cgi?id=1159490#c9 )

Comment 2

[(no longer active)]

OK, fair enough.

Michael, can you try that please?  Let me know if you need help.  Thanks!

Comment 3

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

Attachment: Disable clipboard menu commands correctly in non-(X)HTML documents

I haven't pushed this to try yet, but my quick manual tests suggest that the change works.

Comment 4

[(no longer active)]

Comment on attachment 8616877 [details] [diff] [review]
Disable clipboard menu commands correctly in non-(X)HTML documents

Review of attachment 8616877 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but don't you also need to revert the change to globalOverlay.js?  Without that, I don't think this patch actually fixes what Neil Deakin was complaining about...

Comment 5

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

Attachment: Disable clipboard menu commands correctly in non-(X)HTML documents

Added a test for the new behaviour, and re-added the command enabled check in globalOverlay.js

Any chance you could look at this, and make sure that I'm fixing the problem which you were running into?

Comment 6

[Kate Glazko]

Tracking because 41 is affected.

Comment 7

[Neil Deakin]

Comment on attachment 8616936 [details] [diff] [review]
Disable clipboard menu commands correctly in non-(X)HTML documents

OK, this works better! Thanks!

>+    // Cut isn't enabled in xul documents which use nsClipboardCommand
>+    if (strcmp(aCommandName, "cmd_cut")) {
>+      *outCmdEnabled = nsCopySupport::CanCopy(doc);

This is ok as a change, but it wasn't there before. Can you explain why?

>+function test() {
>+  waitForExplicitFinish();
>+  let tab = gBrowser.selectedTab = gBrowser.addTab("about:blank");
>+  let browser = gBrowser.getBrowserForTab(tab);
>+
>+  let menu_EditPopup = document.getElementById("menu_EditPopup");
>+
>+  browser.addEventListener("load", test_html_noeditable, true);
>+  browser.loadURI("data:text/html,<div>hello!</div>", null, null);
>+
>+  function test_html_noeditable() {

You might consider using BrowserTestUtils.browserLoaded and/or BrowserTestUtils.openNewForegroundTab and write the test as a task instead of using load events directly. This tend to be more reliable on e10s for example.



>+    browser.removeEventListener("load", test_html_noeditable, true);
>+
>+    browser.focus();
>+    goUpdateGlobalEditMenuItems();
>+    let menu_cut_disabled = menu_EditPopup.querySelector("#menu_cut").getAttribute('disabled') == "true";

I don't think we want to be calling goUpdateGlobalEditMenuItems directly in the test. If you checked the context menu though you could just wait for it to open and verify the menu commands (see browser_clipboard.js for an example of this)

Comment 8

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

(In reply to Neil Deakin from comment #7)

> >+    // Cut isn't enabled in xul documents which use nsClipboardCommand
> >+    if (strcmp(aCommandName, "cmd_cut")) {
> >+      *outCmdEnabled = nsCopySupport::CanCopy(doc);
> 
> This is ok as a change, but it wasn't there before. Can you explain why?

nsClipboardCommand is the command handler which is used by documents which don't have an attached editor component. In the old code, IsCommandEnabled always returned false for if the argument was "cmd_cut". With this new patch, HTML or XHTML documents need to return true for "cmd_cut", but xul documents which use this command controller must return the same as they have in the past. Thus, the extra strcmp(aCommandName, "cmd_cut") is required in order to ensure that the old behaviour is still preserved for xul documents.

> >+function test() {
> >+  waitForExplicitFinish();
> >+  let tab = gBrowser.selectedTab = gBrowser.addTab("about:blank");
> >+  let browser = gBrowser.getBrowserForTab(tab);
> >+
> >+  let menu_EditPopup = document.getElementById("menu_EditPopup");
> >+
> >+  browser.addEventListener("load", test_html_noeditable, true);
> >+  browser.loadURI("data:text/html,<div>hello!</div>", null, null);
> >+
> >+  function test_html_noeditable() {
> 
> You might consider using BrowserTestUtils.browserLoaded and/or
> BrowserTestUtils.openNewForegroundTab and write the test as a task instead
> of using load events directly. This tend to be more reliable on e10s for
> example.

Awesome, I didn't know about those utilities, (I copied most of this infra off of another test). I'll look into rewriting the test to use them.

> >+    browser.removeEventListener("load", test_html_noeditable, true);
> >+
> >+    browser.focus();
> >+    goUpdateGlobalEditMenuItems();
> >+    let menu_cut_disabled = menu_EditPopup.querySelector("#menu_cut").getAttribute('disabled') == "true";
> 
> I don't think we want to be calling goUpdateGlobalEditMenuItems directly in
> the test. If you checked the context menu though you could just wait for it
> to open and verify the menu commands (see browser_clipboard.js for an
> example of this)

Does the context menu go through the same update code path as the file menu and the hamburger cut/copy/paste options? I had the impression that they had a different update method, which wasn't changed in nightly (context menus still appear to have correct values in xul for nightly).

Comment 9

[Neil Deakin]

> nsClipboardCommand is the command handler which is used by documents which
> don't have an attached editor component. In the old code, IsCommandEnabled
> always returned false for if the argument was "cmd_cut". With this new
> patch, HTML or XHTML documents need to return true for "cmd_cut", but xul
> documents which use this command controller must return the same as they
> have in the past. Thus, the extra strcmp(aCommandName, "cmd_cut") is
> required in order to ensure that the old behaviour is still preserved for
> xul documents.

Ah, yes, of course. Thanks!


> Does the context menu go through the same update code path as the file menu
> and the hamburger cut/copy/paste options? I had the impression that they had
> a different update method, which wasn't changed in nightly (context menus
> still appear to have correct values in xul for nightly).

Hmm, that's unfortunate. OK, leave as is then, although you could also try the keyboard usage.

Comment 10

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

Attachment: Disable clipboard menu commands correctly in non-(X)HTML documents

Updated version of tests which uses add_task

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3fca18104696

Comment 12

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11016433&repo=mozilla-inbound

Comment 13

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b3a94511813

Comment 14

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #12)
> sorry had to back this out for test failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=11016433&repo=mozilla-
> inbound

also other test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11017101&repo=mozilla-inbound - so that i would consider a try run before landing again

Comment 15

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

Attachment: Disable clipboard menu commands correctly in non-(X)HTML documents

I think I fixed the tests you pointed out, thanks.
Here's a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=daffe1b9884b

Comment 16

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

Attachment: Disable clipboard menu commands correctly in non-(X)HTML documents

I think I've fixed the test failures. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5258a061621e

Comment 17

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

Attachment: Disable clipboard menu commands correctly in non-(X)HTML documents

I think that this will actually pass this time >.>
I had to rewrite the test to get it to work correctly on Linux/Windows, so I'm asking for review again.

Comment 18

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

Attachment: Disable clipboard menu commands correctly in non-(X)HTML documents

It should work now (I think). I had to rewrite the tests to work on linux. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbce2e9b38c5

I'm asking for review again because of new test rewrite

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d648becaa638

Comment 20

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fdfbe35371a

Comment 21

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

Attachment: Disable clipboard menu commands correctly in non-(X)HTML documents

I waited for the try to pass this time ^.^
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2420d56e97eb

I restructured the changes to nsPlaintextEditor.cpp a bit to fix a bug, and am not sure about the style. It should be ready to check in once the style is looked over.

Comment 22

[(no longer active)]

Neil, should we uplift the fix for this bug to 41?

Comment 23

[Neil Deakin]

I think we should, assuming it is finally stable.

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/924374177485

Comment 25

[(no longer active)]

Comment on attachment 8627307 [details] [diff] [review]
Disable clipboard menu commands correctly in non-(X)HTML documents

Approval Request Comment
[Feature/regressing bug #]: Bug 1159490
[User impact if declined]: See comment 1.  The cut and copy commands will be enabled in XUL documents (for example, about:preferences) without the fix to this bug.
[Describe test coverage new/current, TreeHerder]: Has automated tests.
[Risks and why]: This is not the smallest and safest patch on the planet, but it's mostly well understood and for the most part if brings us back to the behavior we had before bug 1159490.  Ideally this would have landed before the uplift but it missed it by a day.  I'd prefer to take this on Aurora as soon as we can so that we will have enough time to deal with regressions, if any.
[String/UUID change made/needed]: None.

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/924374177485

Comment 27

[Kate Glazko]

Comment on attachment 8627307 [details] [diff] [review]
Disable clipboard menu commands correctly in non-(X)HTML documents

Approving uplift to Aurora because this has been on m-c for a week without issues so far, and a good idea to land early in the cycle.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8b41d82bd38

Comment 29

[Vasilica Tamas, QA [:vtamas]]

This issue is still reproducible on Firefox 42.0a1 (2015-07-30) and Firefox 41.0a2 (2015-07-30) under Windows 7 64-bit, Ubuntu 13.10 64-bit and Mac OS X 10.10.4.

See screenshot: http://i.imgur.com/6Bc2xWK.jpg

Based on this testing I am reopening this issue.

Comment 30

[Neil Deakin]

Please don't reopen fixed bugs unless the original reported issue (comment 0) is not fixed. In this case, I think you are seeing bug 1178047.

Comment 31

[Vasilica Tamas, QA [:vtamas]]

Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1178047#c8 and https://bugzilla.mozilla.org/show_bug.cgi?id=1178047#c9 I understand that the intended behaviour of this bug is to disable the cut/copy commands for the following pages:

urlbar in nav-bar
searchbar in nav-bar
searchbar in Bookmarks sidebar
about:addons
about:config
about:newtab
about:preferences#general Home Page
about:preferences#search Keyword
about:preferences#content Exceptions -> Address of website
about:preferences#applications Searchbar
about:preferences#privacy Exceptions -> Address of website
about:preferences#privacy Show Cookies -> Searchbar
about:preferences#security Exceptions -> Address of website
about:preferences#advanced Network -> Connection Settings
about:permissions
about:sync-tabs
Developer Tools: searchbar in Inspector
Developer Tools: Web Console
Developer Tools: searchbar in Debugger
Developer Tools: searchbar in Network Monitor
Bookmark Library
History Library

Am I right?
Because I’ve verified all these pages on Firefox 41 Beta 6 (20150831172306) and Firefox 42.0a2 (2015-09-02) on Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.4 and I have noticed an issue:
 
Cut/Copy commands are not disabled for about:newtab page on Firefox 42.0a2 across all platforms. This issue is not reproducible on Firefox 41 Beta 6.

Should I file a new bug for this issue?

Comment 32

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

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #31)
> Based on https://bugzilla.mozilla.org/show_bug.cgi?id=1178047#c8 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=1178047#c9 I understand that
> the intended behaviour of this bug is to disable the cut/copy commands for
> the following pages:
> 
> urlbar in nav-bar
> searchbar in nav-bar
> searchbar in Bookmarks sidebar
> about:addons
> about:config
> about:newtab
> about:preferences#general Home Page
> about:preferences#search Keyword
> about:preferences#content Exceptions -> Address of website
> about:preferences#applications Searchbar
> about:preferences#privacy Exceptions -> Address of website
> about:preferences#privacy Show Cookies -> Searchbar
> about:preferences#security Exceptions -> Address of website
> about:preferences#advanced Network -> Connection Settings
> about:permissions
> about:sync-tabs
> Developer Tools: searchbar in Inspector
> Developer Tools: Web Console
> Developer Tools: searchbar in Debugger
> Developer Tools: searchbar in Network Monitor
> Bookmark Library
> History Library
> 
> Am I right?
> Because I’ve verified all these pages on Firefox 41 Beta 6 (20150831172306)
> and Firefox 42.0a2 (2015-09-02) on Windows 7 64-bit, Ubuntu 14.04 32-bit and
> Mac OS X 10.10.4 and I have noticed an issue:
>  
> Cut/Copy commands are not disabled for about:newtab page on Firefox 42.0a2
> across all platforms. This issue is not reproducible on Firefox 41 Beta 6.
> 
> Should I file a new bug for this issue?

I believe that in Firefox 42, about:newtab was moved from xul to html (bug 1167601), meaning that the clipboard commands would no longer be disabled. I don't think that this is unexpected.

Comment 33

[Vasilica Tamas, QA [:vtamas]]

Based on Comment 32 and on my previous testing from Comment 31, I am marking this bug as Verified.