Merge TextEditRules and HTMLEditRules with TextEditor and HTMLEditor
Categories
(Core :: DOM: Editor, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox71 | --- | fixed |
People
(Reporter: masayuki, Assigned: masayuki)
References
(Blocks 1 open bug)
Details
Attachments
(11 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
I've not considered anything though.
Currently, editor is implemented with 5 big classes. 3 of them are manages most handling of editor and they represent "editor" from the point of view of outside. The classes are:
EditorBase
-> TextEditor
-> HTMLEditor
.
On the other hand, the other 2 classes handles some edit operations, but not all of them. They are:
TextEditRules
-> HTMLEditRules
When TextEditor
or HTMLEditor
instance is initialized, TextEditRules
is created and stored by the former, HTMLEditRules
is created and stored by the latter. And then, TextEditRules
and HTMLEditRules
stores reference of its owner editor instance.
And then, WillDoAction()
of TextEditRules
and HTMLEditRules
is called when one of the editor classes handles something, then, some actions are handled in it. Some other actions are handled by editor classes by themselves after that, and finally, DidDoAction()
of them is called and some other actions are handled at this time.
While TextEditRules
and HTMLEditRules
are dosing something, they create a stack class instance of AudoSafeEditorData
and it guarantees that the life time of the owner editor.
However, now this does not look safe from our source code analysis. I.e., we need to access them with MOZ_KnownLive()
like here. This is really ugly and makes the code harder to read.
Additionally, TextEditRules
and HTMLEditRules
may call editor methods finally like above. So, I have no idea why we need to keep this design. In my understanding, it was designed for making TextEditor
and HTMLEditor
be possible to have different behavior mode. However, although HTMLEditor
has both HTML editing mode and plain text editing mode but the difference is handled in each method. Furthermore, it does not make sense to implement such different editing mode with another EditRules
class because editor module is not cheap to maintain around security issues so that shared code is better for this place.
So, currently, I'm thinking that we should move each editing action in TextEditRules
and HTMLEditRules
to the caller of WillDoAction()
or create new editor method and make editor call it instead. Then, finally, we should remove those classes.
If you guys have some ideas about this issue, let me know.
Assignee | ||
Updated•6 years ago
|
Comment 1•6 years ago
|
||
If HTMLEditor isn't inheritance class TextEditor, EditorBase is necessary. Password support (in TextEditor) is unnecessary for HTMLEditor.
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Makoto Kato [:m_kato] from comment #1)
If HTMLEditor isn't inheritance class TextEditor, EditorBase is necessary. Password support (in TextEditor) is unnecessary for HTMLEditor.
Right. But can we make HTMLEditor
inherit EditorBase
directly? (Just question, I've not investigated it.) If we do it, nsIPlaintextEditor
interface needs to be implemented by EditorBase
at least.
Comment 3•6 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] (JST, +0900)(Sick, maybe slow response) from comment #2)
(In reply to Makoto Kato [:m_kato] from comment #1)
If HTMLEditor isn't inheritance class TextEditor, EditorBase is necessary. Password support (in TextEditor) is unnecessary for HTMLEditor.
Right. But can we make
HTMLEditor
inheritEditorBase
directly? (Just question, I've not investigated it.)
Yes, as object orient design. But a lot of codes such as spellchecker already use TextEditor
directly and plain text only member variable is a few in TextEditor
class. So this isn't strong opinion to keep this class desing.
Before merging this, we should move a lot of node operations in nsIEditor
to nsIHTMLEditor
. It is unnecessary for TextEditor
.
If we do it,
nsIPlaintextEditor
interface needs to be implemented byEditorBase
at least.
nsIPlaintextEditor
is another decision. nsIPlaintextEditor
's methods can move to nsIEditor
and nsIEditorMailSupport
. Of course, I can remove nsIEditorMailSupport
if we merge TextEditor
with EditorBase
.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
Anyway, I think that we should merge them into editor classes then:
EditRules
does not need to manage editor, selection, etc by themselves.- a lot of methods which are accessed from
EditRules
can be private/protected completely. (currently they are accessed as friend classes)
But perhaps, we should keep the files for making developers can access each line history easier. So, in a while, TextEditRules.cpp
and HTMLEditRules.cpp
may implement some methods of EditorBase
, TextEditor
or HTMLEditor
.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
The main purpose of them is modifying
TopLevelEditSubActionData::mChangedRange
. Therefore, they should be in the
struct.
Assignee | ||
Comment 6•5 years ago
|
||
TextEditRules::DocumentIsEmpty()
is a wrapper of TextEditor::IsEmpty()
so
that we can get rid of it simply.
HTMLEditRules::DocumentIsEmpty()
needs to change. It's oddly checks only
EditorBase::mPaddingBRElementForEmptyEditor
is nullptr
or not. However,
the editor may be completely empty. And the result may be different from
TextEditor::IsEmpty()
which is not overridden by HTMLEditor
. For partially
solving this issue, this patch makes HTMLEditor
overrides IsEmpty()
and
optimizes TextEditor::IsEmpty()
.
With this change, the caller of HTMLEditRules::DocumentIsEmpty()
may behave
differently in the only caller, HTMLEditor::SelectEntireDocument()
. And
unfortunately, its root called from SelectAllCommand::DoCommand()
. However,
it does just collapse Selection
into the root element (<body>
or
Document.documentElement
) if it returns true
. Therefore, this change
must be safe since anyway SelectionRefPtr()->SelectAllChildren()
with
the root element is exactly same as SelectionRefPtr()->Collapse()
with
the empty root element if it's truly empty.
Assignee | ||
Comment 7•5 years ago
|
||
And also this patch make each AutoEditSubActionNotifier
creator check
the result of OnStartToHandleTopLevelEditSubAction()
at least for
NS_ERROR_EDITOR_DESTROYED
.
We need to take care of its destructor's result later, though.
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
And also this patch make each AutoEditSubActionNotifier
creator check
the result of OnStartToHandleTopLevelEditSubAction()
at least for
NS_ERROR_EDITOR_DESTROYED
.
We need to take care of its destructor's result later, though.
Assignee | ||
Comment 9•5 years ago
|
||
It takes a lot of bool
out arguments. Therefore, we should make it a
stack only class and caller should retrieve only necessary information.
Assignee | ||
Comment 10•5 years ago
|
||
Assignee | ||
Comment 11•5 years ago
|
||
Assignee | ||
Comment 12•5 years ago
|
||
Assignee | ||
Comment 13•5 years ago
|
||
Assignee | ||
Comment 14•5 years ago
|
||
Now, we can get rid of TextEditRules
and HTMLEditRules
completely.
And also this patch renames their cpp files to TextEditSubActionHandler
and HTMLEditSubActionHandler
.
TextEditor::Init()
and HTMLEditor::Init()
are still complicated due to
AutoEditInitRulesTrigger
. The following patch will remove it.
Assignee | ||
Comment 15•5 years ago
|
||
Only for solving the order of AutoEditInitRulesTrigger
destruction when
HTMLEditor::Init()
calls TextEditor::Init()
, TextEditor
has unnecessary
counter and the initialization code is made harder to understand.
Therefore, this patch makes TextEditor::Init()
and HTMLEditor::Init()
directly call InitEditorContentAndSelection()
at appropriate time.
Additionally, HTMLEditor::Init()
can call EditorBase::Init()
instead of
TextEditor::Init()
since TextEditor::Init()
does nothing for HTMLEditor
actually.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 16•5 years ago
|
||
Comment 17•5 years ago
|
||
Comment 18•5 years ago
|
||
Comment 19•5 years ago
|
||
bugherder |
Comment 20•5 years ago
|
||
Comment 21•5 years ago
|
||
Comment 22•5 years ago
|
||
Comment 23•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c9c5ddab9b18
https://hg.mozilla.org/mozilla-central/rev/46c0a2f0acb4
https://hg.mozilla.org/mozilla-central/rev/396a5f6301a3
https://hg.mozilla.org/mozilla-central/rev/7442cbe672da
https://hg.mozilla.org/mozilla-central/rev/f1c646b0f812
https://hg.mozilla.org/mozilla-central/rev/e71921b729c6
Comment 24•5 years ago
|
||
bugherder |
Assignee | ||
Updated•5 years ago
|
Comment 25•5 years ago
|
||
Comment 26•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/72e2b71294b4
https://hg.mozilla.org/mozilla-central/rev/620fa59408e8
Description
•