Closed Bug 676401 Opened 10 years ago Closed 10 years ago

The queryCommandEnabled API doesn't take the active editing host into account

Categories

(Core :: DOM: Editor, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: ehsan.akhgari, Assigned: kaze)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 3 obsolete files)

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?
Assignee: nobody → kaze
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.
Status: NEW → ASSIGNED
Attached file 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?
Attached patch patch proposal (obsolete) — Splinter Review
Attachment #552877 - Flags: review?(ehsan)
OS: Mac OS X → All
Attached patch patch proposal (obsolete) — Splinter Review
forgot the `SimpleTest.finish()'
Attachment #552877 - Attachment is obsolete: true
Attachment #552877 - Flags: review?(ehsan)
Attachment #552911 - Flags: review?(ehsan)
(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 on attachment 552911 [details] [diff] [review]
patch proposal

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

I believe this is the wrong patch!
Attachment #552911 - Flags: review?(ehsan)
Keywords: dev-doc-needed
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.
Attachment #552877 - Flags: review+
(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.
Attached patch patch proposal (obsolete) — Splinter Review
updated and rebased patch
Attachment #552911 - Attachment is obsolete: true
Attachment #553191 - Flags: review?(ehsan)
Comment on attachment 553191 [details] [diff] [review]
patch proposal

Thanks!
Attachment #553191 - Flags: review?(ehsan) → review+
Has this passed try?  Does it need landing?
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.
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.
Attached patch patch proposalSplinter Review
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…
Attachment #553191 - Attachment is obsolete: true
Attachment #553429 - Flags: review?(ehsan)
Blocks: 345845
Comment on attachment 553429 [details] [diff] [review]
patch proposal

Looks great, thanks!
Attachment #553429 - Flags: review?(ehsan) → review+
Pushed to inbound.
http://hg.mozilla.org/mozilla-central/rev/63f07bd1d64f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
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?
Blocks: 680060
Depends on: 692153
Documentation updated:

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

Also mentioned on Firefox 9 for developers.
"copy" should actually be always enabled. (It isn't because of bug 692153.)
You need to log in before you can comment on or make changes to this bug.