Closed Bug 1808906 Opened 1 year ago Closed 1 year ago

[Yahoo Mail] Changing the font and size of the text renders highlights poorly

Categories

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

defect

Tracking

()

VERIFIED FIXED
111 Branch
Tracking Status
firefox108 --- wontfix
firefox109 --- wontfix
firefox110 --- wontfix
firefox111 --- verified

People

(Reporter: oardelean, Assigned: masayuki)

References

()

Details

Attachments

(2 files)

Notes

  • This does not reproduce in Chrome. It seems to happen only if the font is selected prior to the highlight color. Please see the attachment for more details.

Found in

  • Nightly 110.0a1

Affected versions

  • Nightly 110.0a1
  • Firefox 109.0b9
  • Firefox 108.0.1

Tested platforms

  • macOS 12
  • Ubuntu 22
  • Windows 10

Affected platforms

  • macOS 12
  • Ubuntu 22
  • Windows 10

Unaffected platforms

  • N/A

Steps to reproduce

  1. Launch Firefox and go to https://mail.yahoo.com
  2. Log in with a valid account.
  3. Click on the “Compose” button to open a new e-mail window.
  4. Select a different font and text size.
  5. Select a highlight color.
  6. Type something in the e-mail window.

Expected result

  • Highlight color is aligned with the text font and size.

Actual result

  • Highlight color is not aligned with the text font and size.

Regression range

  • Will look for one as soon as possible.
Severity: -- → S4
Has STR: --- → yes

I think I can fix this quickly.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All

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 PreservedStyles 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

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/649e12877126
Make the style editor handle `<font>` at last when there are multiple preserved styles r=m_kato
Failed to create upstream wpt PR due to merge conflicts. This requires fixup from a wpt sync admin.
Flags: needinfo?(masayuki)

Sorry for the bustage. I'll take a look soon.

Flags: needinfo?(masayuki)

Oh, I think that the bustage must be caused by https://treeherder.mozilla.org/jobs?repo=autoland&revision=dfd4ec69146560b6de52edbc4acdb49762daf38c which the patch for this bug depending on. So, could you back it out too?

Flags: needinfo?(smolnar)

Sure, we'll reland this one and back out bug 1807829
Thanks for the heads up

Flags: needinfo?(smolnar)

(In reply to Sandor Molnar from comment #8)

Sure, we'll reland this one and back out bug 1807829
Thanks for the heads up

No, don't reland this. It depends on the patch for bug 1807829.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/cb55480eb7ff
Make the style editor handle `<font>` at last when there are multiple preserved styles r=m_kato

Backed out changeset cb55480eb7ff (Bug 1808906) for causing wpt failures on multitest.html.
Backout link
Push with failures <--> wpt5
Failure Log

Flags: needinfo?(masayuki)

Oh, sorry for taking your time again. I'm not sure why it unexpectedly passes even though I don't see the error in tryserver. I'll retry to land it after checking it after rebasing.

Flags: needinfo?(masayuki)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b1cd7f16a87b
Make the style editor handle `<font>` at last when there are multiple preserved styles r=m_kato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/38029 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 111 Branch
Upstream PR merged by moz-wptsync-bot

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.

Flags: needinfo?(masayuki)

I think that it's risky to uplift without testing in Nightly era because it changes our traditional behavior so that some web apps could be regressed by this change.

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

Status: RESOLVED → VERIFIED
Duplicate of this bug: 1680592
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: