Currently, our editor sets `color` attribute of `<font>` to given value as-is.
However, the other browsers normalize it to `#[0-9a-z]{6}` before setting the
value, and some WTP expects the behavior. Therefore, we need to follow it for
avoiding meaningless failures.
According to WPT, the parameter can be `"transparent"` even without
`styleWithCSS`. Additionally, CSS color values are also allowed if
`styleWithCSS` is `true`. Therefore, this patch includes some fixes for them
too to avoid new failures.
Differential Revision: https://phabricator.services.mozilla.com/D167500
The readonly flag of `HTMLEditor` can be set by chrome script or
`Document.execCommand("contentReadOnly")`. However, the XPCOM APIs should keep
working to update the editable content. E.g., if focused editor is a single
line text field, the app may want to change the value even if it's not editable
by the users.
Note that this patch does not fix all cases of all APIs because this is not
important for Firefox even though the status can be created by web apps (but
Firefox does not use XPCOM APIs basically, instead using XUL commands and it
should not work with readonly state since it may be kicked by user
interactions, e.g., executing a menu item or a shortcut key).
Therefore, it's enough to work it in current Thunderbird for now.
Differential Revision: https://phabricator.services.mozilla.com/D167353
Currently, Gecko does not support updating `font-size` style even in the CSS
mode. Therefore, `CSSEditUtils` claims that `<font size="...">` style is not
CSS editable. Supporting it requires a lot of new code but currently we don't
have any reports about this poor support. Therefore, we should only try to
make the style editor remove `font-size` properties around selection at setting
`<font size="...">` style for now.
Additionally, Blink and WebKit does not allow to nest multiple `<font>` elements
and `<span>` elements which specify `font-size`, `font-family` or `color`. When
setting one of the styles, they split ancestors which are related to the style
at range boundaries. Then, unwrap the unnecessary elements. This makes the
DOM tree simpler, thus, the later handling will be simpler. Therefore, it's
worthwhile to follow them.
Unfortunately, there are some logic differences when handling styles in elements
having `id` attribute. Therefore, there are some new test failures, but I guess
that it does not cause any problems in web apps in the wild because it's hard to
manage editable identified elements in cross browsers and I have no idea of
motivations to do it.
Depends on D166617
Differential Revision: https://phabricator.services.mozilla.com/D167011
Height of inline elements are considered with current `font-size` and
`line-height`. Therefore, if content in inline elements are taller, the
`background-color` does not fill the bigger content background entirely.
For solving this issue, Chrome handles styles of `<font>` element as
outer-most style. This is reasonable approach, let's follow this.
For solving this issue, we can change the order of `PreservedStyle`s at setting
the preserved styles. Then, `SetInlinePropertiesAsSubAction` is called with
reversed order and apply later style first and applies newer styles to all
content in an element which is previously inserted. Therefore, the `<font>`
element styles should be last elements of `PendingStyles::mPreservingStyles`.
When applying new style, our style editor does not reuse existing `<font>`
element, and this causes writing WPT harder. Therefore, this patch also changes
the applying range of `<font>` style to wrapping existing `<font>` element if
and only if its content is entirely selected.
Unfortunately, this approach cannot get exactly same result as Chrome because we
insert outer-most `<font>` element first, then, try to apply `background-color`,
at this moment, our style editor applies the style to the previously inserted
`<font>` element instead of creating new `<span>` element. This behavior is
required for compatibility in the other cases. Additionally, changing only this
behavior requires a lot of method changes to specify how to handle it. However,
this incompatible behavior may not cause any problems in web apps in the wild.
Therefore, this patch does not solve this incompatible issue. I think that once
we get a bug report caused by this difference, we should redesign how to set
multiple inline styles once.
Differential Revision: https://phabricator.services.mozilla.com/D166617
Height of inline elements are considered with current `font-size` and
`line-height`. Therefore, if content in inline elements are taller, the
`background-color` does not fill the bigger content background entirely.
For solving this issue, Chrome handles styles of `<font>` element as
outer-most style. This is reasonable approach, let's follow this.
For solving this issue, we can change the order of `PreservedStyle`s at setting
the preserved styles. Then, `SetInlinePropertiesAsSubAction` is called with
reversed order and apply later style first and applies newer styles to all
content in an element which is previously inserted. Therefore, the `<font>`
element styles should be last elements of `PendingStyles::mPreservingStyles`.
When applying new style, our style editor does not reuse existing `<font>`
element, and this causes writing WPT harder. Therefore, this patch also changes
the applying range of `<font>` style to wrapping existing `<font>` element if
and only if its content is entirely selected.
Unfortunately, this approach cannot get exactly same result as Chrome because we
insert outer-most `<font>` element first, then, try to apply `background-color`,
at this moment, our style editor applies the style to the previously inserted
`<font>` element instead of creating new `<span>` element. This behavior is
required for compatibility in the other cases. Additionally, changing only this
behavior requires a lot of method changes to specify how to handle it. However,
this incompatible behavior may not cause any problems in web apps in the wild.
Therefore, this patch does not solve this incompatible issue. I think that once
we get a bug report caused by this difference, we should redesign how to set
multiple inline styles once.
Differential Revision: https://phabricator.services.mozilla.com/D166617
The problem in Yahoo mail is, when you apply new font size to:
```
<div><font size="7">{}<br></font></div>
```
then, when you type text, you'll get:
```
<div><font size="7"><font size="4">abc<br></font></font></div>
```
because `HTMLEditor::CreateStyleForInsertText` does not check ancestors to
apply new style to empty text node which is created by the method for
placeholder. Of course, the other browsers update the existing
`<font size="7">` instead and we should follow it.
However, there are 3 problems:
1. Our editor may adjust insertion point to nearest editable text node if caret
in empty inline elements.
2. Our editor supports increase/decrease font size which have been removed from
`Document.execCommand`, but Thunderbird keeps using it, and that's implemented
with an independent method.
3. Our style editor code does not support applying style to collapsed selection.
Therefore, this patch does:
1. keep create empty text node to apply new style at specific point.
2. give up to use new path if there is preserved font size increment/decrement.
3. separate `HTMLEditor::SetInlinePropertiesAsSubAction` and add new path for
applying style to collapsed range.
Then, `HTMLEditor::CreateStyleForInsertText` can use
`HTMLEditor::SetInlinePropertiesAsSubAction` which supports handling new styles
with existing ancestors.
Differential Revision: https://phabricator.services.mozilla.com/D166416
Height of inline elements are considered with current `font-size` and
`line-height`. Therefore, if content in inline elements are taller, the
`background-color` does not fill the bigger content background entirely.
For solving this issue, Chrome handles styles of `<font>` element as
outer-most style. This is reasonable approach, let's follow this.
For solving this issue, we can change the order of `PreservedStyle`s at setting
the preserved styles. Then, `SetInlinePropertiesAsSubAction` is called with
reversed order and apply later style first and applies newer styles to all
content in an element which is previously inserted. Therefore, the `<font>`
element styles should be last elements of `PendingStyles::mPreservingStyles`.
When applying new style, our style editor does not reuse existing `<font>`
element, and this causes writing WPT harder. Therefore, this patch also changes
the applying range of `<font>` style to wrapping existing `<font>` element if
and only if its content is entirely selected.
Unfortunately, this approach cannot get exactly same result as Chrome because we
insert outer-most `<font>` element first, then, try to apply `background-color`,
at this moment, our style editor applies the style to the previously inserted
`<font>` element instead of creating new `<span>` element. This behavior is
required for compatibility in the other cases. Additionally, changing only this
behavior requires a lot of method changes to specify how to handle it. However,
this incompatible behavior may not cause any problems in web apps in the wild.
Therefore, this patch does not solve this incompatible issue. I think that once
we get a bug report caused by this difference, we should redesign how to set
multiple inline styles once.
Depends on D166416
Differential Revision: https://phabricator.services.mozilla.com/D166617
The problem in Yahoo mail is, when you apply new font size to:
```
<div><font size="7">{}<br></font></div>
```
then, when you type text, you'll get:
```
<div><font size="7"><font size="4">abc<br></font></font></div>
```
because `HTMLEditor::CreateStyleForInsertText` does not check ancestors to
apply new style to empty text node which is created by the method for
placeholder. Of course, the other browsers update the existing
`<font size="7">` instead and we should follow it.
However, there are 3 problems:
1. Our editor may adjust insertion point to nearest editable text node if caret
in empty inline elements.
2. Our editor supports increase/decrease font size which have been removed from
`Document.execCommand`, but Thunderbird keeps using it, and that's implemented
with an independent method.
3. Our style editor code does not support applying style to collapsed selection.
Therefore, this patch does:
1. keep create empty text node to apply new style at specific point.
2. give up to use new path if there is preserved font size increment/decrement.
3. separate `HTMLEditor::SetInlinePropertiesAsSubAction` and add new path for
applying style to collapsed range.
Then, `HTMLEditor::CreateStyleForInsertText` can use
`HTMLEditor::SetInlinePropertiesAsSubAction` which supports handling new styles
with existing ancestors.
Differential Revision: https://phabricator.services.mozilla.com/D166416
It returns the non-editable `<body>` causes the deletion handlers fail to
adjust range in the editing host and that caused hitting the assertion.
This patch also includes the wrong testcase with which the bug was reported.
Differential Revision: https://phabricator.services.mozilla.com/D164533
If the start boundary is immediately before the next node or if the end boundary
is immediately after the previous node, it should be shrunken for consistency
with setting style behavior and the other browsers.
Depends on D164526
Differential Revision: https://phabricator.services.mozilla.com/D164527
The remaining issue is, `RemoveInlinePropertiesAsSubAction` uses
`AutoInlineStyleSetter` for removing bold style coming from parent block or
its ancestors with new `<span style="font-weight: normal">`. However,
in the normal removing code users `Selection` range restorer. Therefore,
we cannot adjust the range simply without splitting the part of the normal
part and the special part for bold style.
This patch moves the part from an existing `for` loop which calls
`RemoveStyleInside` to a new `for` loop.
Depends on D164524
Differential Revision: https://phabricator.services.mozilla.com/D164525
They ignore non-editable nodes at scanning the DOM tree. According to the
other browsers' behavior, at least `PromoteInlineRange` should not do it.
(Removing `<a name>` is not a feature of `execCommand` and the `unlink` behavior
is anyway incompatible.)
Additionally, we don't need to stop scanning the DOM tree when we meet `<body>`.
Scanning its parents also fine because typically it's wont be extended since
there should be `<head>` element before `<body>`.
Therefore, we can make them simpler.
Depends on D164523
Differential Revision: https://phabricator.services.mozilla.com/D164524
Chrome and Safari selects content nodes which are applied the new style.
E.g., when `abc[def]ghi` gets embolden, it should become `abc<b>[def]</b>ghi`,
but Gecko does `abc[<b>def]</b>ghi` because `RangeUpdater::SelAdjSplitNode`
does not update the split point (i.e., end of the new text node, "abc" in the
example) [1].
Therefore, this patch makes `AutoInlineStyleSetter` record first and last points
trying to apply the style, then, `HTMLEditor::SetInlinePropertiesAsSubAction`
can set `Selection` to the applied range.
1. https://searchfox.org/mozilla-central/rev/dd216c1307a2bf1b0d2465b9749fa86dac44303a/editor/libeditor/SelectionState.cpp#329-334
Depends on D164522
Differential Revision: https://phabricator.services.mozilla.com/D164523
Blink's behavior is better than Gecko. That is, when selected range crosses
an inline element boundary, Blink shrinks selected range to select only
children, but Gecko extends selected range to cover all inline parents around
the range. E.g., when setting `abc[<i>def]</i>ghi` to `<b>`, Chrome changes it
to `abc<i><b>def</b></i>ghi`, but Gecko changes it to `abc<b><i>def</i></b>ghi`.
The Gecko's behavior is odd especially when `abc<i>[def]</i>ghi` since user
does not select the `<i>`, but Gecko wraps it in new `<b>`.
However, if the range contains an inline element entirely, e.g.,
`abc[<i>def</i>]ghi`, Chrome changes it to `abc<b><i>def</i></b>ghi` without
shrinking the range. This is reasonable too.
On the other hand, there is a case that browsers need to extend the range.
That is an parent inline element has the style of what we're setting a range to.
E.g., `abc<span style="background-color: red"><b>[def]</b></span>ghi`, user
wants to update the style attribute of the `<span>` element rather than wrapping
the selected text node with new `<span>` element in the `<b>`. Therefore,
if shrunken range starts from very beginning of a node or ends at very end of
a node and the inline parents have same style, we need extend the range to wrap
it. (Note that Gecko deletes same styles in range with
`HTMLEditor::RemoveStyleInside`. Therefore, wrapping most distant inline
element makes Gecko works as expected.)
Finally, if the containers of range boundaries are not same one, we should
extend the range to save the number of creating element. E.g.,
`<span>[abc</span> <span>def]</span>` should become
`<b><span>abc</span> <span>def</span>` rather than
`<span><b>abc</b></span><b> </b><span><b>def</b></span>`.
Note that the new expected failures are caused by the difference of selection
range after applying the style. That should be fixed by the following patch.
Depends on D164521
Differential Revision: https://phabricator.services.mozilla.com/D164522
As of the prior patch, these are no longer needed. I removed
these with a script, then ran clang-format on the files, then
manually reverted a few unrelated changed from the formatter.
Differential Revision: https://phabricator.services.mozilla.com/D164829
Chrome preserves inline styles of first character in the deleting/replacing
if first visible thing is text. Safari preserves inline style of the first
content of the range (i.e., styles of `<img>` etc is also respected).
For now, due to the market share and Safari's behavior is not obviously better
than Chrome, let's try to emulate the Chrome's behavior. However, the rules are
complicated and probably depend on how it implements the deletion. Therefore,
this patch makes Gecko only scan following text node if selection range starts
from end of a text node.
Depends on D164520
Differential Revision: https://phabricator.services.mozilla.com/D164521
Currently, `HTMLEditor` preserves styled elements with the reversed order in
the loop.
https://searchfox.org/mozilla-central/rev/d7d5c89ee7bc70dec0fd846689ca030f94931393/editor/libeditor/HTMLEditSubActionHandler.cpp#8638-8658
I.e., if `<b><i></i></b>` is restored in the HTML styling mode, it will be
restored as `<i><b>...</b></i>`. This blocks writing tests for bug 1789574.
Therefore, this should be fixed first.
(As I commented in the method, ideally we should store all inline elements and
restore all of them, but currently we don't need to do it. It requires redesign
of the style cache, but it seems that we don't know web apps which is broken by
current behavior. Therefore, I think that we should do it when we meet a
trouble with the behavior.)
Depends on D164519
Differential Revision: https://phabricator.services.mozilla.com/D164520
This patch removes some traditional behavior check in the mochitest and
port it into expected results which match with the behavior of Chrome.
Almost all the new tests pass in Chrome, but some fails due to odd result in
Chrome. E.g., Chrome sometimes append `style` attribute with empty value.
Therefore, the expectations in this patch must be what Chrome developers
expected at implementing it.
Depends on D164004
Differential Revision: https://phabricator.services.mozilla.com/D164519
The other browsers use any inline elements to set CSS property for applying
an inline style. However, Gecko limits it to a `<span>` which does not have
any attributes. The other browsers' design is better for saving number of
elements and runtime cost of inserting new element (moving all children to the
new element and inserting it to the original position). Therefore, it's nicer
to follow the other browses. Then, we can avoid new WPT failures at aligning
other behaviors to the other browsers.
With doing that, removing style code requires complicated changes because
`RemoveStyleInside` assumes that one element has one style, but after taking
the compatible behavior, one element can have multiple styles including the
style represented by the element itself.
Note that the new expected failures are caused by bug 1183844. Gecko returns
closest ancestor element's background color for
`queryCommandValue("backColor")`. Therefore, it returns "transparent" of the
inner `<span>` element.
Differential Revision: https://phabricator.services.mozilla.com/D164004
Now, we can make there simpler because the part does one of them:
* Replaces element with new `<span>`
* Removes element
* Removes attribute
* Does nothing
However, there are duplicated code. We should consider what there does with
lambdas.
Depends on D164002
Differential Revision: https://phabricator.services.mozilla.com/D164003
This change makes the following changes smaller. Once we remove CSS style
first, then, it becomes easier to consider whether we should delete the element
since `style` attribute may be deleted by `ChangeStyleTransaction` if the
removing style is the last rule in it.
Depends on D164000
Differential Revision: https://phabricator.services.mozilla.com/D164001
In the test case, `deleteFromDocument` call makes the point unset. We should
make it check whether a legacy mutation event listener creates unexpected result
after calling `RemoveAlignFromDescendants`.
Differential Revision: https://phabricator.services.mozilla.com/D163767
Gecko wraps selection (and parent elements if entirely selected in them) in
new `<span>` element and set `text-decoration`. However, the other browsers
tries to reuse selected or parent element which already has `text-decoration`
style. The other browsers' behavior is more reasonable from point of view of:
* smaller footprint
* minimizing to update the DOM tree
And aligning the behavior makes it easier to check the compatibility between
browsers and us avoid from new test failures aligning other behaviors to the
other browsers.
If there is an element specifying `text-decoration`, its `text-decoration`
declaration should be updated first.
If found element is `<i>`, `<s>` or `<strike>`, it should be replaced with new
`<span>` because these elements just represents the visual style and we should
not use such elements in the CSS mode (bug 1802736). At this time, unless
the element has `text-decoration` rules in its `style` attribute value, we
the new `text-decoration` style should have the value represented by the
removing element (i.e., `underline` for `<i>`, `line-through` for the others).
However, if found element is `<ins>` or `<del>`, we should set its
`text-decoration` and unless it already has `text-decoration` rules, we need
to append corresponding style (`underline` for `<ins>` and `line-through` for
`<del>`) too.
When setting the values or removing a value from `text-decoration` declaration,
the value should be normalized to represent only `text-decoration-line` for
compatibility with the other browsers and keeping the implementation simpler.
And also the value should be built as the following order:
1. underline
2. overline
3. line-though
rather than updating current value with complicated code. Then, the tests can
compare with one expectation.
Depends on D163188
Differential Revision: https://phabricator.services.mozilla.com/D163429
The other browsers do not move styling nodes to existing element if the element
is never created in the mode. For example, `execCommand("bold")` in the CSS mode
for `[abc]<b>def</b>`, the other browsers make it
`<span style="font-weight: bold">abc</span><b>def</b>`, but Gecko does
`<b>abcdef</b>`. Similarly, `execCommand("bold")` in the HTML mode for
`[abc]<span style="font-weight: bold">def</span>`, the other browsers make it
`<b>abc</b><span style="font-weight: bold">def</span>`, but Gecko does it
`<span style="font-weight: bold">abcdef</span>`.
This patch makes Gecko align the behavior to the other browsers.
Differential Revision: https://phabricator.services.mozilla.com/D163188
`execCommand("superscript")` and `execCommand("subscript")` are handled
specially.
`<sup>` and `<sub>` are exclusive. Therefore, existing element is removed first.
However, they are not mapped to CSS due to bug 394304 comment 2 even though the
behavior is different from the other browsers. Therefore, Gecko does not remove
`vertical-align` styles from selected ranges. On the other hand, the other
browsers do it. This makes sense and matches with the handling in HTML mode.
Therefore, we should remove `vertical-align` style around the selection ranges.
Note that this updates existing WPTs.
The first block of each command checks how the command work with similar
`<span>` having `vertical-align` style. The new expectation matches with the
other browsers (i.e., currently they fail in the other browsers).
The second block of each command checks how the command work with similar
situation but there is a sibling which has the applying style (i.e., `<sup>`
or `<sub>`). These tests keep failing in the other browsers since they want
to update `vertical-align` style at the selected range in the `<span>` having
`vertical-align` style.
Anyway, this fix is for avoiding new WPT failures of fixing bug 1792386, and
these commands must not be used so widely especially in the CSS mode.
Therefore, I believe that this change is not so risky.
Depends on D163183
Differential Revision: https://phabricator.services.mozilla.com/D163184
* `SetInlinePropertyOnTextNode`
* `SetInlinePropertyOnNode`
* `SetInlinePropertyOnNodeImpl`
* `ElementIsGoodContainerForTheStyle`
These `HTMLEditor` methods run only when `SetInlinePropertyOnTextNode` or
`SetInlinePropertyOnNode` is called, they take common style information as 3
arguments and they run in a short period. Therefore, they can be in a stack
only class.
Differential Revision: https://phabricator.services.mozilla.com/D163183
When keyboard events are fired by auto repeat feature of OS, each event is fired
in each event loop. However, `repeat` feature of the `synthesizeKey` and
typing multiple characters with `sendString` dispatch keyboard events in an
event loop. Therefore, asynchronous scrolling is handled differently from
users' scenario. So we should emulate the users' scenario better with
synthesizing each event with waiting a tick.
Depends on D162527
Differential Revision: https://phabricator.services.mozilla.com/D162528
It uses legacy `hitEventLoop` hack with `setTimeout`. Now, we have better
utility methods and can avoid using callbacks with using async/await.
However, waiting only for `waitToClearOutAnyPotentialScrolls` does not fix
the intermittent failure. Therefore, this patch checks whether the scroll
result in its wrapper to make sure that there is no pending scroll.
Differential Revision: https://phabricator.services.mozilla.com/D162527
And also making it and `BuildCSSDeclarations` return pairs of CSS property and
value with an array makes their callers simpler.
Depends on D162517
Differential Revision: https://phabricator.services.mozilla.com/D162518
This patch creates a base class of `EditorInlineStyle`, the name is
`EditorElementStyle`. It requires only attribute of HTML. However, it's
obviously different from `EditorInlineStyle`'s rule which is `mHTMLProperty`
cannot be `nullptr`. Therefore, the methods which can treat only
`EditorInlineStyle` do not want the data only whose attribute is not `nullptr`.
For solving this issue at build time, this approach is better than renaming
`EditorInlineStyle` and make it have the new mode.
Depends on D162513
Differential Revision: https://phabricator.services.mozilla.com/D162514
Most of them are used only by the WebIDL bindings. Therefore, this fixes only
one caller in `HTMLEditUtils`.
`GetText` requires to change `nsContentUtils`, therefore it'll be done by the
following patch.
Differential Revision: https://phabricator.services.mozilla.com/D162065
Due to the preceding `DeleteInvisibleASCIIWhiteSpaces` call, the destination
may be lost since `aLeftContentInBlock` may have already been removed from the
DOM tree. Therefore, it should stop handling the deletion when destination is
modified by the web app.
Depends on D161423
Differential Revision: https://phabricator.services.mozilla.com/D161424
It still refers `Selection` via `EditorBase::GetFirstRangeStartPoint`, however,
it should work with `aRanges` instead.
Depends on D161422
Differential Revision: https://phabricator.services.mozilla.com/D161423
Before this patch, we had two `checkVisibilty` methods on the
nsISelectionController interface, backed by several layers of implementation,
ultimately backed by a single function on nsTextFrame (which didn't actually
do anything meaningful with any of the parameters).
As it turns out, this API only had one caller, in HTMLEditUtils.cpp.
This patch converts that caller to directly query nsTextFrame (if the given
node's primary frame is indeed a nsTextFrame). The direct function-call is
renamed to HasVisibleText(), to be a bit clearer about it being text-specific
and also to avoid confusion with the (unrelated) recently-specified HTML
checkVisibility() API.
With these changes, we can remove the API from the nsISelectionController
interface and its implementations.
This patch also updates the HTMLEditUtils::IsInVisibleTextFrames documentation
(with s/all/any/) to reflect the reality of what the nsTextFrame impl actually
does.
Differential Revision: https://phabricator.services.mozilla.com/D160563
If the range ends at a node, its parents are never iterated by
`ContentSubtreeIterator` because it collects the nodes end in the given range.
However, if the range starts from a new empty parent node, e.g.,
```
abc[<b>def]</b>
```
Then, the `<b>` element should be deleted rather than alive as `abc<b></b>`.
Therefore, unless the end container does not match with start container, it
should extend the range to contain empty parents while the range ends at end
of a node.
Differential Revision: https://phabricator.services.mozilla.com/D160165
If the range ends at a node, its parents are never iterated by
`ContentSubtreeIterator` because it collects the nodes end in the given range.
However, if the range starts from a new empty parent node, e.g.,
```
abc[<b>def]</b>
```
Then, the `<b>` element should be deleted rather than alive as `abc<b></b>`.
Therefore, unless the end container does not match with start container, it
should extend the range to contain empty parents while the range ends at end
of a node.
Differential Revision: https://phabricator.services.mozilla.com/D160165
`nsITransactionManager` provides too low level things and editor may want to
stop using it in the future if Undo Manager spec is implemented in another
browser. Therefore, I'd like to stop exposing `nsITransactionManager` instance
for editor.
Note that `mozilla::TransactionManager` is still created and used by the
UI of mailer of SeaMonkey (i.e., under `mailnews`), once we move
`nsITransactionManager` instance moved into SeaMonkey, we can make
`mozilla::TransactionManager` stop inheriting `nsITransactionManager`.
Differential Revision: https://phabricator.services.mozilla.com/D160008
Native behaviour on MacOS dictates one whitespace being removed after double-clicking a word and pressing delete.
This behaviour is achieved by saving the information that the selection is created by doubleclick to the `nsFrameSelection`
and using it in the `DeleteRangeTransaction`, where the range is extended by one whitespace character before or after the range.
Differential Revision: https://phabricator.services.mozilla.com/D159613
The testcase is tricky. It creates 2 `Selection` ranges, one is collapsed at
end of the `<html>`, the other is collapsed in the new `<h3>` element. The
first one is ignored by delete handler since Gecko does not allow to edit
outside `<body>` for now.
Then, deleting non-collapsed selection ranges tries to delete empty parent
blocks at the remaining collapsed selection range in the `<h3>`. At this time,
it works with `nsIEditor::eNone`. Then, its `GetNewCaretPosition` does not
return a valid point. Then, the `Run` does not remove the `Selection` range
outside the `<body>`.
Therefore, `HTMLEditor::DeleteSelectionAndPrepareToCreateNode()` will see
2 ranges, then, hit the assertion.
Although there are some other cases which meet 2 or more `Selection` ranges
after deletion in `DeleteSelectionAndPrepareToCreateNode`, but for now, we
should make `AutoEmptyBlockAncestorDeleter::Run` collapse `Selection` when
it deletes empty ancestors.
Differential Revision: https://phabricator.services.mozilla.com/D159581
The failure on Android is caused by async initialization of `<textarea>` runs
after initializing selection in it. Therefore, `ArrowUp` is synthesized when
all text is selected and collapses it to start.
Then, I see another failure on Linux and macOS in `contenteditable` case at
running TV and TV-nonfis. It seems that it's caused by remaining `Selection`
of the preceding test (`vertical-rl` case) does something wrong. Once
removing all ranges at `blur`ring, the failures in `vertical-lr` case has
gone.
Differential Revision: https://phabricator.services.mozilla.com/D159566
Including it in `HTMLEditor.h` causes creating static constructors for
the modules which include `HTMLEditor.h`. And the public methods are used
only in the `libeditor`. Therefore, we can make `HTMLEditor.h` stop including
it and stop exposing `JoinSplitNodeDirection.h`.
Differential Revision: https://phabricator.services.mozilla.com/D159363
One of the reason of bug 1793873 is, `HTMLEditUtils::IsSplittableNode` returns
`false` for `<textarea>`. Then, `insertParagraph` etc command with `Selection`
collapsed in `<textarea>` causes splitting the `<textarea>` and it's not split
at same parent.
Then, I found that we need to take care `<button>` specially. It creates a
block formatting context, and it's a form control so that it should never be
split. Therefore, `HTMLEditor::GetAncestorElement` requires to take care of
`<button>` element with additional scan option for minimizing the risk.
Differential Revision: https://phabricator.services.mozilla.com/D159229
It restores `Selection` with `AutoSelectionRestorer` instance created first.
Therefore it does not want the callers (currently, only
`HTMLEditor::HandleOutdentAtSelection` only) change `Selection` after doing it
without special reasons. Therefore, it shouldn't return last caret point
suggestion which is not a good point to put caret actually. Then, callers
do not need to handle it as they've never done.
Differential Revision: https://phabricator.services.mozilla.com/D159231
nsISupports.h includes nsISupportsBase.h, so it should be equivalent.
In the next patch, I'm changing things so that nsISupports is defined in
nsISupports.h instead of nsISupportsBase.h, and deleting the latter, so
this change will be needed anyways. I'm guessing people were using IWYU
or something like that.
Differential Revision: https://phabricator.services.mozilla.com/D159169
Between splitting a node and undoing it, web apps can move split nodes anywhere.
Therefore, it shouldn't assume they are always in same parent node, and
`RangeUpdater::SelAdjJoinNodes` needs to handle it correctly.
Unfortunately, `RangeUpdater::SelAdjJoinNodes` cannot handle nested cases
correctly, e.g., right node was in `aRemovedContent` or right node was in
the container of `aStartOfRightContent.GetContainer()`. However, it's not
a new regression, and such complicated situation breaks undoing anyway.
Therefore, I think that we don't need to care about it for now.
Differential Revision: https://phabricator.services.mozilla.com/D159230
It's the only method which is used outside of the editor module. Therefore,
we can stop exposing `EditorUtils.h` later.
Depends on D158634
Differential Revision: https://phabricator.services.mozilla.com/D158635
It's currently referred from outside only by `TextServicesDocument.cpp`.
It's not under `libeditor` but in the editor module. Therefore, let's allow
it to refer the header files under `libeditor` directly. Then, we can stop
exposing `HTMLEditUtils.h`.
Depends on D158633
Differential Revision: https://phabricator.services.mozilla.com/D158634
They are used in public inline methods of `HTMLEditor`. Therefore, they should
be defined in an exposed header file without other internal use things for the
editor module. Then, we can stop exposing `HTMLEditHelpers.h`.
Depends on D158632
Differential Revision: https://phabricator.services.mozilla.com/D158633
It's always called with `DeleteSelectedContent::Yes`. Therefore, the argument
is not required. Then, its name can be `InsertFromTransferableAtSelection`.
Differential Revision: https://phabricator.services.mozilla.com/D158483
It just creates `HTMLWithContextInserter` and calls its `Run()`. Then, `Run()`
works as a sub action handler. Therefore, we can make `DoInsertHTMLWithContext`
(which sounds like a helper class of a handler method) a sub action handler.
Then, it can handle the optional insertion point in it and
`HTMLWithContextInserter::Run` does not need to handle it.
Differential Revision: https://phabricator.services.mozilla.com/D158482
They use `bool` arguments a lot. Therefore, some call-sites are hard to read.
They should be replaced with `enum class`es. Note that this patch does not
make the raw value of new `enum class`es to `bool` unless they are used in the
heap.
Differential Revision: https://phabricator.services.mozilla.com/D158481
`nsIEditor.undo` and `nsIEditor.redo` are called with `1` except by the search
bar, and search bar wants to undo everything to reset the value. Therefore,
search bar needs an API to undo all, and the others do not need the number of
undoing/redoing transactions. Therefore, this patch adds `nsIEditor.undoAll`
for search bar, and remove the arguments from `nsIEditor.undo` and
`nsIEditor.redo`.
Differential Revision: https://phabricator.services.mozilla.com/D158338
They are methods to take 2 out params, and it's not convenient for light use.
I think that there should be 3 attributes, `undoRedoEnabled`, `canUndo` and
`canRedo`. Then, the findbar does not need to check number of transactions
with `nsITransactionManager`.
Differential Revision: https://phabricator.services.mozilla.com/D158337
`nsIEditor.transactionManager` is used only for some simple purposes, however,
`nsIEditor` exposes the rich API. That makes it harder to maintain internal
code around transactions. Instead, `nsIEditor` exposes only simple and
necessary APIs.
This patch creates a new API to clear undo/redo history and make the users in
mozilla-central use it instead of using `nsITransactionManager.clear()`.
Differential Revision: https://phabricator.services.mozilla.com/D158336
I misunderstood the traditional behavior before bug 1789967, that is,
`HTMLEditor` collapsed `Selection` to end of the deepest last child container
element **or text node** of the `<body>`, rather than to end of **only** the
deepest last child container element. This misunderstanding caused the backout
of the first landing, but I didn't realize this mistake.
Therefore, this patch makes the crashtests which are touched in bug 1789967 and
the collapsed point is at a text node (or failed to consider the collapsed
point) collapse `Selection` to end of the deepest last text node of their
`<body>` elements.
Differential Revision: https://phabricator.services.mozilla.com/D158177
If splitting element is a list item element,
`HandleInsertParagraphInListItemElement` may just move it into a list element
if it's followed by the list element, or may create new paragraph element if
the list item element is empty and last list item element.
If splitting element is a heading element,
`HandleInsertParagraphInHeadingElement` may create new paragraph element if
splitting at end of the heading element and not followed by `<br>`.
Therefore, neither `SplitNodeResult` nor `CreateElementResult` is proper for
their result type, and the caller `InsertParagraphSeparatorAsSubAction`, just
wants to know the right handle element which should contain caret and a caret
point suggestion.
For solving these issues, this gives an alias to `CreateElementResult`, makes
them return the right paragraph even if it's not newly created nor split
element, and replaces `blockElementToPutCaret` with the returned element.
Differential Revision: https://phabricator.services.mozilla.com/D158106
The test compares the node name of the split element and its previous sibling
element, so it should switch the scanning direction with the pref to switch
split node direction.
Note that the behavior is different from the other browsers in some cases.
Therefore, we cannot port this to WPT.
Differential Revision: https://phabricator.services.mozilla.com/D158105
The check assumes that the container of `aPointToInsert` is always right node.
However, its child is moved to new node if we switch the direction, so we should
make it check the edge case with `SplitNodeResult::GetNextContent()`.
Differential Revision: https://phabricator.services.mozilla.com/D158104
The reason why the changes of `SplitNodeTransaction` is smaller than
`JoinNodesTransaction` is, `HTMLEditor::DoSplitNode` does almost all things
instead.
Differential Revision: https://phabricator.services.mozilla.com/D158102
This patch adds new path to adjust selection for the new split node direction.
So this does not change any behavior of the existing path.
Differential Revision: https://phabricator.services.mozilla.com/D158100
This changes the existing path's behavior a bit. However, it should not affect
to web apps in the wild because it must be really rare case that web apps
inserting new nodes into the removing node while moving its children.
Differential Revision: https://phabricator.services.mozilla.com/D158099
This patch add new path to adjust selection for the new join nodes direction.
So this does not change any behavior of the existing path.
Differential Revision: https://phabricator.services.mozilla.com/D158098
There are only 3 places where nsMemory.h is still needed (image/RasterImage.cpp,
gfx/thebes/gfxFT2FontList.cpp, and nsMemory.cpp). Remove the rest.
Differential Revision: https://phabricator.services.mozilla.com/D158213
Any callers do not refer "ignored", "handled" and "cancel" state without
checking whether the method returns error or not. Therefore, error state
can be (and should be) managed by `mozilla::Result`'s error state.
Perhaps, it should store a caret position later because deletion handlers
of `HTMLEditor` use it, but update `Selection` immediately.
Differential Revision: https://phabricator.services.mozilla.com/D158080
Similar to the previous patch, this changes a lot of lines. However, I think
that it's not so hard to investigate regression point in this patch because
`CreateNodeResultBase` is not used so many times at handling on edit action.
Differential Revision: https://phabricator.services.mozilla.com/D157575
Similar to the previous patch, this changes a lot of lines. However, I think
that it's not so hard to investigate regression point in this patch because
`CreateNodeResultBase` is not used so many times at handling on edit action.
Differential Revision: https://phabricator.services.mozilla.com/D157575
Unfortunately, it's hard to split this (and similar) patch because we need to
touch `MoveNodeResult` itself for making it available as the ok type of
`Result`. However, I guess that it's not so hard to investigate regression
point if something would be found later because `MoveNodeResult` is mainly
used by deletion which is the most complicated part of `HTMLEditor`, but
`MoveNodeResult` is used only in a few cases.
Differential Revision: https://phabricator.services.mozilla.com/D157574
This is a hack for compatibility with the other browsers. When we move first
line of `<pre>` to left paragraph whose `white-space` style does not make it
preformatted, they drop the last linefeed because of unnecessary.
Differential Revision: https://phabricator.services.mozilla.com/D157419
It splits inline elements at the destination of first line in the right block.
However, it typically creates empty inline elements before the right block and
may be never used because it sets the insertion point to before the right node
of the splitting.
Therefore, it should stop creates empty inline elements (if they are required,
they should be created in `HTMLEditor::MoveNodeOrChildrenWithTransaction`
instead) and adjust split point after the element if it didn't split any nodes.
Differential Revision: https://phabricator.services.mozilla.com/D157418
When the first line of right block element is moved before the block element,
unnecessary line break may be moved to immediately before the right block
element. In the case, it needs to clean it up instead of trying to find
unnecessary line break at end of the left block which is a container of the
right block element.
Differential Revision: https://phabricator.services.mozilla.com/D157417
Empty inline nodes except non-container nodes are not required in the
destination paragraph. Therefore, it should just remove the node from the
DOM tree.
Differential Revision: https://phabricator.services.mozilla.com/D157416
We don't ignore invisible data node at joining 2 paragraphs and this is
a different behavior from the other browsers. When looking for a content
from current block boundary, `AutoBlockElementsJoiner` should keep scanning
visible things with ignoring invisible data nodes. Then, it should delete
all invisible things after joining the paragraphs.
Differential Revision: https://phabricator.services.mozilla.com/D157414
Chrome and Safari preserve `white-space` with `style` attribute to keep
collapsible or preserved white-spaces as-is. If an HTML element is moved,
`style` attribute should be set to it. Otherwise, create `<span>` element
whose `style` attribute has the declaration for `white-space` and move
content into it.
Differential Revision: https://phabricator.services.mozilla.com/D157413
Gecko just joins 2 blocks when editable block parents are same element, e.g.,
both are `<div>`. However, Chrome and Safari moves only first line of the
right block into the left block, and Gecko does it when both blocks are
different elements.
Ideally, we should take same behavior as Chrome and Safari because it's
reasonable for both compatibility with the other browsers and consistency
when both blocks are different but has same style, then we don't need to
maintain different behavior paths.
However, doing it for all elements are too risky because right block will be
merged into left block if right block has no line break. On the other hand,
without doing it, preserving `white-space` is really hard because we need to
maintain the both paths.
Therefore, I'd like to change the behavior only when both blocks have different
`white-space` styles. Usually, web apps do not change `white-space` for each
block, so I think that this is safer than doing this in all elements,
additionally, we can revert the behavior easy since this patch is really small.
Differential Revision: https://phabricator.services.mozilla.com/D157412
This fixes bug 503838 partially. The new utility method scans unnecessary
`<br>` with strict check as far as possible. Then, we can delete the node or
the preformatted line break at end of the last text node.
Differential Revision: https://phabricator.services.mozilla.com/D157411
This patch ports most part of `editor/libeditor/tests/test_bug772796.html` to
WPT because this kind of behaviors are not tested by `editing/run/delete.html`
nor `editing/run/forwarddelete.html`. (Not ported tests are invalid HTML
structure cases and list item cases, the reason why not doing this for the
latter is, it needs a lot of cases and not important for most web apps.)
The most expectations are based on Chrome and Safari (they both behave almost
same), but they fail a lot in `join-pre-and-other-block.html` and
`white-space: pre-line` cases in the other tests.
Even though this ports a lot of cases, for making easier to compare the
behavior change in the following patches, we should keep the tests.
Differential Revision: https://phabricator.services.mozilla.com/D157410
The line never runs in automated tests, and
`RemoveBlockContainerElementsWithTransaction` does not check whether the DOM
tree gets as expected or not. Therefore, I have no idea how to test this.
Differential Revision: https://phabricator.services.mozilla.com/D157573
It may be called even when there is no selection range and focused element.
However, it assumes that there is a selection range, and an editable element
has focus. Therefore, now, if there is an editing host and user tries to
do "Select All" without clicking somewhere before doing it, "Select All" does
nothing.
Differential Revision: https://phabricator.services.mozilla.com/D157409
They and their callees work with the result of `GetRoot()` which is the document
element or the body element. If the body is not editable, `Selection` should
not be updated in non-editable region nor `<br>` elements should not be
inserted in both non-focused editable elements and non-editable elements.
Therefore, they should run only when the document element or the `<body>`
element is editable.
To keep testing crashtests as reported, this patch makes tests which have
`contenteditable` except `<html>` and `<body>` initialize `Selection` as
what we've done. And clean up the tests for helping to port them to WPT
in the future (bug 1725850).
Differential Revision: https://phabricator.services.mozilla.com/D157408
It does different thing for `TextEditor` and `HTMLEditor`, and used almost
internally. Therefore, it should be implemented in the sub classes and
we should name them better.
Differential Revision: https://phabricator.services.mozilla.com/D157407
The method is enough simple, and uses bad cast from point of view of OOP.
Therefore, this patch make the sub classes implement the method only for each.
Differential Revision: https://phabricator.services.mozilla.com/D157406
Deletion of mutation observers from a list resulted in O(n^2) behavior and could lead to massive freezes.
This is resolved by using a LinkedList instead, reducing complexity to O(n).
A safely iterable doubly linked list was implemented based on `mozilla::DoublyLinkedList`,
allowing to insert and remove elements while iterating the list.
Due to the nature of `mozilla::DoublyLinkedList`, every Mutation Observer now inherits `mozilla::DoublyLinkedListElement<T>`.
This implies that a Mutation Observer can only be part of one DoublyLinkedList.
This conflicts with some Mutation Observers, which are being added to multiple `nsINode`s.
To continue supporting this, new MutationObserver base classes `nsMultiMutationObserver` and `nsStubMultiMutationObserver` are introduced,
which create `MutationObserverWrapper` objects each time they are added to a `nsINode`.
The wrapper objects forward every call to the actual observer.
Differential Revision: https://phabricator.services.mozilla.com/D157031
The method is enough simple, and uses bad cast from point of view of OOP.
Therefore, this patch make the sub classes implement the method only for each.
Differential Revision: https://phabricator.services.mozilla.com/D157406
It does different thing for `TextEditor` and `HTMLEditor`, and used almost
internally. Therefore, it should be implemented in the sub classes and
we should name them better.
Differential Revision: https://phabricator.services.mozilla.com/D157407
The method is enough simple, and uses bad cast from point of view of OOP.
Therefore, this patch make the sub classes implement the method only for each.
Differential Revision: https://phabricator.services.mozilla.com/D157406
Unfortunately, the result is really different from the other browsers.
E.g., when `execCommand("indent")` at `<div>abc<br>{}<br></div>`, Gecko inserts
a `<div>` and set its `margin-left`. However, Chrome splits parent `<div>`
element and wrap the `<div>` which contains the caret in new `<blockquote>`.
Therefore, this does not contain new WPT.
Depends on D157404
Differential Revision: https://phabricator.services.mozilla.com/D157405
After this change, it's possible to set the pref, or use editor.document.execCommand("styleWithCSS", false, "true") to enable css in mail compose.
Differential Revision: https://phabricator.services.mozilla.com/D157276
When selection is `abc<b>[def</b>]ghi`, `insertParagraph` command will delete
the `<b>` element first, then, `Selection` becomes `abc{}ghi`. Then,
`HTMLEditor::InsertParagraphSeparatorAsSubAction` wraps all of the line in
the default paragraph, `<div>`, with
`HTMLEditor::FormatBlockContainerWithTransaction` (although this is incompatible
behavior with the other browsers). At this time, new `<div>` is inserted before
the first text node and then, move the text nodes into the new `<div>`.
However, `RangeUpdater::DidMoveNode` just slides the offsets if containers of
registered DOM points are the ex-parent of the moving nodes. Therefore, the
tracked selection range in `HTMLEditor::FormatBlockContainerWithTransaction`
become `<div></div>abc{}def`, then, `<div>abcdef</div>{}`, but the expected
behavior is of course, `<div>abc{}def</div>`, then, split the new `<div>`.
So the problem is, `DidMoveNode` assumes that DOM points won't point the moving
content node. If the node is pointed, it should keep pointing in the new
parent.
Note that the expectations of new tests are based on Chrome, therefore, the
new known failures are incompatible with Chrome.
Differential Revision: https://phabricator.services.mozilla.com/D156798
It's hard to know how some tests failed in test_bug1318312.html. Therefore,
this patch makes the selection range comparison easier to read, and add
description to explain what was doing at the failure.
Differential Revision: https://phabricator.services.mozilla.com/D156526
Additionally,
* `PropItem` -> `PendingStyle`
* `StyleCache` -> `PendingStyleCache`
* `AutoStyleCacheArray` -> `AutoPendingStyleCacheArray`
And finally, `PendingStyle` (formally `PropItem`) is changed to `class` and
its members are encapsuled.
Differential Revision: https://phabricator.services.mozilla.com/D155318
For consistency with the previous patch, we should rename them too. Then, we're
getting rid of unclear word "Prop" from the public methods.
Differential Revision: https://phabricator.services.mozilla.com/D155316
I usually retry to understand what they mean. Therefore, I'd like to give new
names for them (and rename `TypeInState` class in a following patch).
Differential Revision: https://phabricator.services.mozilla.com/D155315
Only the value member needs to be updated when setting the prop multiple times.
Therefore, we cannot change all members to constants.
Differential Revision: https://phabricator.services.mozilla.com/D155313
According to the debug, its value can be CSS property value if in the CSS mode.
For making the value meaning easier to understand, this renames it to
mAttributeValueOrCSSValue.
Differential Revision: https://phabricator.services.mozilla.com/D155312
It's currently no problem to manage it with a raw pointer because it may be
set to a dynamic atom only when `nsIHTMLEditor.insertLinkAroundSelection` is
called with unknown attribute, but comm-central uses it only with `href`
attribute.
I think that we should change the API just to take `href` value in the future,
but for now, it should be `RefPtr<nsAtom>`.
Differential Revision: https://phabricator.services.mozilla.com/D155311
It's always a pointer to `nsStaticAtom` instance or `nullptr`. Therefore,
it can be `nsStaticAtom*` and we can make its users treat `nsStaticAtom`
instead of `nsAtom`.
Additionally, this patch changes some pointer parameters to references if
they are never `nullptr`.
Differential Revision: https://phabricator.services.mozilla.com/D155310
In bug 1739524, I misunderstood the meaning of `aOffset` of `SelAdjJoinNodes`.
After joining 2 nodes, and a point points right node which will have ex-left
node content, the point needs to point ex-start of the right node to keep
next insertion point as-is. Therefore, it's not useful with new join nodes
direction, it needs to know the ex-offset of the right node.
Differential Revision: https://phabricator.services.mozilla.com/D155438
Before the fix of bug 1758420, `TextComposition`'s text node and offset in it
are updated at creating `CompositionTransaction`. However, it's now put off
until getting the mutations. Therefore, it always unsets `pointToInsert` at
first time of the composition and fails to notify the listeners.
This patch makes it retrieve the result after calling `DoTransactionInternal`
and handle the post processing with the new point.
Differential Revision: https://phabricator.services.mozilla.com/D155052
It's a Gecko specific feature, and it sets background color of parent block
elements of selection ranges. This does similar things to
`SetInlinePropertiesAsSubAction`, but still refers `Selection` directly and
uses `AutoSelectionRestorer`. For consistency between similar methods, this
patch makes it use `AutoRangeArray`.
Depends on D154353
Differential Revision: https://phabricator.services.mozilla.com/D154354
`HTMLEditor::RelativeFontChange()` and its helpers are based on
`SetInlinePropertiesAsSubAction()` and its helpers. Therefore, they may have
similar problem to switch join/split direction. Therefore, this and the
following patches clean them up too.
Depends on D154350
Differential Revision: https://phabricator.services.mozilla.com/D154351
I think that `HTMLEditor::SetInlinePropertyAsSubAction` and
`HTMLEditor::RemoveInlinePropertyAsSubAction` should not be called multiple
times in one edit action. Therefore, I'd like to make them take multiple
styles once. Then, we could collect all targets before touching the DOM tree
in the future.
Depends on D154348
Differential Revision: https://phabricator.services.mozilla.com/D154349
This is a pre-fix for bug 1735608. Currently, the loop assumes that collected
nodes are always right node, but it'll be changed, therefore, it creates
new array for the follow up loops handle expected nodes.
Depends on D154346
Differential Revision: https://phabricator.services.mozilla.com/D154347
I'd like to get rid of `AutoSelectionRangeArray` and `AutoRestoreSelection`,
and I'd like to make it stop touching `Selection` while it is removing style
of each content nodes in the range. Therefore, this patch rewrites it with
`AutoRangeArray`. Then, we can reduce the indent level of the nested `for`
loops.
Note that it creates `AutoEditSubActionNotifier`, so it's a sub-edit action
handler. Therefore, this patch renames it.
Depends on D154345
Differential Revision: https://phabricator.services.mozilla.com/D154346
I've already made a caller of `HTMLEditor::ClearStyleAt`,
`HTMLEditor::CreateStyleForInsertText`, in bug 1770877, so this fixes a bug of
the patch.
`HTMLEditor::ClearStyleAt` is still updates `Selection` only in some cases.
And one of the caller has not handle the `Selection` update. Therefore, once
it completely stop touching `Selection`, `ComputeEditingHost` will fail and
the other paths update `Selection`, so it should do it too. (Without the
change, `test_dragdrop.html` fails.)
Finally, we don't need `EditResult` anymore because we have
`Result<EditorDOMPoint, nsresult>`.
Differential Revision: https://phabricator.services.mozilla.com/D154343
Similar to the previous patch, and for consistency between editor helper
classes, we should make result of getter methods of `EditorDOMPointBase` too.
Differential Revision: https://phabricator.services.mozilla.com/D153841
We'll need the version to return `dom::Text*` in coming patches for bug 1735608.
For avoiding the class to have a lot of getters, we should make result of the
getters templated.
Differential Revision: https://phabricator.services.mozilla.com/D153840
`GetLeftContent()` etc is explained as:
> This may return nullptr if the method didn't split at start edge of the node.
On the other hand, `SplitNodeResult::GetPreviousContent()` and
`SplitNodeResult::GetNextContent()` returns previous or next content node
of split point when the split is not performed, but not in error.
Therefore, it should check `SplitNodeResult::DidSplit()` instead of
`SplitNodeResult::isOk()`.
However, there is another problem. The constructor cannot specify the order
of the 2 splits. Therefore, it's hard to maintain the constructor.
Fortunately, there is only one user, so we can make the user create the
result with another constructor.
Differential Revision: https://phabricator.services.mozilla.com/D153838
If splitting node creates new right node instead of new left node,
`TopLevelEditSubActionData::mChangedRange` ends by start of the right node.
However, `RemoveEmptyNodesIn` uses `PostContentIterator` which collects DOM
nodes whose "end tag" appear in the range. Therefore, for cleaning up new
empty right nodes correctly, we need to extend the range to contain the node.
Additionally, it removes unexpected element which is editable but shouldn't be
removed from the DOM tree. Therefore, this patch adds new check such as
`HTMLEditUtils::IsRemovalNode()`.
Finally, this patch adds a check whether the editor is a mail editor or not
at considering whether a node is a "mail-cite" because `contenteditable` in
web apps do not need special handling for such Gecko-specific element.
Differential Revision: https://phabricator.services.mozilla.com/D153834
The test oddly passes without the fix in `HandleInsertBRElement`. I guess that
post-processing in `HTMLEditor::OnEndHandlingTopLevelEditSubActionInternal()`
handles `Selection`, and anyway the insertion point of the following
`insertText` is wrong. It seems that we don't need to uplift this.
Differential Revision: https://phabricator.services.mozilla.com/D153832
`increaseFontSize`, `decreaseFontSize`, `gethtml`, `heading` and `readonly`
commands were disabled for a year in all channels, but no regression reports
have been filed. Therefore, we can delete the commands and the telemetry
probes.
Note that `cmd_getContents` command which is the internal command of `gethtml`
is not used in comm-central too. Therefore, this patch deletes the command
handler, `nsClipboardGetContentsCommand`, and `Command::GetHTML` too.
Differential Revision: https://phabricator.services.mozilla.com/D153720
For doing that, this patch also making `HTMLEditor::AlignContentsAtSelection`
which the only caller of `HTMLEditor::AlignNodesAndDescendants` handle
`Selection` with `AutoRangeArray` because it uses `AutoSelectionRestorer` and
we need to adjust `Selection` after restored.
Differential Revision: https://phabricator.services.mozilla.com/D152980
It's only caller is `HTMLEditor::AlignContentsAtSelection` which is called only
by `HTMLEditor::AlignAsSubAction` at almost last of it. Therefore, it's safe
to handle `mNewBlockElement` at the caller of
`AlignContentsAtSelectionWithEmptyDivElement` is safe.
Differential Revision: https://phabricator.services.mozilla.com/D152978
The change of expected assertion count in the crash test is caused by that the
test calls `execCommand` recursively from the legacy mutation event listeners.
Therefore, stopping updating `Selection` for each DOM tree change causes that
the target range in the nested edit action is changed. However, the nested
handling has already been disabled by default, so this should not be a problem
for the users.
Differential Revision: https://phabricator.services.mozilla.com/D152973
It's called only by `HTMLEditor::HandleCSSIndentAtSelection` which is called
only by `HTMLEditor::HandleIndentAtSelection`. They don't touch `Selection`
after calling it. Therefore, we can make it adjust collapsing selection point
by itself.
Differential Revision: https://phabricator.services.mozilla.com/D152972
With taking an `AutoRangeArray` which is initialized with `Selection`, it can
restore selection and check if it's collapsed and if collapsed in the expected
list element. Then, its caller can apply the returned range to `Selection`
because it's an edit sub-action handler.
Differential Revision: https://phabricator.services.mozilla.com/D152969
It oddly retrieve an ancestor list element which contains one range in the
selection ranges. So, working it with `AutoRangeArray` which is initialized
with `Selection` makes `HTMLEditor` smaller...
Differential Revision: https://phabricator.services.mozilla.com/D152966
They set `TopLevelEditSubActionData::mNewBlockElement` and their root callers
in edit sub-action level are only `InsertParagraphSeparatorAsSubAction` and
`FormatBlockContainerAsSubAction`, and they are called each other. Therefore,
this patch changes these 3 methods once.
Differential Revision: https://phabricator.services.mozilla.com/D152964
The following patches touch the logic to restore selection after handling
commands. However, it seems that they are not tested because I found some
regressions by manual testing, but didn't cause orange on tryserver.
`outdent-preserving-selection.tentative.html` causes a crash due to the
`MOZ_ASSERTION` in `HTMLEditor::HandleOutdentAtSelectionInternal`. It means
that I misunderstand the logic and had put the assertion. I should fix it
later with reading the complicated code again. For now, I just change it
to `NS_WARNING_ASSERTION` instead.
And also `test_cmd_absPos.html` is the first test to check toggling `position`
between `static` and `absolute`. Therefore, it detects wrong `MOZ_ASSERT`s
which test whether `EditActionData` or `TopLevelEditSubActionData` is created
or not **before** they create them by themselves. So, this patch removes the
wrong assertions.
Differential Revision: https://phabricator.services.mozilla.com/D152962
The biggest set of APIs from ns[T]StringObsolete which are still heavily used
are the string searching APIs. It appears the intention was for these to be
replaced by the `FindInReadable` APIs, however that doesn't appear to have
happened.
In addition, the APIs have some quirks around their handling of mixed character
widths. These APIs generally supported both narrow strings and the native
string type, probably because char16_t string literals weren't available until
c++11. Finally they also used easy-to-confuse unlabeled boolean and integer
optional arguments to control behaviour.
These patches do the following major changes to the searching APIs:
1. The ASCII case-insensitive search method was split out as
LowerCaseFindASCII, rather than using a boolean. This should be less
error-prone and more explicit, and allows the method to continue to use
narrow string literals for all string types (as only ASCII is supported).
2. The other [R]Find methods were restricted to only support arguments with
matching character types. I considered adding a FindASCII method which would
use narrow string literals for both wide and narrow strings but it would've
been the same amount of work as changing all of the literals to unicode
literals.
This ends up being the bulk of the changes in the patch.
3. All find methods were re-implemented using std::basic_string_view's find
algorithm or stl algorithms to reduce code complexity, and avoid the need to
carry around the logic from nsStringObsolete.cpp.
4. The implementations were moved to nsTStringRepr.cpp.
5. An overload of Find was added to try to catch callers which previously
called `Find(..., false)` or `Find(..., true)` to set case-sensitivity, due
to booleans normally implicitly coercing to `index_type`. This should
probably be removed at some point, but may be useful during the transition.
Differential Revision: https://phabricator.services.mozilla.com/D148300
Currently, `HTMLEditor` assumes that padding `<br>` element for empty last line
is outside of inline elements, but it may happen because of both:
* starting from bug 1778091, `HTMLEditor` move `<br>` element into new empty
inline elements at inserting new paragraph.
* web apps can put it into inline elements.
After splitting inline elements and which do not have meaningful content, users
cannot put caret into the empty inline elements so that the elements stay unless
delete around there.
For avoiding the leak due to meaningless elements, we should delete them at
splitting inline elements at inserting new text.
Note that Chrome does not pass the new tests of resetting ancestor bold style
because Chrome wraps the `<b>` with `<i>`, however, the `<i>` has odd
`style=""`. Perhaps, the test framework should ignore it because it's not
important for the web-compatibility.
On the other hand, Chrome completely fails only the last testcase since it
unwraps the `<b>` from the last `<br>`, so the bold style which was applied by
the web app to the last `<br>` is lost. This is not reasonable.
Differential Revision: https://phabricator.services.mozilla.com/D152616
In Yahoo! Mail, the paragraph has `<br>` after `<span>` element which has
`background-color`. In this case, Gecko creates the following DOM tree after
splitting the paragraph:
```
<div><span>foo</span></div><div><span></span><br></div>
```
Then, the empty `<span>` in the right paragraph will be removed by the
post-processing. However, in this case, the inline element is required for
preserving the style continued from the previous paragraph.
In this case, we should move the `<br>` element into the `<span>` to make
it non-empty and avoid it to be removed. This is compatible with Chrome.
Differential Revision: https://phabricator.services.mozilla.com/D151345
Currently I'm think that the split/join node direction should be managed per
`HTMLEditor` instance because per transaction makes undo/redo handling
complicated and may require additional space to save it in each transaction
instance. However, the direction should be changeable with new Gecko specific
`execCommand` to support both direction until creating first split/join node
transaction or anytime with clearing the transactions.
Differential Revision: https://phabricator.services.mozilla.com/D149192
This removes HTMLMenuItemElement and all the code and tests preffed off
by dom.menuitem.enabled.
The HTML parser changes are the result of applying the previous patch.
Differential Revision: https://phabricator.services.mozilla.com/D149979
Note that the odd path which always returns `NS_ERROR_FAILURE` is not covered by
the tests. Therefore, this patch adds `MOZ_ASSERT` to make somebody hit it and
report a bug.
Differential Revision: https://phabricator.services.mozilla.com/D149108
Note that `CSSEditUtils` does not change `Selection` except
`RemoveCSSInlineStyleWithTransaction` which is used only by aligning in a block.
Therefore, this patch does not touch `CSSEditUtils`.
Differential Revision: https://phabricator.services.mozilla.com/D149106
The preceding call of `InsertBRElement` may collapse selection at the inserted
padding `<br>` element. Only when calling `HandleInsertParagraphInParagraph`,
`InsertParagraphSeparatorAsSubAction` uses `Selection`. So, only in this case,
we need to recompute the point to split for keeping current (odd) behavior.
Note that in normal cases, using `atStartOfSelection` gets same result.
However, if there are some invisible nodes such as comment nodes, doing it
may change the behavior. For now, we should keep the current behavior. It
should be sorted out when we make it stop inserting `<br>` elements for the
preparation of split without checking whether it's actually necessary.
Differential Revision: https://phabricator.services.mozilla.com/D149101
It touches `Selection` redundantly (i.e., immediately after that, it or its
caller collapse `Selection`). So we can just stop it because we can ignore
the cases when the handling fails after the redundant `Selection` update and
it can be caused by tricky things with the mutation event listeners.
Note that without changing `SplitNodeResult` a lot, we cannot make it return
`SplitNodeResult`, and currently there is no motivation to do it because the
only caller does not need the detail of the split. Therefore, I give up doing
it.
Differential Revision: https://phabricator.services.mozilla.com/D149099
I tried to make the latter half preparation to call
`HTMLEditor::SplitParagraphWithTransaction`, but I cannot make it cleaner
because it needs to return `HTMLBRElement*` and `EditorDOMPoint` with maybe
modifying the DOM tree. So, I keep it as-is, but I make replace the unnecessary
duplicated `EditorDOMPoint` with the original one which is adjusted to avoid
to create new empty link in the right paragraph.
Differential Revision: https://phabricator.services.mozilla.com/D149096
Using the wide scope `EditorDOMPoint pointToInsertBR` makes it harder to read.
Although this duplicates a call of `HTMLEditor::InsertBRElement` and a couple of
post processing, but I think that it's easier to read/understand.
Differential Revision: https://phabricator.services.mozilla.com/D149094
If it needs to insert a `<br>` but the editor does not want to create new
paragraph, its caller should be default to insert a `<br>` element. Rather
than checking this at inserting `<br>` element, it should do it at considering
the place to insert the new `<br>`.
Differential Revision: https://phabricator.services.mozilla.com/D149093
This patch makes it take `AutoRangeArray` and it can save a snapshot of the
ranges and restore it like `AutoSelectionRestorer`.
For accessing `HTMLEditor::RangeUpdaterRef` for clearing the saved ranges,
`AutoRangeArray` needs to store `HTMLEditor`. Therefore, its constructors
and the destructor cannot be inlined in the header due to the include-hell.
Differential Revision: https://phabricator.services.mozilla.com/D149089
They at most twice updates the `Selection` right now. We can make them update
`Selection` once. And this patch makes the scope of `AutoRangeArray` clearer.
Differential Revision: https://phabricator.services.mozilla.com/D149087
Masayuki Nakano
Marian-Vasile Laza
Cosmin Sabou
Sandor Molnar
Tom Schuster
Andrew McCreight
Jan-Niklas Jaeschke
Mark Banner
Daniel Holbert
Noemi Erli
Cristian Tuns
Chengyu Sun
Butkovits Atila
Andreea Pavel
Iulian Moraru
Magnus Melin
Makoto Kato
Nika Layzell
Tooru Fujisawa
Neil Deakin
Joel Maher
Emilio Cobos Álvarez
Norisz Fay