Last Comment Bug 676401 - The queryCommandEnabled API doesn't take the active editing host into account
: The queryCommandEnabled API doesn't take the active editing host into account
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla9
Assigned To: Fabien Cazenave [:kaze]
:
Mentors:
data:text/html,<div>not editable</div...
Depends on: 692153
Blocks: 345845 680060 760052
  Show dependency treegraph
 
Reported: 2011-08-03 15:33 PDT by :Ehsan Akhgari (away Aug 1-5)
Modified: 2012-06-20 08:31 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (3.50 KB, text/html)
2011-08-13 07:25 PDT, Fabien Cazenave [:kaze]
no flags Details
patch proposal (31.25 KB, patch)
2011-08-13 09:34 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review
patch proposal (7.37 KB, patch)
2011-08-13 16:16 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (32.06 KB, patch)
2011-08-15 09:07 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review
patch proposal (34.12 KB, patch)
2011-08-16 03:56 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari (away Aug 1-5) 2011-08-03 15:33:21 PDT
See the testcase in the URL field.

The reason why this happens is that for most commands, we just look to see if there is an editor available, in which case we go ahead and return true.  This used to be correct before Firefox 3 when we only supported designMode, but in the face of contentEditable, we should pay attention to the active editing host.  For example, the testcase should alert false, because the selection doesn't fall in the editable section.

A good way to fix this is to look at the implementations of IsCommandEnabled in editor/ and make each one respect the active editing host.  For most commands, we should check to see if the selection's anchor node and check to see if it falls in the active editing host.

Fabien, can you look into this, please?
Comment 1 Fabien Cazenave [:kaze] 2011-08-13 06:20:27 PDT
Working on it. I’ve added an `isSelectionEditable' read-only attribute to <nsIEditor> and I’m looking for a way to test all execCommands…

Side note (bug 456798): `queryCommandSupported' is not implemented and `queryCommandEnabled' raises an exception on non-supported commands.
Comment 2 Fabien Cazenave [:kaze] 2011-08-13 07:25:18 PDT
Created attachment 552870 [details]
testcase

This testcase checks the result of `queryCommandEnabled' on the usual execCommands.

It doesn’t test cut/copy/paste yet because enhanced privileges are required. That will be done in the unit test.

There are a few `ns*Command' that can’t be tested that way:

./libeditor/base/nsEditorCommands.cpp
  nsCutOrDeleteCommand
  nsCopyOrDeleteCommand
  nsPasteTransferableCommand
  nsSwitchTextDirectionCommand
  nsSelectionMoveCommands
  nsInsertPlaintextCommand
  nsPasteQuotationCommand

./composer/src/nsComposerCommands.cpp
  nsBaseStateUpdatingCommand
  nsPasteNoFormattingCommand
  nsRemoveListCommand
  nsMultiStateCommand
  nsAbsolutePositioningCommand
  nsDecreaseZIndexCommand
  nsIncreaseZIndexCommand
  nsInsertTagCommand

./composer/src/nsComposerDocumentCommands.cpp
  nsSetDocumentOptionsCommand
  nsSetDocumentStateCommand

Ehsan, do you have a suggestion to test these commands?
Comment 3 Fabien Cazenave [:kaze] 2011-08-13 09:34:42 PDT
Created attachment 552877 [details] [diff] [review]
patch proposal
Comment 4 Fabien Cazenave [:kaze] 2011-08-13 16:16:07 PDT
Created attachment 552911 [details] [diff] [review]
patch proposal

