Closed Bug 1170531 Opened 4 years ago Closed 4 years ago

Clipboard menu commands not disabled properly

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox41 + verified
firefox42 --- verified

People

(Reporter: enndeakin, Assigned: Nika)

References

Details

(Keywords: regression)

Attachments

(1 file, 7 obsolete files)

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.
Component: DOM: Events → Menus
Product: Core → Firefox
[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 )
Flags: needinfo?(ehsan)
OK, fair enough.

Michael, can you try that please?  Let me know if you need help.  Thanks!
Assignee: nobody → michael
Flags: needinfo?(ehsan) → needinfo?(michael)
I haven't pushed this to try yet, but my quick manual tests suggest that the change works.
Flags: needinfo?(michael)
Attachment #8616877 - Flags: review?(ehsan)
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...
Attachment #8616877 - Flags: review?(ehsan) → review-
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?
Attachment #8616877 - Attachment is obsolete: true
Attachment #8616936 - Flags: feedback?(enndeakin)
Tracking because 41 is affected.
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)
Attachment #8616936 - Flags: feedback?(enndeakin) → feedback+
(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).
Flags: needinfo?(enndeakin)
> 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.
Flags: needinfo?(enndeakin)
Updated version of tests which uses add_task
Attachment #8616936 - Attachment is obsolete: true
Attachment #8624760 - Flags: review?(ehsan)
Attachment #8624760 - Flags: review?(ehsan) → review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=11016433&repo=mozilla-inbound
Flags: needinfo?(michael)
(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
I think I fixed the tests you pointed out, thanks.
Here's a try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=daffe1b9884b
Attachment #8624760 - Attachment is obsolete: true
Flags: needinfo?(michael)
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.
Attachment #8626628 - Attachment is obsolete: true
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
Attachment #8626839 - Attachment is obsolete: true
Attachment #8626840 - Flags: review?(ehsan)
Attachment #8626840 - Flags: review?(ehsan) → review+
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.
Attachment #8626840 - Attachment is obsolete: true
Attachment #8627307 - Flags: feedback?(ehsan)
Attachment #8627307 - Flags: feedback?(ehsan) → review+
Neil, should we uplift the fix for this bug to 41?
Flags: needinfo?(enndeakin)
I think we should, assuming it is finally stable.
Flags: needinfo?(enndeakin)
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.
Attachment #8627307 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/924374177485
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
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.
Attachment #8627307 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
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?
Flags: needinfo?(enndeakin)
(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.
Based on Comment 32 and on my previous testing from Comment 31, I am marking this bug as Verified.
Status: RESOLVED → VERIFIED
Flags: needinfo?(enndeakin)
You need to log in before you can comment on or make changes to this bug.