Skip to content

Conversation

vitorroriz
Copy link
Contributor

@vitorroriz vitorroriz commented Jan 30, 2024

c1353df

text-wrap balance should consider line-clamp when balancing
https://bugs.webkit.org/show_bug.cgi?id=268302
rdar://121858978

Reviewed by Alan Baradlay.

According to spec resolution [1], if line-clamp
is defined, text-wrap: balance should balance
only within the clamped lines.

Up to this patch, we would balance taking into
consideration all the lines and we would clamp
it after balance.

This patches makes InlineContentBalancer::initialize()
take the maximum number of visible lines into account,
based into the line-clamp property.

Also, this allows for a small optimization:
If line-clamp clamps to 1 line, we can skip balacing.

[1] w3c/csswg-drafts#9310

* LayoutTests/TestExpectations:
* Source/WebCore/layout/formattingContexts/block/BlockLayoutState.h:
(WebCore::Layout::BlockLayoutState::LineClamp::allowedLineCount const):
* Source/WebCore/layout/formattingContexts/inline/InlineContentBalancer.cpp:
(WebCore::Layout::InlineContentBalancer::initialize):
(WebCore::Layout::InlineContentBalancer::computeBalanceConstraints):
* Source/WebCore/layout/formattingContexts/inline/InlineContentBalancer.h:
* Source/WebCore/layout/formattingContexts/inline/InlineFormattingContext.cpp:
(WebCore::Layout::InlineFormattingContext::createDisplayContentForInlineContent):

Canonical link: https://commits.webkit.org/273800@main

867f21f

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

