1807829 - [Yahoo mail] Changing text size does not respect spacing

Status: RESOLVED FIXED

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

Description

[Ardelean Oana, Desktop QA [:oardelean]]

Attachment: Screen Recording 2022-12-28 at 16.43.27.mov

Notes

• This does not reproduce in Chrome. Please see the attached video for more clarity.

Found in

• Nightly 110.0a1;

Affected versions

• Nightly 110.0a1;
• Firefox 108.0.1;
• Firefox 109.0b7;

Tested platforms

• macOS 12;
• Ubuntu 22;
• Windows 10;

Affected platforms

• macOS 12;
• Ubuntu 22;
• Windows 10;

Unaffected platforms

• N/A;

Preconditions

• The user is signed in to yahoo mail with a valid account.

Steps to reproduce

1. Open a new mail by clicking on the "Compose" button.
2. From the text formatting options, select the “Huge” size.
3. Type a few rows with the selected text size.
4. Observe the spacing after each written row.
5. From the text formatting options, select the “Tiny” size.
6. Type a few rows with the selected text size.
7. Observe the spacing after each written row.

Expected result

• Spacing should reflect the text size.

Actual result

• Spacing is not re-applied after changing the text size.

Regression range

• Not a recent regression, will look for a regression range as soon as possible.

Comment 1

[Alice0775 White]

In firefox, the editor insert font tag without remove previous font tag as follows:
<div dir="ltr" data-setdir="false"><font size="7"><font size="1">test</font></font></div>

However, in Edge(chrome):
<div dir="ltr" data-setdir="false"><font size="1">test</font></div>

Comment 2

[Ardelean Oana, Desktop QA [:oardelean]]

I tried searching for a regression range but it seems that I can reproduce the issue up until Nightly 71.0a1(build ID: 20190922213341).
I think it's safe to assume that this is not a regression.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I should check whether it's a bug of the web app or Gecko.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1807829 - Make `HTMLEditor::CreateStyleForInsertText` use same logic as `HTMLEditor::SetInlinePropertiesAsSubAction` to apply new style r=m_kato!

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.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dfd4ec691465
Make `HTMLEditor::CreateStyleForInsertText` use same logic as `HTMLEditor::SetInlinePropertiesAsSubAction` to apply new style r=m_kato

Comment 6

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

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

Comment 7

[Cosmin Sabou [:CosminS]]

Backed out for causing bp-hybrid bustages on HTMLEditUtils.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=hybrid&fromchange=0f2913c81558409941b9594754528df1bdfebf3a&tochange=52ec9536cbbb04064f9dac7ac6c69a4cf466651f&selectedTaskRun=aKqz9nxvSOGVK4aErAoShA.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=402592418&repo=autoland

backout link: https://hg.mozilla.org/integration/autoland/rev/52ec9536cbbb04064f9dac7ac6c69a4cf466651f

Comment 8

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

Upstream PR was closed without merging

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5bacd687fa98
Make `HTMLEditor::CreateStyleForInsertText` use same logic as `HTMLEditor::SetInlinePropertiesAsSubAction` to apply new style r=m_kato

Comment 10

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/5bacd687fa98

Comment 11

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

Upstream PR merged by moz-wptsync-bot

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:masayuki, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox110 to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Ardelean Oana, Desktop QA [:oardelean]]

Verified on Nightly 111.0a1(build ID: 20230119163652) on macOS 12, Ubuntu 22, Windows 10.