1529884 - All public methods of editor which may be called JS API should have argument of caller type

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P2)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

beforeinput event should not be fired if the edit action is caused by web apps, e.g., document.execCommand() (is there any other situation?).

So, public methods which may be called by nsHTMLDocument::ExecCommand() should have the caller type to distinguish whether we need to dispatch beforeinput or not.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm still struggling with this. Currently, we use this path:

1. nsHTMLDocument::ExecCommand()
2. nsCommandManager::DoCommand()
3. nsIController::DoCommand() or nsICommandController::DoCommandWithParams()
4. nsIControllerCommandTable::DoCommand() or nsIControllerCommandTable::DoCommandWithParams()
5. nsControllerCommandTable::DoCommand() or nsControllerCommandTable::DoCommandParams()
6. nsIControllerCommand::DoCommand() or nsIControllerCommand::DoCommandParams()

I've already some those interfaces builtin classes. However, nsIController, nsICommandController and nsIControllerCommand are implemented by JS in comm-central. If we use implicit_context for their methods, the latest context is passed. So, even if execCommand() is called by content script but one of them is implemented by JS, the context becomes chrome context. Note that this does not occur with current Firefox, and JS isn't enabled by default on Tb and some other Gecko embedded applications. Therefore, we can use this approach at least.

However, I'm looking for better approach. The simplest idea is, each edit command class check current context. This approach has same concern about passing context in all interfaces, but the implementation becomes simpler.

Currently, my better approach is, execCommand() access edit command class or editor directly. Then, we can improve the performance and it may make benchmark score better since benchmarks use execCommand() a lot, IIRC.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, unfortunately, the patches become too big and they are really difficult to review for anybody. So, I'll file separate bugs for editor part changes to make reviews easier. However, unfortunately, Makoto-san will be away for 10 days due to Japanese national holidays but I need to keep working to fix these bugs. So, perhaps, I'll request reviews even for editor part to smaug when he's away.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1529884 - part 0: Add new WPT to check whether `document.execCommand()` without editable element and with editable parent or child document