@vitorroriz vitorroriz self-assigned this Jan 30, 2024
@vitorroriz vitorroriz added the CSS Cascading Style Sheets implementation label Jan 30, 2024
@vitorroriz vitorroriz force-pushed the text-wrap-balance-line-clamp branch from b4d42f2 to 11e865f Compare January 30, 2024 01:03
Comment on lines 500 to 505
std::optional<size_t> InlineFormattingContext::numberOfVisibleLinesAllowed() const
{
if (auto lineClamp = layoutState().parentBlockLayoutState().lineClamp())
return lineClamp->maximumLineCount > lineClamp->currentLineCount ? lineClamp->maximumLineCount - lineClamp->currentLineCount : 0;
return { };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason why this has to be on InlineFormattingContext now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just that this is already calculated at InlineFormattingContext::createDisplayContentForInlineContent, so I wanted to reuse it (Which I forgot to do in this patch).

Copy link
Contributor

Choose a reason for hiding this comment

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

please don't. InlineFormattingContext's API should stay focused. if you are interested in sharing code, move it to a helper function to the util class but I don't think it's worth the effort.

Comment on lines 114 to 117
if (!m_inlineFormattingContext.layoutState().placedFloats().isEmpty() || numberOfVisibleLinesAllowed == 1) {
m_cannotBalanceContent = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is quite misleading and somewhat error prone. numberOfVisibleLinesAllowed == 1 means we should not balance which is very different from cannot. -even if InlineContentBalancer currently ends up doing the same for both cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I've almost changed the name to shouldNotBalanceContent. I guess I'll do it. I assume that "should not" also makes sense for the case we just to avoid balancing for floats until it is unsupported.

@@ -62,6 +62,7 @@ class InlineFormattingContext {
std::pair<LayoutUnit, LayoutUnit> minimumMaximumContentSize(const InlineDamage* = nullptr);
LayoutUnit minimumContentSize(const InlineDamage* = nullptr);
LayoutUnit maximumContentSize(const InlineDamage* = nullptr);
std::optional<size_t> numberOfVisibleLinesAllowed() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should somehow reference to line-clamp to make it clear where this constrain comes from

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 30, 2024
@vitorroriz vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Jan 30, 2024
@vitorroriz vitorroriz changed the title text-wrap balance should consider line-clamp when balacing text-wrap balance should consider line-clamp when balancing Jan 30, 2024
@vitorroriz vitorroriz force-pushed the text-wrap-balance-line-clamp branch from 11e865f to 229d01e Compare January 30, 2024 05:24
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 229d01e. Live statuses available at the PR page, #23479

@@ -569,6 +569,13 @@ const InlineLayoutState& InlineFormattingUtils::layoutState() const
return formattingContext().layoutState();
}

std::optional<size_t> InlineFormattingUtils::numberOfVisibleLinesAfterLineClamping() const
Copy link
Contributor

Choose a reason for hiding this comment

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

what does "after line clamping" mean here?

Copy link
Contributor Author

@vitorroriz vitorroriz Jan 30, 2024

Choose a reason for hiding this comment

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

Hmmm, yeah, maybe it's not the best name. How about "numberOfVisibleLinesConsideringLineClamping"? I'm also open for suggestions of course!

@vitorroriz vitorroriz force-pushed the text-wrap-balance-line-clamp branch from 229d01e to 09d86a1 Compare January 30, 2024 17:55
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for 09d86a1. Live statuses available at the PR page, #23479

auto numberOfVisibleLinesAllowed = [&] () -> std::optional<size_t> {
if (auto lineClamp = layoutState().parentBlockLayoutState().lineClamp())
return lineClamp->maximumLineCount > lineClamp->currentLineCount ? lineClamp->maximumLineCount - lineClamp->currentLineCount : 0;
return { };
}();
auto numberOfVisibleLinesConsideringLineClamp = formattingUtils().numberOfVisibleLinesConsideringLineClamp();
Copy link
Contributor

Choose a reason for hiding this comment

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

diff --git a/Source/WebCore/layout/formattingContexts/block/BlockLayoutState.h b/Source/WebCore/layout/formattingContexts/block/BlockLayoutState.h
index b3eb317d69f1..f59cdee0b941 100644
--- a/Source/WebCore/layout/formattingContexts/block/BlockLayoutState.h
+++ b/Source/WebCore/layout/formattingContexts/block/BlockLayoutState.h
@@ -37,6 +37,8 @@ class BlockFormattingContext;
 class BlockLayoutState {
 public:
     struct LineClamp {
+        size_t allowedLineCount() const { return std::max(maximumLineCount - currentLineCount, 0lu); }
+
         size_t maximumLineCount { 0 };
         size_t currentLineCount { 0 };
     };
diff --git a/Source/WebCore/layout/formattingContexts/inline/InlineFormattingContext.cpp b/Source/WebCore/layout/formattingContexts/inline/InlineFormattingContext.cpp
index 23a98ec5d724..df2a13e72dd6 100644
--- a/Source/WebCore/layout/formattingContexts/inline/InlineFormattingContext.cpp
+++ b/Source/WebCore/layout/formattingContexts/inline/InlineFormattingContext.cpp
@@ -345,11 +345,7 @@ void InlineFormattingContext::updateBoxGeometryForPlacedFloats(const LineLayoutR
 
 InlineRect InlineFormattingContext::createDisplayContentForInlineContent(const LineBox& lineBox, const LineLayoutResult& lineLayoutResult, const ConstraintsForInlineContent& constraints, InlineDisplay::Content& displayContent, size_t numberOfPreviousLinesWithInlineContent)
 {
-    auto numberOfVisibleLinesAllowed = [&] () -> std::optional<size_t> {
-        if (auto lineClamp = layoutState().parentBlockLayoutState().lineClamp())
-            return lineClamp->maximumLineCount > lineClamp->currentLineCount ? lineClamp->maximumLineCount - lineClamp->currentLineCount : 0;
-        return { };
-    }();
+    auto numberOfVisibleLinesAllowed = layoutState().parentBlockLayoutState().lineClamp() ? std::make_optional(layoutState().parentBlockLayoutState().lineClamp()->allowedLineCount()) : std::nullopt;

Maybe something like this would work and then you don't have to think about name at all (no util function is needed)

Comment on lines 114 to 124
if (!m_inlineFormattingContext.layoutState().placedFloats().isEmpty() || numberOfVisibleLinesConsideringLineClamp == 1) {
m_shouldNotBalanceContent = true;
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

What I really meant here was that there's a difference between cases when we bail out due to unable to balance (floats are present) and cases when there's only one line and we can take a fast path. InlineContentBalancer may not care about the difference yet but I think it would make the code read better if we decoupled these 2.

Comment on lines 161 to 162
if (numberOfVisibleLinesConsideringLineClamp && lineIndex >= numberOfVisibleLinesConsideringLineClamp)
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can bail out a lot earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't we have to initialize the Balancer until we would reach the last line before clamping?

Copy link
Contributor

Choose a reason for hiding this comment

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

when you say Balancer are you referring to InlineContentBalancer? -just wanna make sure we are talking about the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly!

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, yes we have to initialize the InlineContentBalancer, but we are inside InlineContentBalancer. What I was trying to say with my original comment was please look and make sure we don't compute anything redundant here (i.e. perhaps moving this check a few lines above? -maybe "a lot earlier" was a bit of an exaggeration)

@vitorroriz vitorroriz force-pushed the text-wrap-balance-line-clamp branch from 09d86a1 to c0b6d04 Compare January 31, 2024 00:09
@webkit-early-warning-system
Copy link
Collaborator

Starting EWS tests for c0b6d04. Live statuses available at the PR page, #23479

auto numberOfVisibleLinesAllowed = [&] () -> std::optional<size_t> {
if (auto lineClamp = layoutState().parentBlockLayoutState().lineClamp())
return lineClamp->maximumLineCount > lineClamp->currentLineCount ? lineClamp->maximumLineCount - lineClamp->currentLineCount : 0;
return { };
}();
auto numberOfVisibleLinesConsideringLineClamp = layoutState().parentBlockLayoutState().lineClamp() ? std::make_optional(layoutState().parentBlockLayoutState().lineClamp()->allowedLineCount()) : std::nullopt;
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mind keeping the current variable name? While it was not great as a function name (no context in the header) I think it's fine as a variable name here.

@vitorroriz vitorroriz force-pushed the text-wrap-balance-line-clamp branch from c0b6d04 to 867f21f Compare January 31, 2024 00:44
@vitorroriz vitorroriz added the merge-queue Applied to send a pull request to merge-queue label Jan 31, 2024
https://bugs.webkit.org/show_bug.cgi?id=268302
rdar://121858978

Reviewed by Alan Baradlay.

According to spec resolution [1], if line-clamp
is defined, text-wrap: balance should balance
only within the clamped lines.

Up to this patch, we would balance taking into
consideration all the lines and we would clamp
it after balance.

This patches makes InlineContentBalancer::initialize()
take the maximum number of visible lines into account,
based into the line-clamp property.

Also, this allows for a small optimization:
If line-clamp clamps to 1 line, we can skip balacing.

[1] w3c/csswg-drafts#9310

* LayoutTests/TestExpectations:
* Source/WebCore/layout/formattingContexts/block/BlockLayoutState.h:
(WebCore::Layout::BlockLayoutState::LineClamp::allowedLineCount const):
* Source/WebCore/layout/formattingContexts/inline/InlineContentBalancer.cpp:
(WebCore::Layout::InlineContentBalancer::initialize):
(WebCore::Layout::InlineContentBalancer::computeBalanceConstraints):
* Source/WebCore/layout/formattingContexts/inline/InlineContentBalancer.h:
* Source/WebCore/layout/formattingContexts/inline/InlineFormattingContext.cpp:
(WebCore::Layout::InlineFormattingContext::createDisplayContentForInlineContent):

Canonical link: https://commits.webkit.org/273800@main
@webkit-commit-queue webkit-commit-queue force-pushed the text-wrap-balance-line-clamp branch from 867f21f to c1353df Compare January 31, 2024 01:28
@webkit-commit-queue webkit-commit-queue merged commit c1353df into WebKit:main Jan 31, 2024
@webkit-commit-queue
Copy link
Collaborator

Committed 273800@main (c1353df): https://commits.webkit.org/273800@main

Reviewed commits have been landed. Closing PR #23479 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jan 31, 2024
@rkirsling
Copy link
Member

Looks like this patch doesn't build on Windows.

@alanbaradlay
Copy link
Contributor

Looks like this patch doesn't build on Windows.

revert?

@vitorroriz
Copy link
Contributor Author

Looks like this patch doesn't build on Windows.

revert?

Let me see...

@vitorroriz
Copy link
Contributor Author

Interesting, 09d86a1 version of this patch was building for Windows, so I haven't paid much attention on that on the new iteration. Can you, please, provide me the build error you are seeing @rkirsling ?

@vitorroriz
Copy link
Contributor Author

Ops, nevermind it. I found it on the windows pipeline.

@vitorroriz
Copy link
Contributor Author

#23560

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Cascading Style Sheets implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants