[Yahoo Mail] Changing the font and size of the text renders highlights poorly
Categories
(Core :: DOM: Editor, defect, P3)
Tracking
()
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
- Launch Firefox and go to https://mail.yahoo.com
- Log in with a valid account.
- Click on the “Compose” button to open a new e-mail window.
- Select a different font and text size.
- Select a highlight color.
- 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.
Reporter | ||
Updated•1 year ago
|
Assignee | ||
Comment 1•1 year ago
|
||
I think I can fix this quickly.
Assignee | ||
Comment 2•1 year ago
|
||
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
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.
Comment 5•1 year ago
|
||
Backed out for causing for causing bp hybrid bustages
Backout link: https://hg.mozilla.org/integration/autoland/rev/bb00921066d90664b36f059152354bac8a8d0a32
Assignee | ||
Comment 6•1 year ago
|
||
Sorry for the bustage. I'll take a look soon.
Assignee | ||
Comment 7•1 year ago
|
||
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?
Comment 8•1 year ago
|
||
Sure, we'll reland this one and back out bug 1807829
Thanks for the heads up
Assignee | ||
Comment 9•1 year ago
|
||
(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.
Comment 10•1 year ago
|
||
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
Comment 11•1 year ago
|
||
Backed out changeset cb55480eb7ff (Bug 1808906) for causing wpt failures on multitest.html.
Backout link
Push with failures <--> wpt5
Failure Log
Assignee | ||
Comment 12•1 year ago
|
||
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.
Comment 13•1 year ago
|
||
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
Comment 15•1 year ago
|
||
bugherder |
Upstream PR merged by moz-wptsync-bot
Comment 17•1 year ago
|
||
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
towontfix
.
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 18•1 year ago
|
||
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.
Reporter | ||
Comment 19•1 year ago
|
||
Verified on Nightly 111.0a1(build ID: 20230119093127) on macOS 12, Windows 10, Ubuntu 22.
Description
•