The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: kaze)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla9
x86
All
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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?
(Reporter)

Updated

6 years ago
Assignee: nobody → kaze
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
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?
(Assignee)

Comment 3

6 years ago
Created attachment 552877 [details] [diff] [review]
patch proposal
Attachment #552877 - Flags: review?(ehsan)
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
(Assignee)

Comment 4

6 years ago
Created attachment 552911 [details] [diff] [review]
patch proposal

forgot the `SimpleTest.finish()'
Attachment #552877 - Attachment is obsolete: true
Attachment #552877 - Flags: review?(ehsan)
Attachment #552911 - Flags: review?(ehsan)
(Reporter)

Comment 5

6 years ago
(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.
(Reporter)

Comment 6

6 years ago
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)
(Reporter)

Updated

6 years ago
Keywords: dev-doc-needed
(Reporter)

Comment 7

6 years ago
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+
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
Created attachment 553191 [details] [diff] [review]
patch proposal

updated and rebased patch
Attachment #552911 - Attachment is obsolete: true
Attachment #553191 - Flags: review?(ehsan)
(Reporter)

Comment 10

6 years ago
Comment on attachment 553191 [details] [diff] [review]
patch proposal

Thanks!
Attachment #553191 - Flags: review?(ehsan) → review+
(Reporter)

Comment 11

6 years ago
Has this passed try?  Does it need landing?
(Assignee)

Comment 12

6 years ago
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.
(Reporter)

Comment 13

6 years ago
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.
(Assignee)

Comment 14

6 years ago
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…
Attachment #553191 - Attachment is obsolete: true
Attachment #553429 - Flags: review?(ehsan)
(Assignee)

Comment 15

6 years ago
TryServer results look fine: http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=d3b94afcef05
(Assignee)

Updated

6 years ago
Blocks: 345845
(Reporter)

Comment 16

6 years ago
Comment on attachment 553429 [details] [diff] [review]
patch proposal

Looks great, thanks!
Attachment #553429 - Flags: review?(ehsan) → review+
(Reporter)

Comment 17

6 years ago
Pushed to inbound.
http://hg.mozilla.org/mozilla-central/rev/63f07bd1d64f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 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?
(Assignee)

Updated

6 years ago
Blocks: 680060

Updated

6 years ago
Depends on: 692153
Documentation updated:

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

Also mentioned on Firefox 9 for developers.
Keywords: dev-doc-needed → dev-doc-complete
"copy" should actually be always enabled. (It isn't because of bug 692153.)
Blocks: 760052
You need to log in before you can comment on or make changes to this bug.