There are 3 bugs.
One is, `PendingStyles::TakeAllPreservedStyles` returns styles in reversed order
of setting styles, this unexpected order causes that `<font>` element is placed
innermost instead of outermost, thus, background color may be applied to shorter
area than text. Unfortunately, Chrome applies the style in fixed order, but
using same order as setting the styles is reasonable for us because we use
same code to apply multiple styles once.
Another is, `AutoInlineStyleSetter` extends the range to apply new style to
parents even if the given point is not in start or end of the first container.
Current code works fine if there is only one text node in an inline element,
but after un-setting a style, text nodes may be keep split. And also when we
apply new style, we create an empty text node for a placeholder of new text,
therefore, the range may be extended from the empty text node to before/after
its previous/next text node. Note that the latter case needs to work with
invisible things more, but this bug needs to be fixed ASAP due to the dead line,
and invisible things not so appear in inline elements. Therefore, it's okay
to leave it.
The other is, the delete handlers do not clear the cached styles (cached when
`HTMLEditor` starts handling it). If it's caused by a `Backspace` key press
in start of a block, `HTMLEditor` caches the style at caret (right paragraph),
then, collapse `Selection` to end of the ex-left block where may have different
style, then, when user types new text there, the cached style is applied into
to the new text **too**. Therefore, if `Selection` was collapsed and handling
a `Backspace` key press, the cached styles should be forgotten.
Depends on D168640
Differential Revision: https://phabricator.services.mozilla.com/D168865
Gmail sets `styleWithCSS` to `true` before setting `foreColor`, then, sets
`styleWithCSS` to `false` before user typing text. Currently, we normalize
given color value to a proper value for current styling mode, and we have not
assumed that the mode is changed during preserving the style.
For solving this issue, `HTMLEditor::AutoInlineStyleSetter::ApplyStyle` should
normalize the given value again if the value is not proper for
`<font color="...">` (at this moment, the color value is normalized to `#XXXXXX`
style or `rgb(n, n, n)` or `rgba(n, n, n, n)`, therefore the check is simple in
this patch).
Then, I realized that according to `editing/run/forecolor.html` result [1],
the other browsers accept CSS color value as the parameter of `foreColor`
command. Therefore, this patch also allows it, thus, this patch fixes a lot of
WPT failures.
1. https://wpt.fyi/results/editing/run/forecolor.html%3F1-1000?sha=95f9b15d9a&label=master&max-count=1&view=subtest
Depends on D168179
Differential Revision: https://phabricator.services.mozilla.com/D168639
The new path added in bug 1807829 hits the odd result case of `ClearStyleAt`.
`pointToPutCaret` may be updated if the preceding split occurs. Therefore,
it should be returned if it does not split next nodes after the first split
instead of returning unset point (because of not splitting the point,
`unwrappedSplitNodeResult.AtSplitPoint` may return unset point, therefore,
it may return unset point in the case).
Additionally, I forgot to split delete `SplitNodeResult::mCaretPoint` in
bug 1792654, and it causes `SplitNodeDeepWithTransaction` returns unset caret
point. This patch fixes this bug too.
Differential Revision: https://phabricator.services.mozilla.com/D168179
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
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
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
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
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
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
Masayuki Nakano