Closed Bug 1569902 Opened 5 years ago Closed 5 years ago

Editor should stop using non-standardized attributes to put `<br>` elements for padding of empty lines

Categories

(Core :: DOM: Editor, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(5 files)

Editor creates <br mozbogusnode="true"> for empty editor and <br type="_moz"> for empty last line of <textarea> and blocks in HTML editor (without such hacky <br> elements, caret position is computed with 0-height line). However, those attributes can be modified by web apps and might conflict with future spec. Additionally, the runtime cost of setting/removing/checking the attributes may appear in profiles.

I believe that editor should treat HTMLBRElement directly and HTMLBRElement should have specific flags which mean each of the attributes.

Then, we can save the runtime cost, especially mutation observers (including IMEContentObserver).

Editor creates a <br> element when it's root element is empty.
Then, it's stored by TextEditRules::mBogusNode and used for checking
whether the editor is empty quickly. However, this <br> element has
mozeditorbogusnode attribute whose value is true. However, adding or
removing the attribute is not cheap and web apps can refer such illegal
attribute.

Therefore, this patch makes HTMLBRElement take a specific flag whether
it's a bogus node or not. However, this means that this hacky thing will be
exposed outside editor module. For making what is the bogus node clearer,
this patch calls the such <br> elements as "padding <br> element for
empty editor". So, this patch also includes a lot of renaming methods and
variables, and modifying related comments.

Editor creates a <br> element to end of a block if last line
of the block is empty because caret should be placed as there is an empty
line. Such special <br> element has type attribute whose value is "_moz".
However, adding/removing the attribute is expensive and such hacky attribute
shouldn't be referred nor changed by web apps.

Therefore, this patch makes HTMLBRElement take another specific flag whether
it's a special node for empty last line. For making the meaning clearer,
this patch calls the such <br> elements as "padding <br> element for
empty last line" insead of "moz-br". So, this patch also includes a lot of
renaming methods and variables, and modifying related comments.

Note that with this change, IMEContentObserver counts the padding <br>
element in <textarea> because it's inserted before setting the new flag
and setting the flag does not cause DOM tree mutation. This issue will be
fixed by the following patches.

TextEditRules::CreateBR() is used only by
HTMLEditRules::InsertBRIfNeededInternal() and it just calls
TextEditor::InsertBrElementWithTransaction(). Therefore, we can get rid of
it. Then, CreateBRInternal() can be renamed to
CreatePaddingBRElementForEmptyLastLine() since it's shared only by
CreateBR() and CreatePaddingBRElementForEmptyLastLine().

Stopping using attribute for "moz-br", IMEContentObserver cannot know when
new <br> element is changed to padding element for empty last line.
Therefore, editor needs to insert padding <br> element after setting the
flag properly. Then, IMEContentObserver does not need to recompute the
length of <br> element (if it's for padding, it computes the length as 0).

Unfortunately, TextEditor::InsertBrElementWithTransaction() is used in too
many places and it already has optional argument. Therefore, it's difficult
to change it. However, we should share the preparation before creating <br>
element in it with new method. Therefore, this patch creates
EditorBase::PrepareToInsertBRElement() to share the preparation point (almost
just moved from the method). Then, new method is created as
EditorBase::InsertPaddingBRElementForEmptyLastLineWithTransaction() because
it's used both in TextEditor and HTMLEditor. Finally, TextEditor won't
insert <br> element with InsertBrElementWithTransaction(). Therefore, it's
moved to HTMLEditor with renaming to InsertBRElementWithTransaction().

IMEContentObserver treats <br> elements as linefeed, but ignores padding
<br> elements. Padding <br> element for last empty line was inserted as
normal <br> element and then, its attribute was set by editor. Therefore,
IMEContentObserver needed to observe attribute changes. However, editor
stops using attribute to mark as padding and stops inserting as normal <br>
element first (i.e., inserting <br> after setting its flag). Therefore,
IMEContentObserver does not need to observe attribute changes anymore.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/53fd693e44a7
part 1: Stop using attribute to consider whether a `<br>` element is an editor bogus node or not r=m_kato,smaug
https://hg.mozilla.org/integration/autoland/rev/d3d34552e75d
part 2: Stop using attribute to consider whether a `<br>` element is a special node for empty last line or not r=m_kato,smaug
https://hg.mozilla.org/integration/autoland/rev/b9ffd7bff146
part 3: Get rid of `TextEditRules::CreateBR()` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/5a67c3b41886
part 4: Make `TextEditor` inserts padding `<br>` element for empty last line after setting flags to `NS_PADDING_FOR_EMPTY_LAST_LINE` r=m_kato
https://hg.mozilla.org/integration/autoland/rev/483c129b10e3
part 5: Make `IMEContentObserver` stop observing attribute change r=smaug
Blocks: 1540029
No longer blocks: redesign-editor-module
Type: enhancement → task
Regressions: 1609918
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: