Implement baseline alignment function on the new architecture #45102

j-piasecki · 2024-06-21T10:54:45Z

Summary:

On the new architecture, the setup that would allow Yoga to read the baseline of a node was missing. This PR adds it:

adds new ShadowNode trait - BaselineYogaNode that marks a node as having a custom baseline function
adds yogaNodeBaselineCallbackConnector that's responsible for allowing Yoga to call baseline function on the node
changes signatures of lastBaseline and firstBaseline to accept LayoutContext as the first argument, which is necessary to build an attributed string
adds implementation of lastBaseline that's invoked by yogaNodeBaselineCallbackConnector
adds methods for calculating the last baseline in platform-specific TextLayoutManagers, using the same approach on both Android and iOS (this differs from the old architecture where calculations were different)

Changelog:

[GENERAL] [FIXED] - Fixed alignItems: 'baseline' not working correctly on the new architecture

Test Plan:

Tested on the relevant part of RNTester:

Android

Old arch	New arch before	New arch after

iOS

Old arch	New arch before	New arch after

cortinico · 2024-06-21T15:19:40Z

/rebase

cortinico · 2024-06-21T15:24:04Z

Autorebase failed as we're doing GHA work at the moment. @j-piasecki if you could rebase the CI should be green

analysis-bot · 2024-06-21T21:17:44Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	20,383,026	-902,098
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	23,579,561	-902,438
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 8c8c77b
Branch: main

NickGerleman

~~Could we add an RNTester example for this?~~ edit: missed we had the existing one shown in test plan

NickGerleman · 2024-06-24T15:39:59Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

+    Layout layout =
+        createLayout(context, attributedString, paragraphAttributes, width, height, null);


This will recreate AttributedString MappBuffer, Spannable, and relayout, which can be expensive. It would be great if we could reuse some of these, but I'm not sure this is nice to do with current interface, and needing to calculate baseline is not the most common case.

At some point in the future, I am looking to change this code a bit, so that we use PrecomputedText, which associates some glyph-level measurement information with the Spannable when we layout (so that later, the UI thread TextView does not need to relayout after Fabric layout, which can cause scroll jank). I haven't thought about how to organize this yet though.

Is there anything I should do regarding this?

For now, we can leave it. I have some ideas here (like riffing off the spannable IDs used in other places for reuse, or batching baseline and measure together), that I might try to take a stab at later.

Hmm, though I think the Fabric LineMeasurements would contain everything already needed to determine a baseline.

Right now I don't remember that we store line measurements against contraints (my guess it we don't), but it means baseline calculation could rely on information already retrieved most of the time, by measure.

Hmm, though I think the Fabric LineMeasurements would contain everything already needed to determine a baseline.

I'm pretty sure you're right

Right now I don't remember that we store line measurements against contraints (my guess it we don't), but it means baseline calculation could rely on information already retrieved most of the time, by measure.

It's not cached at the moment. Is this close to what you have in mind: 0ea302c?

NickGerleman · 2024-06-24T15:47:54Z

packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java

@@ -504,6 +504,20 @@ private NativeArray measureLines(
            PixelUtil.toPixelFromDIP(height));
  }

+  @SuppressWarnings("unused")
+  private float getLastBaseline(


Does Paper use last line? Spec kinda seems to suggest inline-flex (how RN paragraph mostly works) would align to first line of multiline context. https://www.w3.org/TR/css-inline-3/#baseline-source

Yes it does:

react-native/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/ReactTextShadowNode.java

Line 177 in 4a8f0ee

return layout.getLineBaseline(layout.getLineCount() - 1);

NickGerleman · 2024-06-24T15:48:26Z

...react-native/ReactAndroid/src/main/java/com/facebook/react/views/text/TextLayoutManager.java

+    float maxDescent = 0;
+    for (int i = 0; i < layout.getLineCount(); i++) {
+      if (maxDescent < layout.getLineDescent(i)) {
+        maxDescent = layout.getLineDescent(i);
+      }
+    }


Why do we need to loop through these? My assumption would be that we always want to use descent of the last line (or, first line, depending on above comment).

It's to align with the implementation used on iOS:

react-native/packages/react-native/Libraries/Text/Text/RCTTextShadowView.mm

Lines 359 to 368 in 4a8f0ee

[attributedText enumerateAttribute:NSFontAttributeName

inRange:NSMakeRange(0, attributedText.length)

options:NSAttributedStringEnumerationLongestEffectiveRangeNotRequired

usingBlock:^(UIFont *font, NSRange range, __unused BOOL *stop) {

if (maximumDescender > font.descender) {

maximumDescender = font.descender;

}

}];

return size.height + maximumDescender;

The naming is confusing me here a little bit. I am wondering if these are doing the same things.

A "descender" is part of text that goes under the baseline. So, getting lowest descender, is I think trying to get the bottom of the text. Though, this isn't really the baseline.

But a "descent" usually is the distance between the baseline and the bottom of the line-box. StackOverflow suggests that, this value on Android layout does not have any sort of per-line offset. https://stackoverflow.com/a/43691403

I think the logic works for single-line, but I don't think the Android logic will work correctly for multiline containers.

My bet is that iOS might have been using largest font, to calculate offset from bottom, to approximate the last line height, that we have the direct access to on Android.

There is a difference though, when padding is applied to TextInput. It's included in height but the Java part is unaware of it.

NickGerleman · 2024-06-24T15:54:07Z

packages/react-native/ReactCommon/react/renderer/core/LayoutableShadowNode.h

-  virtual Float firstBaseline(Size size) const;
-  virtual Float lastBaseline(Size size) const;
+  virtual Float firstBaseline(const LayoutContext& layoutContext, Size size) const;
+  virtual Float lastBaseline(const LayoutContext& layoutContext, Size size) const;


It's existing code, but I think this could potentially just be a single function, since rules around multiple baselines, and which to pick, is text-specific concept/implementation detail. If we only give a single baseline function to Yoga, having one baseline function to override which isn't called could be confusing.

Do you want me to remove the firstBaseline?

I had in mind replacing both with a single baseline function, where line selection would be a detail of it.

Done - f576413

NickGerleman · 2024-06-25T09:29:26Z

The way Paper is handling multi-line baseline selection in flex containers seems inconsistent with browsers/flexbox spec. In browser, first-line is chosen. https://jsfiddle.net/jq49r8hb/

I suspect we could get away with changing the behavior in RN to align with the spec and browsers. But I'm curious what @cortinico thinks about this.

j-piasecki · 2024-06-25T14:25:22Z

I've also updated the relevant Android part of RNTester to match what's on iOS:

NickGerleman · 2024-06-26T05:16:09Z

@j-piasecki I think this is looking good. Have we tested/do we have validation for multi-line?

I still kinda think we should switch Paper behavior for web behavior though, now that we have the chance to 😅.

facebook-github-bot · 2024-07-04T12:23:45Z

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cortinico · 2024-07-04T13:24:09Z

@j-piasecki this is failing to build internally with:

[...]\packages\react-native\ReactCommon\react\renderer\components\text\ParagraphShadowNode.cpp(188,30): error: no member named 'getLastBaseline' in 'facebook::react::TextLayoutManager'
  return textLayoutManager_->getLastBaseline(
         ~~~~~~~~~~~~~~~~~~~~^

j-piasecki · 2024-07-04T13:30:32Z

That's weird, it looks like it's pointing here:

react-native/packages/react-native/ReactCommon/react/renderer/components/text/ParagraphShadowNode.cpp

Lines 188 to 192 in 08c8d8d

    
           return textLayoutManager_ 
        
               ->baseline( 
        
                   attributedString, 
        
                   getConcreteProps().paragraphAttributes, 
        
                   size);

but I already updated it. Moreover, there are no references to getLastBaseline at all in the codebase at the moment. Is it possible that it's caused by a stale cache?

cortinico · 2024-07-04T13:39:26Z

Moreover, there are no references to getLastBaseline at all in the codebase at the moment. Is it possible that it's caused by a stale cache?

@j-piasecki my bad I copied the error message form a old revision.

The error is still:

[...]\packages\react-native\ReactCommon\react\renderer\components\text\ParagraphShadowNode.cpp(188,30): error: no member named 'baseline' in 'facebook::react::TextLayoutManager'
  return textLayoutManager_->baseline(
         ~~~~~~~~~~~~~~~~~~~~^

cortinico · 2024-07-04T13:43:09Z

...erer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm

@@ -303,12 +305,12 @@ - (TextMeasurement)_measureTextStorage:(NSTextStorage *)textStorage

                CGSize attachmentSize = attachment.bounds.size;
                CGRect glyphRect = [layoutManager boundingRectForGlyphRange:range inTextContainer:textContainer];
-
+                CGFloat baseline = [layoutManager locationForGlyphAtIndex:range.location].y;
                UIFont *font = [textStorage attribute:NSFontAttributeName atIndex:range.location effectiveRange:nil];


This variable is unused now and is causing various iOS builds to fail (we do have -Wall everywhere)

j-piasecki · 2024-07-04T14:22:48Z

The error is still:
[...]\packages\react-native\ReactCommon\react\renderer\components\text\ParagraphShadowNode.cpp(188,30): error: no member named 'baseline' in 'facebook::react::TextLayoutManager' return textLayoutManager_->baseline( ~~~~~~~~~~~~~~~~~~~~^

I don't see why it would fail there. All TextLayoutManager headers (for ios, android, and cxx) have the baseline method declared in the public scope and all of them have the implementation.

One thing I noticed is that I was using float type instead of Float. I think the error message would be different if that was the cause, but I changed it nonetheless.

facebook-github-bot · 2024-07-04T14:32:02Z

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cortinico · 2024-07-04T14:57:26Z

I don't see why it would fail there. All TextLayoutManager headers (for ios, android, and cxx) have the baseline method declared in the public scope and all of them have the implementation.

Ok so the issue is that we do have other TextLayoutManager in our codebase that are not exposed to GitHub. I'll take care of providing a baseline function there and returning 0 so that should unblock us 👍

NickGerleman · 2024-07-05T17:33:05Z

The one problem left is that the baseline is "cascading" when nesting multiple views and texts together

As mentioned above, this is caused by every attachment being positioned on the baseline, so views containing text are effectively moved up by the descent. This one might be tricky to solve - I think it would need to align with the last baseline of the attached view.

@j-piasecki Sorry, I missed reading this in the last iteration.

In an inline/text context, items are by default aligned so that their baselines mare matched. I think the "cascading" problem is an artifact of always treating the baseline of the inline view as its bottom.

A flexbox container's baseline is set based on its children. So, the baseline of that blue "View", should really match the baseline of the text inside of it.

Yoga does implement this algorithm, for determining the baseline of a child when using align-items: baseline, but React Native's text layout doesn't do anything similar for positioning attachments, so nothing will be seen for that. https://github.com/facebook/yoga/blob/5009f5c1accb3dde14e1e18930df70c18c70dc75/yoga/algorithm/Baseline.cpp#L17

Not something for this PR, but would definitely be welcome as followup if you ever feel like doing more around this area.

...erer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm

facebook-github-bot · 2024-07-08T10:09:39Z

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

j-piasecki · 2024-07-08T12:32:04Z

In an inline/text context, items are by default aligned so that their baselines mare matched. I think the "cascading" problem is an artifact of always treating the baseline of the inline view as its bottom.

That's exactly what's happening.

Yoga does implement this algorithm, for determining the baseline of a child when using align-items: baseline, but React Native's text layout doesn't do anything similar for positioning attachments, so nothing will be seen for that. https://github.com/facebook/yoga/blob/5009f5c1accb3dde14e1e18930df70c18c70dc75/yoga/algorithm/Baseline.cpp#L17

Not something for this PR, but would definitely be welcome as followup if you ever feel like doing more around this area.

Sure, I would be up for it, though I don't think it would be contained to the React Native's part of text layout. Every inline view is passed as an attachment which is essentially a rectangle without any information about it's children or baseline.

Moreover, it looks like inline views are aligned on the last baseline instead of the first one: https://jsfiddle.net/tgkdr63s/

NickGerleman · 2024-07-08T14:11:18Z

Moreover, it looks like inline views are aligned on the last baseline instead of the first one: https://jsfiddle.net/tgkdr63s/

If you change inline-block to inline-flex this changes (baseline determination is different for flex containers vs block containers) See #45102 (comment)

NickGerleman · 2024-07-08T14:19:19Z

From https://jsfiddle.net/jq49r8hb/ I think the first baseline behavior will also still happen when aligning inline-block element in flex container too. I think the last baseline behavior may be specific to inline-block inside block context?

facebook-github-bot · 2024-07-10T19:49:02Z

@cortinico merged this pull request in 2932c0f.

github-actions · 2024-07-10T19:49:04Z

This pull request was successfully merged by @j-piasecki in 2932c0f.

^{When will my fix make it into a release? | How to file a pick request?}

SupertigerDev · 2024-07-29T08:14:04Z

why is something like this not possible still?

https://jsfiddle.net/yhc43e9w/1/

Aligning the box in the center.

Summary: In #45102 I've implemented a baseline alignment function for the new architecture. I've noticed one thing I've missed previously - `locationForGlyphAtIndex` is [relative to the line fragment](https://developer.apple.com/documentation/appkit/nslayoutmanager/1403239-locationforglyphatindex), not the container. This means that the attachments would be put in the wrong place in multiline text. This PR fixes that by adding the position of the entire line to the attachment position. ## Changelog: [IOS] [FIXED] - Fixed baseline attachment position in multiline text Pull Request resolved: #46172 Test Plan: Checked on relevant example of RNTester. |Before|After| |-|-| |<img width="546" alt="Screenshot 2024-08-22 at 15 53 14" src="https://github.com/user-attachments/assets/c1861655-9253-44fd-9f2f-796aff83df1e">|<img width="546" alt="Screenshot 2024-08-22 at 15 52 37" src="https://github.com/user-attachments/assets/27f07bc7-a8f6-4696-a414-815e7ece642c">| Reviewed By: andrewdacenko Differential Revision: D61662006 Pulled By: cipolleschi fbshipit-source-id: 5eafdae1800c06d9fc61bfac99584e6e25a05c24

j-piasecki added 11 commits June 21, 2024 22:07

Setup baseline func

eaa1752

Add context

ce3d20b

Implement iOS

0f61afb

Implement Android

346b0e1

Extract the common code

b29ab4a

Use correct method name

9d7f269

Use lastBaseline method

c8c08c6

Restore format

07f5ffe

Rename baseline getter

62931c9

Adjust indent

4aa7b46

Remove duplicate method

00ccf02

j-piasecki force-pushed the @jpiasecki/baseline-func-new-arch branch from 763988c to 00ccf02 Compare June 21, 2024 20:43

NickGerleman reviewed Jun 24, 2024

View reviewed changes

j-piasecki added 3 commits June 25, 2024 15:39

Unify baseline functions

f576413

Align RNTester examples

0a0d4b5

Remove interleaving

57b7fbe

I removed too much

bf798be

j-piasecki added 3 commits June 26, 2024 14:33

Implement baseline on top of measureLines

0ea302c

Remove getLastBaseline fromUnify baseline calculations

c29777c

Use first line as baseline

02b109d

j-piasecki added 3 commits July 4, 2024 10:45

getLastBaseline -> baseline

3fca450

Don't copy string multiple times

c6ac596

Inline doMeasureLines

08c8d8d

cortinico reviewed Jul 4, 2024

View reviewed changes

j-piasecki added 2 commits July 4, 2024 15:45

Remove unused font variable

1235a96

Use Float instead of float

3b97820

NickGerleman reviewed Jul 5, 2024

View reviewed changes

...erer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm Outdated Show resolved Hide resolved

...erer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm Show resolved Hide resolved

Use line fragment rect

828deaa

facebook-github-bot closed this in 2932c0f Jul 10, 2024

facebook-github-bot added the Merged This PR has been merged. label Jul 10, 2024

This was referenced Aug 15, 2024

Implement baseline alignment on Fabric microsoft/react-native-windows#13580

Open

Integrate RN Nightly 7/19 microsoft/react-native-windows#13554

Merged

j-piasecki mentioned this pull request Aug 22, 2024

Add line offset for baseline alignment of text attachments #46172

Closed

		Layout layout =
		createLayout(context, attributedString, paragraphAttributes, width, height, null);

	[attributedText enumerateAttribute:NSFontAttributeName
	inRange:NSMakeRange(0, attributedText.length)
	options:NSAttributedStringEnumerationLongestEffectiveRangeNotRequired
	usingBlock:^(UIFont font, NSRange range, __unused BOOL stop) {
	if (maximumDescender > font.descender) {
	maximumDescender = font.descender;
	}
	}];

	return size.height + maximumDescender;

Implement baseline alignment function on the new architecture #45102

Implement baseline alignment function on the new architecture #45102

Conversation

j-piasecki commented Jun 21, 2024 • edited Loading

Summary:

Changelog:

Test Plan:

Android

iOS

cortinico commented Jun 21, 2024

cortinico commented Jun 21, 2024

analysis-bot commented Jun 21, 2024 • edited Loading

NickGerleman left a comment • edited Loading

Choose a reason for hiding this comment

NickGerleman Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickGerleman commented Jun 25, 2024 • edited Loading

j-piasecki commented Jun 25, 2024 • edited Loading

NickGerleman commented Jun 26, 2024

facebook-github-bot commented Jul 4, 2024

cortinico commented Jul 4, 2024

j-piasecki commented Jul 4, 2024

cortinico commented Jul 4, 2024

Choose a reason for hiding this comment

j-piasecki commented Jul 4, 2024

facebook-github-bot commented Jul 4, 2024

cortinico commented Jul 4, 2024

NickGerleman commented Jul 5, 2024 • edited Loading

facebook-github-bot commented Jul 8, 2024

j-piasecki commented Jul 8, 2024

NickGerleman commented Jul 8, 2024

NickGerleman commented Jul 8, 2024

facebook-github-bot commented Jul 10, 2024

github-actions bot commented Jul 10, 2024

SupertigerDev commented Jul 29, 2024 • edited Loading

j-piasecki commented Jun 21, 2024 •

edited

Loading

analysis-bot commented Jun 21, 2024 •

edited

Loading

NickGerleman left a comment •

edited

Loading

NickGerleman Jun 24, 2024 •

edited

Loading

NickGerleman commented Jun 25, 2024 •

edited

Loading

j-piasecki commented Jun 25, 2024 •

edited

Loading

NickGerleman commented Jul 5, 2024 •

edited

Loading

SupertigerDev commented Jul 29, 2024 •

edited

Loading