Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

text-combine-upright: all & text-transform don't work together #8703

Closed
wants to merge 1 commit into from
Closed

text-combine-upright: all & text-transform don't work together #8703

wants to merge 1 commit into from

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Jan 16, 2023 •
edited by webkit-early-warning-system 

`text-combine-upright: all` & `text-transform` don't work together
https://bugs.webkit.org/show_bug.cgi?id=250657

Reviewed by NOBODY (OOPS!).

This patch is to align WebKit with Gecko / Firefox and Blink / Chromium.

Merge - https://src.chromium.org/viewvc/blink?view=rev&revision=167624

When we transform text, we set transformed text to
RenderText::m_text. RenderCombineText overwrites the text at
combineText() and use originalText() at combineTextIfNeeded() in
painting. Therefore, text combine paints original text instead of
transformed text.

* Source/WebCore/rendering/RenderCombineText.h: Add "m_renderedText" as String definition
* Source/WebCore/rendering/RenderCombineText.cpp:
(RenderCombineText::combineTextIfNeeded): Change 'originalText' to "m_renderedText" and call it also while object replacement
* LayoutTests/fast/text/text-combine-upright-text-transform.html: Add Test Case
* LayoutTests/fast/text/text-combine-upright-text-transform-expected.html: Add Test Case Expectation

2537d1f

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ❌ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ❌ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-wk2
✅ 🛠 tv-sim ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 402b261)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ❌ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2 ⏳ 🧪 api-mac   🧪 gtk-wk2
✅ 🧪 api-ios   🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-wk2
✅ 🛠 tv-sim   🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 added the CSS Cascading Style Sheets implementation label Jan 16, 2023
@Ahmad-S792 Ahmad-S792 self-assigned this Jan 16, 2023
nt1m
nt1m approved these changes Jan 16, 2023
View reviewed changes
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test could be a web-platform-test in the css/css-writing-modes directory, but not required for this patch.

LayoutTests/fast/text/text-combine-text-transform-expected.html Outdated Show resolved Hide resolved
LayoutTests/fast/text/text-combine-text-transform-expected.html Outdated Show resolved Hide resolved
LayoutTests/fast/text/text-combine-text-transform.html Outdated Show resolved Hide resolved
LayoutTests/fast/text/text-combine-text-transform.html Outdated Show resolved Hide resolved
Source/WebCore/rendering/RenderCombineText.h Outdated Show resolved Hide resolved
@Ahmad-S792 Ahmad-S792 marked this pull request as ready for review January 16, 2023 23:24
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 16, 2023
@Ahmad-S792 Ahmad-S792 force-pushed the fix250657-text-combin-upright-transform branch from 402b261 to d757636 Compare January 16, 2023 23:55
@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash d757636)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe   🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ⏳ 🛠 wincairo
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 gtk-wk2
✅ 🧪 api-ios   🧪 mac-wk1   🧪 api-gtk
✅ 🛠 tv   🧪 mac-wk2
✅ 🛠 tv-sim   🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Jan 16, 2023
@Ahmad-S792
text-combine-upright: all & text-transform don't work together
2537d1f 
`text-combine-upright: all` & `text-transform` don't work together
https://bugs.webkit.org/show_bug.cgi?id=250657

Reviewed by NOBODY (OOPS!).

This patch is to align WebKit with Gecko / Firefox and Blink / Chromium.

Merge - https://src.chromium.org/viewvc/blink?view=rev&revision=167624

When we transform text, we set transformed text to
RenderText::m_text. RenderCombineText overwrites the text at
combineText() and use originalText() at combineTextIfNeeded() in
painting. Therefore, text combine paints original text instead of
transformed text.

* Source/WebCore/rendering/RenderCombineText.h: Add "m_renderedText" as String definition
* Source/WebCore/rendering/RenderCombineText.cpp:
(RenderCombineText::combineTextIfNeeded): Change 'originalText' to "m_renderedText" and call it also while object replacement
* LayoutTests/fast/text/text-combine-upright-text-transform.html: Add Test Case
* LayoutTests/fast/text/text-combine-upright-text-transform-expected.html: Add Test Case Expectation
@Ahmad-S792 Ahmad-S792 force-pushed the fix250657-text-combin-upright-transform branch from d757636 to 2537d1f Compare January 17, 2023 00:57
@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash 2537d1f)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ❌ 🛠 🧪 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk ✅ 🛠 wincairo
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ❌ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ❌ 🧪 mac-wk2
✅ 🛠 tv-sim ❌ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 17, 2023
nt1m
nt1m reviewed Jan 17, 2023
View reviewed changes
Source/WebCore/rendering/RenderCombineText.cpp
@@ -99,7 +100,7 @@ void RenderCombineText::combineTextIfNeeded()

// An ancestor element may trigger us to lay out again, even when we're already combined.
if (m_isCombined)
RenderText::setRenderedText(originalText());
RenderText::setRenderedText(m_renderedText);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the original intended was to copy the string.

@darinadler @cdumez The original Blink patch did RenderText::setRenderedText(m_renderedText.impl()), although I'm not sure if that's the common pattern.

Copy link
Contributor

@cdumez cdumez Jan 17, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me. No reason to take the StringImpl here, they should be equivalent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like maybe the Blink merge simply isn't working, the initial version without any of my requested changes had the test failing on Windows.

alanbaradlay
alanbaradlay reviewed Jan 17, 2023
View reviewed changes
Source/WebCore/rendering/RenderCombineText.h
@@ -53,6 +54,7 @@ class RenderCombineText final : public RenderText {
float m_combinedTextWidth { 0 };
float m_combinedTextAscent { 0 };
float m_combinedTextDescent { 0 };
String m_renderedText;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This naming is confusing. It does not represent the rendered combined text, this is pre-combined after-transformed text.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not even sure if I agree with this approach, but since RenderCombine is going to go through a proper refactoring soonish, I guess this will do for now.

nt1m
nt1m requested changes Jan 17, 2023
View reviewed changes
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #8703 (comment), seems like the patch isn't working (even before my requested changes).

I'd recommend contributing the tests to WPT so we can make sure we fix this later on however.

@Ahmad-S792
Copy link
Contributor Author

Left comment on main bug and closing this PR now. Thanks all!

@Ahmad-S792 Ahmad-S792 closed this Jan 17, 2023
@Ahmad-S792 Ahmad-S792 deleted the fix250657-text-combin-upright-transform branch March 31, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdumez cdumez cdumez left review comments

@alanbaradlay alanbaradlay alanbaradlay left review comments

@nt1m nt1m nt1m requested changes

Assignees

@Ahmad-S792 Ahmad-S792

Labels
CSS Cascading Style Sheets implementation merging-blocked Applied to prevent a change from being merged
Projects
None yet
Milestone
No milestone
6 participants
@Ahmad-S792 @webkit-early-warning-system @cdumez @nt1m @alanbaradlay @webkit-ews-buildbot