react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Implement baseline alignment function on the new architecture

Open j-piasecki opened this issue 1 year ago • 24 comments

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
baseline-android-old-arch baseline-android-new-arch-before Screenshot 2024-07-02 at 16 40 38

iOS

Old arch New arch before New arch after
baseline-ios-old-arch baseline-ios-new-arch-before Screenshot 2024-07-02 at 16 40 29

j-piasecki avatar Jun 21 '24 10:06 j-piasecki

/rebase

cortinico avatar Jun 21 '24 15:06 cortinico

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

cortinico avatar Jun 21 '24 15:06 cortinico

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: 8c8c77b974ce3d4c3036361e56276a6ff2b02208 Branch: main

analysis-bot avatar Jun 21 '24 21:06 analysis-bot

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.

NickGerleman avatar Jun 25 '24 09:06 NickGerleman

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

Screenshot 2024-06-25 at 16 30 06 Screenshot 2024-06-25 at 16 20 22

j-piasecki avatar Jun 25 '24 14:06 j-piasecki

@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 😅.

NickGerleman avatar Jun 26 '24 05:06 NickGerleman

I've updated the logic to calculate the baseline based on the first line. There's a small issue with padding and TextInputs - layoutMetrics are set too late, so I took them directly from yogaNode, I'm not sure if that's the correct approach.

Here's the result:

Screenshot 2024-06-27 at 15 36 44 Screenshot 2024-06-27 at 15 36 40
And the code
import { SafeAreaView, View, Text, TextInput } from 'react-native';

export default function App() {
  return (
    <SafeAreaView style={{flex: 1, flexDirection: 'row', alignItems: 'baseline'}}>
      <View style={{width: 50, height: 50, backgroundColor: 'red'}} />
      <View style={{width: 150, backgroundColor: 'red'}}>
        <Text style={{fontSize: 15}}>Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</Text>
      </View>
      <View style={{width: 150, backgroundColor: 'red'}}>
        <Text style={{fontSize: 11}}>Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.</Text>
      </View>
      <TextInput style={{padding: 0, paddingTop: 10, backgroundColor: 'red'}} value='Lorem ipsum' />
    </SafeAreaView>
  )
}

edit:

It looks like it broke the existing example: Screenshot 2024-06-27 at 16 13 40

j-piasecki avatar Jun 27 '24 13:06 j-piasecki

Using getContentWithMeasuredAttachments solved that problem on Android, it's still affecting iOS. There's also an issue of how views nested in text are processed - every view is effectively treated as having its baseline at the bottom, which results in this cascade when nesting: Screenshot 2024-06-28 at 16 06 57

I got it working okay-ish on Android by doing some wrong assumptions and hard-coding a few things: Screenshot 2024-06-28 at 16 10 13

I believe that it would be necessary to have every attachment also have a baseline property to fix that properly.

j-piasecki avatar Jun 28 '24 14:06 j-piasecki

I needed to update how measureLines works on iOS. Previously it was using data about the font used for the first character. This isn't correct when there are multiple spans styled differently on the same line, or when an inline view is present in the text.

I've changed it to use the y-position of the first character, which I believe is the baseline. Then I set the descender to lineRect.height - baseline, which should be equal to the descend if I understand the methods correctly.

This fixed the interleaving View and Text example in RNTester and I didn't see anything obviously broken either.

