`WSRunScanner` and `TextFragmentData` should not refer style of scan start container for checking whether specific node is preformatted or not
Categories
(Core :: DOM: Editor, defect, P2)
Tracking
()
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
Attachments
(5 files)
I don't like to change traditional behavior before shipping beforeinput event. However, this bug causes odd failure in some WPT, and this must not affect actual web apps because web apps must not put preformatted inline elements into contenteditable.
Currently, TextFragmentData (formally, WSRunScanner and its derived class, WSRunObject) check whether a point is preformatted text or not with "scan start point". Therefore, if white-space sequence across multiple inline elements and some of them have different white-space style value, its behavior is broken. So, we need to fix this bug 1644313, unfortunately.
|
Assignee | |
Comment 1•5 years ago
|
||
Currently, TextFragmentData stores whether the scan start point is
preformatted or not. However, it does not make sense because the class
may contain other text nodes which may have different style in its range.
The main job of TextFragmentData is managing white-spaces as visible
sequence or invisible sequence. So, preformatted white-space should be
treated as visible character because of not collapsible with adjacent
formatted ASCII white-spaces.
First of all, this patch its initializer stop scanning white-spaces if
it meets non-empty preformatted text node.
Note that the new failures are caused by the difference whether which
white-space sequence should be normalized when modifying text at text node
or inline element boundary. This difference should be fixed in another
bug because our new normalizer does not handle this same as Blink for now.
|
|
Assignee | |
Comment 2•5 years ago
|
||
Simply, if a white-space is in a preformatted text node, we shouldn't modify
it for normalizing. This patch adds such checks to various points in
WSRunObject.cpp.
Depends on D84317
|
|
Assignee | |
Comment 3•5 years ago
|
||
The check does not make sense because TextFragmentData's range may contain
multiple text nodes which may have different style.
With the previous patch, the range won't cross characters in preformatted
text nodes. Therefore, their result won't contain preformatted characters
right now. So, we can just drop the unnecessary check from them.
Depends on D84319
|
|
Assignee | |
Comment 4•5 years ago
|
||
Traditionally, WSRunScanner (and formerly WSRunObject) treats all text
nodes in its range when scan start container node has preformatted style.
This means that when starting start from start or end of preformatted text node
or inline element, it treats adjacent white-spaces which is not preformatted
as preformatted.
This patch fixes this issue. Because of the fix of preceding patches,
BoundaryData stops scanning if it meets preformatted text node so that
this patch makes the scanners mark whether the boundary crossed or not a
preformatted character from scan start point to reason content.
Note that the above change causes new failures with <listing> element which
should be treated as <pre> element, but HTMLEditUtils treats it as
non-container inline element.
https://html.spec.whatwg.org/multipage/dom.html#elements-in-the-dom:listing
Therefore, this patch changes the definition of <listing> element and
<xmp> element which is also mentioned in the above link to container
element etc. <listing> element is treated exactly same as <pre>,
therefore, the new definition is same as <pre>, but <xmp> does not
allow any tags after opens it (i.e., even if its close tag). Therefore,
<xmp> definition is different from <listing> and <pre> elements'
definition.
Depends on D84320
|
|
Assignee | |
Comment 5•5 years ago
|
||
The patches will fix about a thousand known test failures (mostly in fontname.html) in the traditional path. And also they will fix about 10 test failures in new white-space normalizer path.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 6•5 years ago
|
||
With the previous patch, 2 bugs become visible.
One is a MOZ_ASSERT in
WhiteSpaceVisibilityKeeper::MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange()
is wrong. When TextFragmentData::GetReplaceRangeDataAtEndOfDeletionRange()
returns a replace range, it may set the start to inclusive next character of
end to delete. So, start of replace range may start after end to delete when
container of end to delete is not a text node and its next editable char point
is an ASCII white-space. Therefore, this patch rewrite it with
MOZ_ASSERT_IF and EditorDOMPoint::IsInTextNode().
The other is TextFragmentData::GetReplaceRangeDataAtEndOfDeletionRange()
and TextFragmentData::GetReplaceRangeDataAtStartOfDeletionRange() may call
TextFragmentData::GetFirstASCIIWhiteSpacePointCollapsedTo() and
TextFragmentData::GetEndOfCollapsibleASCIIWhiteSpaces() with a point in
preformatted text node. Therefore, this patch adds the check into them.
Depends on D84323
|
||
Comment 8•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/70f2b09f0b57
https://hg.mozilla.org/mozilla-central/rev/64991750fbdb
https://hg.mozilla.org/mozilla-central/rev/b17cfb4cf764
https://hg.mozilla.org/mozilla-central/rev/d418b81621a9
https://hg.mozilla.org/mozilla-central/rev/724aa6e8cdc6
|
||
Comment 9•5 years ago
|
||
The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.
For more information, please visit auto_nag documentation.
|
|
Assignee | |
Comment 10•5 years ago
|
||
No, this is too risky change for beta, and this is long standing bug.
Description
•