Closed
Bug 676401
Opened 12 years ago
Closed 12 years ago
The queryCommandEnabled API doesn't take the active editing host into account
Categories
(Core :: DOM: Editor, defect)
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)
3.50 KB,
text/html
|
Details | |
34.12 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: nobody → kaze
Assignee | ||
Comment 1•12 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•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•12 years ago
|
||
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•12 years ago
|
||
Attachment #552877 -
Flags: review?(ehsan)
Assignee | ||
Updated•12 years ago
|
OS: Mac OS X → All
Assignee | ||
Comment 4•12 years ago
|
||
forgot the `SimpleTest.finish()'
Attachment #552877 -
Attachment is obsolete: true
Attachment #552877 -
Flags: review?(ehsan)
Attachment #552911 -
Flags: review?(ehsan)
Reporter | ||
Comment 5•12 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•12 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•12 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 7•12 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•12 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•12 years ago
|
||
updated and rebased patch
Attachment #552911 -
Attachment is obsolete: true
Attachment #553191 -
Flags: review?(ehsan)
Reporter | ||
Comment 10•12 years ago
|
||
Comment on attachment 553191 [details] [diff] [review] patch proposal Thanks!
Attachment #553191 -
Flags: review?(ehsan) → review+
Reporter | ||
Comment 11•12 years ago
|
||
Has this passed try? Does it need landing?
Assignee | ||
Comment 12•12 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•12 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•12 years ago
|
||
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•12 years ago
|
||
TryServer results look fine: http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=d3b94afcef05
Reporter | ||
Comment 16•12 years ago
|
||
Comment on attachment 553429 [details] [diff] [review] patch proposal Looks great, thanks!
Attachment #553429 -
Flags: review?(ehsan) → review+
Reporter | ||
Comment 17•12 years ago
|
||
Pushed to inbound.
Comment 18•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/63f07bd1d64f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Comment 19•12 years ago
|
||
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•12 years ago
|
||
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
Comment 21•12 years ago
|
||
"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.
Description
•