The one problem left is that the baseline is "cascading" when nesting multiple views and texts together
Screenshot 2024-07-01 at 14 26 20 Screenshot 2024-07-01 at 14 26 19
import React from "react";
import { Text, View, SafeAreaView } from "react-native";
export default class TextBaseLineLayoutExample extends React.Component {
  render() {
    const texts = [];
    for (let i = 9; i >= 0; i--) {
      texts.push(
        <Text key={i} style={{fontSize: 8 + i * 5, backgroundColor: '#eee'}}>
          {i}
        </Text>,
      );
    }

    const marker = (
      <View style={{width: 20, height: 20, backgroundColor: 'gray'}} />
    );

    return (
      <SafeAreaView>
        <View style={{flexDirection: 'row', alignItems: 'baseline', backgroundColor: 'red'}}>
          {marker}
          <Text style={{fontSize: 15}} selectable={true}>
            Text1

            <View
              style={{
                flexDirection: 'row',
                alignItems: 'baseline',
                backgroundColor: 'cyan',
              }}>
              <Text style={{fontSize: 20}}>Text2
                {marker}
                <View
                  style={{
                    flexDirection: 'row',
                    alignItems: 'baseline',
                    backgroundColor: 'yellow',
                  }}>
                  <Text style={{fontSize: 25}}>Text3
                  {marker}
                  <View
                    style={{
                      flexDirection: 'row',
                      alignItems: 'baseline',
                      backgroundColor: 'orange',
                    }}>
                      <Text style={{fontSize: 15}}>Text4
                      {marker}
                      </Text>
                    </View>
                  </Text>
                </View>
              </Text>
            </View>
          </Text>

          <View style={{ width: 30, height: 30, backgroundColor: 'gray'}} />
        </View>
      </SafeAreaView>
    );
  }
}

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.

And there's the matter of reading padding directly from the yogaNode in TextInputs.

j-piasecki avatar Jul 01 '24 14:07 j-piasecki

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

facebook-github-bot avatar Jul 03 '24 12:07 facebook-github-bot

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.

Sorry I missed this ping @NickGerleman. As I suspect this is more of a niche feature in React Native, it makes sense to align to the web spec and pick the first line in multiline string for the sake of alingment.

cortinico avatar Jul 03 '24 13:07 cortinico

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.

I'm fine with this particular scenario not being properly handled as I have the feeling it might just over-complicate the measuring logic. It also makes sense that the views are placed at the baseline, resulting in the "cascading" effect you mentioned @j-piasecki.

I'd like to hear @NickGerleman opinion on this one, but I believe we can attempt to land this as it is 👍

cortinico avatar Jul 03 '24 14:07 cortinico

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

facebook-github-bot avatar Jul 04 '24 12:07 facebook-github-bot

@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(
         ~~~~~~~~~~~~~~~~~~~~^

cortinico avatar Jul 04 '24 13:07 cortinico

That's weird, it looks like it's pointing here: https://github.com/facebook/react-native/blob/08c8d8dfffe2240c8a02f07527f03a21cd9c753b/packages/react-native/ReactCommon/react/renderer/components/text/ParagraphShadowNode.cpp#L188-L192 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?

j-piasecki avatar Jul 04 '24 13:07 j-piasecki

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 avatar Jul 04 '24 13:07 cortinico

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.

j-piasecki avatar Jul 04 '24 14:07 j-piasecki

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

facebook-github-bot avatar Jul 04 '24 14:07 facebook-github-bot

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 👍

cortinico avatar Jul 04 '24 14:07 cortinico

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.

  1. https://www.w3.org/TR/css-flexbox-1/#flex-baselines
  2. https://www.w3.org/TR/css-align-3/#baseline-export

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.

NickGerleman avatar Jul 05 '24 17:07 NickGerleman

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

facebook-github-bot avatar Jul 08 '24 10:07 facebook-github-bot

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/

j-piasecki avatar Jul 08 '24 12:07 j-piasecki

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 https://github.com/facebook/react-native/pull/45102#discussion_r1651258302

NickGerleman avatar Jul 08 '24 14:07 NickGerleman

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?

NickGerleman avatar Jul 08 '24 14:07 NickGerleman

@cortinico merged this pull request in facebook/react-native@2932c0f71f1882607d9e579e5c09db28e131a4c9.

facebook-github-bot avatar Jul 10 '24 19:07 facebook-github-bot

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

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

github-actions[bot] avatar Jul 10 '24 19:07 github-actions[bot]

why is something like this not possible still?

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

Aligning the box in the center.

SupertigerDev avatar Jul 29 '24 08:07 SupertigerDev