forgot the `SimpleTest.finish()'
Comment 5 :Ehsan Akhgari (away Aug 1-5) 2011-08-15 08:19:56 PDT
(In reply to Fabien Cazenave (:kazé) from comment #2)
> Created attachment 552870 [details]
> testcase
> 
> This testcase checks the result of `queryCommandEnabled' on the usual
> execCommands.
> 
> It doesn’t test cut/copy/paste yet because enhanced privileges are required.
> That will be done in the unit test.
> 
> There are a few `ns*Command' that can’t be tested that way:
> 
> ./libeditor/base/nsEditorCommands.cpp
>   nsCutOrDeleteCommand
>   nsCopyOrDeleteCommand
>   nsPasteTransferableCommand
>   nsSwitchTextDirectionCommand
>   nsSelectionMoveCommands
>   nsInsertPlaintextCommand
>   nsPasteQuotationCommand
> 
> ./composer/src/nsComposerCommands.cpp
>   nsBaseStateUpdatingCommand
>   nsPasteNoFormattingCommand
>   nsRemoveListCommand
>   nsMultiStateCommand
>   nsAbsolutePositioningCommand
>   nsDecreaseZIndexCommand
>   nsIncreaseZIndexCommand
>   nsInsertTagCommand
> 
> ./composer/src/nsComposerDocumentCommands.cpp
>   nsSetDocumentOptionsCommand
>   nsSetDocumentStateCommand
> 
> Ehsan, do you have a suggestion to test these commands?

Those commands are not exposed through execCommand IINM.  To test them, you can grab the editor's command manager manually in a chrome test and fire these commands.
Comment 6 :Ehsan Akhgari (away Aug 1-5) 2011-08-15 08:21:13 PDT
Comment on attachment 552911 [details] [diff] [review]
patch proposal

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

I believe this is the wrong patch!
Comment 7 :Ehsan Akhgari (away Aug 1-5) 2011-08-15 08:44:25 PDT
Comment on attachment 552877 [details] [diff] [review]
patch proposal

I'm reviewing this version of the patch, instead.

>+  // common execCommands

Nit: common commands

>+  SpecialPowers.setCharPref("capability.policy.policynames",                      "allowclipboard");
>+  SpecialPowers.setCharPref("capability.policy.allowclipboard.sites",             "http://mochi.test:8888");
>+  SpecialPowers.setCharPref("capability.policy.allowclipboard.Clipboard.cutcopy", "allAccess");
>+  SpecialPowers.setCharPref("capability.policy.allowclipboard.Clipboard.paste",   "allAccess");
>+  commands = ["cut", "paste", "copy"];
>+  for (i = 0; i < commands.length; i++) {
>+    IsCommandEnabled(commands[i]);
>+    document.execCommand(commands[i], false, false);
>+  }
>+
>+  // delete/undo/redo
>+  commands = ["delete", "undo", "redo"];
>+  for (i = 0; i < commands.length; i++) {
>+    IsCommandEnabled(commands[i]);
>+    document.execCommand(commands[i], false, false);
>+  }

Why do you need to actually run these commands here?

Also, you should revert your changes to the prefs using clearUserPref when you're done in this test.

r=me with those.
Comment 8 Fabien Cazenave [:kaze] 2011-08-15 08:58:01 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #6) 
> I believe this is the wrong patch!

Ugh, sorry. You’re right. :-/

(In reply to Ehsan Akhgari [:ehsan] from comment #7)
> >+  SpecialPowers.setCharPref("capability.policy.allowclipboard.Clipboard.paste",   "allAccess");
> >+  commands = ["cut", "paste", "copy"];
> >+  for (i = 0; i < commands.length; i++) {
> >+    IsCommandEnabled(commands[i]);
> >+    document.execCommand(commands[i], false, false);
> >+  }
> >+
> >+  // delete/undo/redo
> >+  commands = ["delete", "undo", "redo"];
> >+  for (i = 0; i < commands.length; i++) {
> >+    IsCommandEnabled(commands[i]);
> >+    document.execCommand(commands[i], false, false);
> >+  }
> 
> Why do you need to actually run these commands here?

We have to execute this commands because:
 * there's nothing to undo if we haven't modified the selection first;
 * there's nothing to redo if we haven't undone something first.

I’m getting a proper patch ready ASAP.
Comment 9 Fabien Cazenave [:kaze] 2011-08-15 09:07:34 PDT
Created attachment 553191 [details] [diff] [review]
patch proposal

updated and rebased patch
Comment 10 :Ehsan Akhgari (away Aug 1-5) 2011-08-15 11:00:39 PDT
Comment on attachment 553191 [details] [diff] [review]
patch proposal

Thanks!
Comment 11 :Ehsan Akhgari (away Aug 1-5) 2011-08-15 11:01:03 PDT
Has this passed try?  Does it need landing?
Comment 12 Fabien Cazenave [:kaze] 2011-08-15 15:52:13 PDT
I'm finding out that this patch creates three regressions on browserscope2 tests: RTE2-QE_PASTE_TEXT-1 (dM|body|div), i.e. the test checking `document.queryCommandEnabled("paste")'.

Sorry, I haven’t seen this regression because my patch queue was a mess. I’ll look at this tomorrow.
Comment 13 :Ehsan Akhgari (away Aug 1-5) 2011-08-15 16:27:26 PDT
Hmm, I may have an idea what's going on with the RTE2-QE_PASTE_TEXT-1 failure.  You should make sure that there is actually something on the clipboard before querying for the paste command.  The clipboard on Linux is asynchronous, which probably causes this test to at least fail on Linux.

To test this hypothesis, try copying something to the clipboard and then running the tests manually.  If that fixes the problem, you can use the waitForClipboard function to make sure that there is something on the clipboard before you run the richtext2 suite.
Comment 14 Fabien Cazenave [:kaze] 2011-08-16 03:56:03 PDT
Created attachment 553429 [details] [diff] [review]
patch proposal

You’re right! Funny to see that my clipboard is empty after several hours of work… Vim magic.

This patch modifies test_richtext2.html to ensure the clipboard isn't empty before running the test suite, as you suggested. It’ll allow to run the browserscope/richtext2 tests alone.

Pushing to Try…
Comment 15 Fabien Cazenave [:kaze] 2011-08-16 11:41:25 PDT
TryServer results look fine: http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=d3b94afcef05
Comment 16 :Ehsan Akhgari (away Aug 1-5) 2011-08-17 10:27:09 PDT
Comment on attachment 553429 [details] [diff] [review]
patch proposal

Looks great, thanks!
Comment 17 :Ehsan Akhgari (away Aug 1-5) 2011-08-17 10:29:12 PDT
Pushed to inbound.
Comment 18 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-18 03:54:15 PDT
http://hg.mozilla.org/mozilla-central/rev/63f07bd1d64f
Comment 19 :Ms2ger (⌚ UTC+1/+2) 2011-08-18 04:20:01 PDT
Comment on attachment 553429 [details] [diff] [review]
patch proposal

>+    PRBool isEditable = PR_FALSE;
>+    NS_SUCCEEDED(editor->GetIsSelectionEditable(&isEditable));

That doesn't make sense. Please file a followup to either remove the NS_SUCCEEDED() or to check its result?
Comment 20 Eric Shepherd [:sheppy] 2011-11-04 10:00:47 PDT
Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/NsIEditor#Attributes

Also mentioned on Firefox 9 for developers.
Comment 21 neil@parkwaycc.co.uk 2011-12-23 03:00:26 PST
"copy" should actually be always enabled. (It isn't because of bug 692153.)

Note You need to log in before you can comment on or make changes to this bug.