Currently, there are no tests of execCommand() without editable element.
Additionally, current Gecko propagate "cut" and "copy" commands into child
document (I think it's odd).

This test checks the behavior. The expected results are considered mainly
from Chrome, but also Chrome does not pass all tests because some their
behavior are odd. E.g., Chrome returns true for execCommand("styleWithCSS")
even though it does nothing. So, this patch makes its expected result false.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1529884 - part 1: Make Document use hashtable to map HTML command and internal command data

Currently, nsHTMLDocument converts HTML command (e.g., used by execCommand()
to internal XUL command with array in the global space. However, it requires
scan of the array for every command access.

This patch makes nsHTMLDocument use hashtable to make the conversion faster.

New mapping info comes from:

• mXULCommandName is same as internalCommandString
• mCommand is mapped in CommandList.h from mXULCommandName
• mGetEditorCommandFunc is mapped from mXULCommandName in:
  • https://searchfox.org/mozilla-central/rev/d143f8ce30d1bcfee7a1227c27bf876a85f8cede/editor/libeditor/EditorController.cpp#31-32,34-38,40-41,43,45-51,54-57,67-112
  • https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/editor/libeditor/HTMLEditorController.cpp#26-28,31-39,48,51-52,55-58,60-63,65-73,76-80,83-88,90-91,93-94,97-100,102-104
• mHTMLParam and is converted from useNewParam and convertToBoolean:
  • If corresponding editor command class's DoCommandParam() just calls
    DoCommand(), HTMLParam::Ignore.
  • If useNewParam is true and convertToBoolean is false, given value
    should be ignored and may set constant instead. In this case,
    HTMLParam::Ignore.
  • If useNewParam is false and convertToBoolean is false, given value
    should be treated as string. In this case, HTMLParam::RespectAsString.
  • If useNewParam is false and convertToBoolean is true, given value
    should be treated as bool. In this case, if given command is not a legacy
    one, HTMLParam::RespectAsBool. Otherwise, i.e., if given command is a
    legacy one, HTMLParam::RespectAsInvertedBool.
  • Otherwise, HTMLParam::RespectAsString.
• mCommandParmType is:
  • CommandParamType::None if mHTMLParam is HTMLParam::Ignore and param
    is always ignored.
  • CommandParamType::BoolInStateAttribute if mHTMLParam is
    HTMLParam::RespectAsBool or HTMLParam::RespectAsInvertedBool.
  • CommandParamType::StringInStateAttribute or
    CommandParamType::StringInDataAttribute if command is one of:
    https://searchfox.org/mozilla-central/rev/75294521381b331f821aad3d6b60636844080ee2/dom/html/nsHTMLDocument.cpp#2426-2432
  • CommandParamType::CStringInStateAttribute, otherwise.

Note that CommandParamType will be removed by part 6 because ExecCommand() starts to treat EditorCommand directly from it.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1529884 - part 2: Implement ConvertToMidasInternalCommand() as returning InternalCommandData

This patch creates ConvertToInternalCommand() as the replacement of
ConvertToMidasInternalCommand() and ConvertToMidasInternalCommandInner().
It returns InternalCommandData. Therefore, every caller can compare
Command instead of using strcmp().

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1529884 - part 3: Make Document::ConvertToInternalCommand() check the input value when the caller requires adjusted value

aAdjustedValue of nsHTMLDocument::ConvertToInternalCommand() is not
nullptr when it's called by ExecCommand() or QueryCommandState().
However, QueryCommandState() does not need the value actually. Therefore,
we can move the input value check from ExecCommand() to ConvertToInsernalCommand()`.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1529884 - part 4: Make Document::ExecCommand() do security check first if given command is supported

This minimize the next patch.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1529884 - part 5: Make Document::ExecCommand() use EditorCommand directly as far as possible

Most commands are dispatched only when the document has contenteditable or
in designMode. In such case, command target is considered with the following
order:

1. HTMLEditor for the document.
2. TextEditor if focused element has one.
3. Other command controller table associated with window or DocShell.

In the case of #1 and #2, ExecCommand() can use EditorCommand directly.

In case of #3, there are two situations. One is when command is the clipboard
write command, i.e., "paste", it's targeted to the window. The other is, when
command is a clipboard read command, i.e., "cur" or "copy", it's targeted to
its window or focused subdocument via DocShell.

But note that clipboard write commands are special cases. They have always been
handled with active element since they are always handled with DocShell.
Therefore, the priority between #1 and #2 is opposite only at handling them.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1529884 - part 6: Split EditorCommand::DoCommandParams() to avoid creating unnecessary nsCommandParams instance

This patch splits EditorCommand::DoCommandParams() to DoCommandParam()
which take nothing, const Maybe<bool>&, const nsAString&,
const nsACString& or nsITransferable instead of nsCommandParam*.

This means that EditorCommand needs to manage mapping of each Command
value with a type of param and where it is stored in nsCommandParams.
Therefore, nsHTMLDocument does not need to manage it anymore.

Note that the following URLs are current param getters in DoCommandParams():

• Command::PasteTransferable handled by PasteTransferableCommand
• Command::InsertText handled by InsertPlaintextCommand
• Command::SetDocumentModified, Command::SetDocumentUseCSS, Command::SetDocumentReadOnly, Command::SetDocumentInsertBROnEnterKeyPress, Command::ToggleObjectResizers, Command::ToggleInlineTableEditor and Command::ToggleAbsolutePositionEditor handled by SetDocumentStateCommand
• Command::SetDocumentDefaultParagraphSeparator handled by SetDocumentStateCommand
• Command::FormatBlock, Command::FormatFontName, Command::FormatFontSize, Command::FormatFontColor, Command::FormatDocumentBackgroundColor, Command::FormatBackColor, Command::FormatJustifyLeft, Command::FormatJustifyRight, Command::FormatJustifyCenter and Command::FormatJustifyFull handled by MultiStateCommandBase subclasses
• Command::InsertHTML handled by InsertHTMLCommand
• Command::InsertLink and Command::InsertImage handled by InsertTagCommand

Note that this changes CommandInt from int8_t to uint8_t because now, number of Command is over 128.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1529884 - part 6: Through subject principal at Document::ExecCommand() to constructor of EditorBase::AutoEditActionDataSetter

nsHTMLDocument::ExecCommand() knows subject principal. This patch makes
it tell EditorCommand::DoCommand() and EditorCommand::DoCommandParam().
Then, makes they tell each editor public methods which may cause dispatching
beforeinput event once we implement it. Finally, each editor public
method sets it to the constructor of EditorBase::AutoEditActionDataSetter.
This means that when editor tries to dispatch beforeinput event, editor
can check whether it's called by JS or not from everywhere once
EditorBase::AutoEditActionDataSetter starts to store it.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

These are complicated patches, will take several days to review.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #11)

These are complicated patches, will take several days to review.

Sure. No problem. And please give higher priority to the other patches which need to be landed into 68.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1529884 - part 7: Make `ExecCommand()` and related methods use `a` prefix for every parameter

(I'd like to clean up the related methods with direct access to EditorCommand in another bug.)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66cd074a9182
part 0: Add new WPT to check whether `document.execCommand()` without editable element and with editable parent or child document r=smaug
https://hg.mozilla.org/integration/autoland/rev/dd9794f7d400
part 1: Make Document use hashtable to map HTML command and internal command data r=smaug
https://hg.mozilla.org/integration/autoland/rev/f44697e42459
part 2: Implement ConvertToMidasInternalCommand() as returning InternalCommandData r=smaug
https://hg.mozilla.org/integration/autoland/rev/460df951943a
part 3: Make Document::ConvertToInternalCommand() check the input value when the caller requires adjusted value r=smaug
https://hg.mozilla.org/integration/autoland/rev/7af1b30df51c
part 4: Make Document::ExecCommand() do security check first if given command is supported r=smaug
https://hg.mozilla.org/integration/autoland/rev/258c4da663ca
part 5: Make Document::ExecCommand() use EditorCommand directly as far as possible r=smaug
https://hg.mozilla.org/integration/autoland/rev/9ee7acab98a4
part 6: Through subject principal at Document::ExecCommand() to constructor of EditorBase::AutoEditActionDataSetter r=smaug
https://hg.mozilla.org/integration/autoland/rev/87b36ec23f8a
part 7: Make `ExecCommand()` and related methods use `a` prefix for every parameter r=smaug

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17251 for changes under testing/web-platform/tests

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/66cd074a9182
https://hg.mozilla.org/mozilla-central/rev/dd9794f7d400
https://hg.mozilla.org/mozilla-central/rev/f44697e42459
https://hg.mozilla.org/mozilla-central/rev/460df951943a
https://hg.mozilla.org/mozilla-central/rev/7af1b30df51c
https://hg.mozilla.org/mozilla-central/rev/258c4da663ca
https://hg.mozilla.org/mozilla-central/rev/9ee7acab98a4
https://hg.mozilla.org/mozilla-central/rev/87b36ec23f8a

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged