1792386 - Change `Selection` result after changing selected text with `execCommand`

Status: RESOLVED FIXED

(Core :: DOM: Editor, enhancement, P2)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

When execCommand("bold") is performed for a[b]c, Gecko makes a[<b>b]</b>c. This will be changed to a<b>[b</b>]c if we simply switch the split node direction in bug 1735608. This inconsistency is terrible because we make the new behavior opt-out with new execCommand command.

On the other hand, Chrome does a<b>[b]</b>c. I think that Chrome's behavior is more reasonable than ours because it selects only the text which applied the new style, i.e., all of the range is applied the new style. Additionally, the result does not need to depend on the split node direction.

Perhaps, we should follow the Chrome's behavior.

Comment 2

[Jimmy]

Perhaps Firefox should just copy every single behaviour of WebKit and Chromium. I have been building a rich text editor for Firefox over the last 5-6 months, and Firefox is absolutely terrible to work with. The Selection API is just one big mess of compatibility issues. Please discard the selection API and write a new from scratch that just copies the behaviour of Chromium and WebKit. I like Firefox, but things like this is why Firefox is not going to attract more users.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Bringing over the S2 severity from the duplicate bug. Feel free to drop it back to S3 if you disagree.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1792386 - part 1: Make `HTMLEditor::SetInlinePropertiesAsSubAction` extend and/or shrink range smarter r=m_kato!

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

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1792386 - part 2: Make `AutoInlineStyleSetter` store updated range to compute new `Selection` after applying the style r=m_kato!

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

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1792386 - part 3: Clean up `HTMLEditor::PromoteInlineRange` and `HTMLEditor::PromoteRangeIfStartsOrEndsInNamedAnchor` r=m_kato!

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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1792386 - part 4: Make `HTMLEditor::RemoveInlinePropertiesAsSubAction` handle deleting bold style coming from parent in its own loop r=m_kato!

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

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1792386 - part 5: Move some jobs in `HTMLEditor::RemoveInlinePropertiesAsSubAction` into `AutoInlineStyleSetter` r=m_kato!

Depends on D164525

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(sickish, maybe slower respose)]

Attachment: Bug 1792386 - part 6: Make `HTMLEditor::RemoveInlinePropertiesAsSubAction` shrink the selection range r=m_kato!

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bde5b32f6762
part 1: Make `HTMLEditor::SetInlinePropertiesAsSubAction` extend and/or shrink range smarter r=m_kato

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37592 for changes under testing/web-platform/tests

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fdad088f180
part 2: Make `AutoInlineStyleSetter` store updated range to compute new `Selection` after applying the style r=m_kato

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00c291a4751b
part 3: Clean up `HTMLEditor::PromoteInlineRange` and `HTMLEditor::PromoteRangeIfStartsOrEndsInNamedAnchor` r=m_kato

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61991a0482c4
part 4: Make `HTMLEditor::RemoveInlinePropertiesAsSubAction` handle deleting bold style coming from parent in its own loop r=m_kato

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb7615f4b6f4
part 6: Make `HTMLEditor::RemoveInlinePropertiesAsSubAction` shrink the selection range r=m_kato

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/1d1c36b6caef
part 5: Move some jobs in `HTMLEditor::RemoveInlinePropertiesAsSubAction` into `AutoInlineStyleSetter` r=m_kato

Comment 17

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/bde5b32f6762
https://hg.mozilla.org/mozilla-central/rev/2fdad088f180
https://hg.mozilla.org/mozilla-central/rev/00c291a4751b
https://hg.mozilla.org/mozilla-central/rev/61991a0482c4
https://hg.mozilla.org/mozilla-central/rev/1d1c36b6caef
https://hg.mozilla.org/mozilla-central/rev/bb7615f4b6f4

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot