Skip to content

Apply text-justify: none on lines #38054

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Apply text-justify: none on lines #38054

wants to merge 1 commit into from

Conversation

AlexsanderDamaceno
Copy link
Contributor

@AlexsanderDamaceno AlexsanderDamaceno commented Dec 17, 2024 •
edited by webkit-early-warning-system
Loading

4fb2286

Apply text-justify: none on lines
https://bugs.webkit.org/show_bug.cgi?id=241304

Reviewed by NOBODY (OOPS!).

Verify if text-justify is set to none, and do not compute justification on lines.

* LayoutTests/fast/css3-text/css3-text-justify/text-justify-none-01-expected.txt: Added.
* LayoutTests/fast/css3-text/css3-text-justify/text-justify-none-01.html: Added.
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.cpp:
(WebCore::Layout::LineBuilder::placeInlineAndFloatContent):

4fb2286

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@AlexsanderDamaceno AlexsanderDamaceno self-assigned this Dec 17, 2024
@AlexsanderDamaceno AlexsanderDamaceno added the CSS Cascading Style Sheets implementation label Dec 17, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Dec 17, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 06cd35a)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ❌ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ⏳ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ❌ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv   🛠 mac-safer-cpp ❌ 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@AlexsanderDamaceno AlexsanderDamaceno changed the title Apply text-justify: none on lines; Apply text-justify: none on lines Dec 17, 2024
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Dec 17, 2024
nt1m
nt1m reviewed Dec 17, 2024
View reviewed changes
Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml Outdated
Comment on lines 1602 to 1604
Copy link
Member

Choose a reason for hiding this comment

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

inter-character/inter-word need to be implemented to enable this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to make work in the text-justify: none in this pr, it is possible or is necessary implement the others?

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, but I wouldn't enable the preference in that case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, gonna set all for false again

Copy link
Contributor Author

@AlexsanderDamaceno AlexsanderDamaceno Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If put false, the test case will not work as text-justify is disabled, there is a way to enable for text-justify: none?

Copy link
Member

Choose a reason for hiding this comment

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

The status of CSSTextJustifyEnabled is testable, which means tests should have it enabled by default

@AlexsanderDamaceno AlexsanderDamaceno removed the merging-blocked Applied to prevent a change from being merged label Dec 17, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Dec 17, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 3c4175f)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac   🛠 wpe   🛠 win
✅ 🧪 bindings   🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2   🧪 win-tests
✅ 🧪 webkitperl   🧪 ios-wk2   🧪 api-mac   🧪 api-wpe
  🧪 ios-wk2-wpt   🧪 mac-wk1 ✅ 🛠 wpe-cairo
  🧪 api-ios   🧪 mac-wk2 ✅ 🛠 gtk
  🛠 vision ⏳ 🧪 mac-AS-debug-wk2   🧪 gtk-wk2
  🛠 vision-sim   🧪 mac-wk2-stress   🧪 api-gtk
⏳ 🧪 vision-wk2   🧪 mac-intel-wk2
  🛠 tv   🛠 mac-safer-cpp
  🛠 tv-sim
  🛠 watch
  🛠 watch-sim

@AlexsanderDamaceno
Apply text-justify: none on lines
4fb2286 
https://bugs.webkit.org/show_bug.cgi?id=241304

Reviewed by NOBODY (OOPS!).

Verify if text-justify is set to none, and do not compute justification on lines.

* LayoutTests/fast/css3-text/css3-text-justify/text-justify-none-01-expected.txt: Added.
* LayoutTests/fast/css3-text/css3-text-justify/text-justify-none-01.html: Added.
* Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml:
* Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.cpp:
(WebCore::Layout::LineBuilder::placeInlineAndFloatContent):
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Dec 17, 2024 •
edited
Loading

EWS run on current version of this PR (hash 4fb2286)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2
✅ 🛠 tv ✅ 🛠 mac-safer-cpp
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@AlexsanderDamaceno
Copy link
Contributor Author

@nt1m hey, is all right for this one?

@nt1m nt1m requested a review from alanbaradlay December 18, 2024 01:31
alanbaradlay
alanbaradlay reviewed Dec 18, 2024
View reviewed changes
Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.cpp
// the last line before a forced break or the end of the block is start-aligned.
auto hasTextAlignJustify = (isLastInlineContent || m_line.runs().last().isLineBreak()) ? rootStyle.textAlignLast() == TextAlignLast::Justify : rootStyle.textAlign() == TextAlignMode::Justify;
if (hasTextAlignJustify) {
if (hasTextAlignJustify && rootStyle.textJustify() != TextJustify::None) {
Copy link
Contributor

Choose a reason for hiding this comment

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

according to the spec text-justify: none sets the justification opportunities to 0 (https://drafts.csswg.org/css-text/#text-justify-property). does this mean ruby align is impacted too as it aligns the base content using the number of justification opportunities? (https://drafts.csswg.org/css-ruby/#ruby-align-property)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for ruby align it should not apply ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alanbaradlay in this case we should verify if ruby-align is present and not skip justify, do you think this approach would be ok ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nt1m nt1m nt1m left review comments

@alanbaradlay alanbaradlay alanbaradlay left review comments

Assignees

@AlexsanderDamaceno AlexsanderDamaceno

Labels
CSS Cascading Style Sheets implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AlexsanderDamaceno @webkit-early-warning-system @nt1m @alanbaradlay @webkit-ews-buildbot