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.

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

Expose props for setting TextLineBounds and LineStackingStrategy #9218

Closed
aschultz opened this issue Nov 30, 2021 · 5 comments
Closed

Expose props for setting TextLineBounds and LineStackingStrategy #9218

aschultz opened this issue Nov 30, 2021 · 5 comments
Labels
Area: Layout Area: Text enhancement Needs: Dev Design Partner: Xbox (label may be applied by bot based on author) Recommend: Not Planned Recommend that issue should be given Not Planned milestone.
Milestone

Comments

@aschultz
Copy link
Member

Summary

XAML has two properties which control the bounding region and layout of each line of text: TextLineBounds and LineStackingStrategy.

Both of these properties should be exposed as React props on the <Text /> element to enable fine-tuning of text layout, such as when vertically centering text within a button. The default XAML values are extremely problematic.

Motivation

By default, XAML uses a TextLineBounds of Full, meaning that the bounding box includes ascenders and descenders. The height of ascenders/descenders relative to the font baseline can vary by font, and a run of text may or may not have ascenders or descenders. The result is that the position of the baseline relative to the bounding box can vary significantly and this makes it impossible to consistently positioning the line of text relative to other elements. Vertically positioning text within a button, for example, is an exercise in frustration as any change to the text or font can shift it up or down.

This article goes into detail about the same problem on web, which currently lacks the knobs XAML provides. It describes an active proposal to add new CSS properties like leading-trim to address the problem.

XAML has this functionality already available (set TextLineBounds="Tight"), we just need to expose it through React.

As for LineStackingStrategy... it normally defaults to MaxHeight, but RNW recently (as of 0.65) changed this to BlockLineHeight because of MaxHeight's poor handling of small line heights. This change has the side-effect of making multi-line text much more consistent (it will simply obey line height instead of shifting around based on the content). However, there may be cases where the other available values (MaxHeight and BaselineToBaseline) are useful. I suggest exposing it as well.

Basic Example

No response

Open Questions

Should this be exposed as an RNW-specific prop or should it align with the CSS leading-trim spec?

@ghost ghost added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Nov 30, 2021
@chrisglein chrisglein added Area: Text Partner: Xbox (label may be applied by bot based on author) labels Dec 2, 2021
@chrisglein
Copy link
Member

If there's a gap here RN-wide then we'd want to make sure whatever solution we come up with is communicated out so we can have alignment across RN platforms, even if we do something for Windows first.

@chrisglein chrisglein removed the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Jan 3, 2022
@chrisglein
Copy link
Member

@aschultz Do you have any workarounds you're employing in the meantime to get your text measurement exact? How blocked are you? Coming up with a solution here that aligns all of RN will take more time.

@chrisglein chrisglein added the Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) label Jan 3, 2022
@chrisglein chrisglein added this to the Backlog milestone Jan 3, 2022
@aschultz
Copy link
Member Author

aschultz commented Jan 4, 2022

AFAIK, it's not possible to work around this aside from patching the Text component to support the property. We have cases where devs manually added padding or margins to position text correctly, but they're extremely fragile and specific to each usage of a control.

Looking at RN's iOS code, I think an implementation is possible since they actually have full access to the font metrics and do the line height / offset calculations directly. Not sure about Android.

Related - There's an outstanding bug on RN that looks similar to this fix in RNW. So anyone diving into this on the RN side could probably knock both out at the same time and bring some consistency across platforms.

@ghost ghost added Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Jan 4, 2022
@chrisglein chrisglein added Area: Layout and removed Needs: Attention 👋 An issue that had been tagged "Needs: Author Feedback" has received activity (label applied by bot) labels Jan 6, 2022
@rozele
Copy link
Collaborator

rozele commented Jan 11, 2022

It's theoretically possible to do this outside of the core RN Text component using refs and findNodeHandle, but generally speaking findNodeHandle isn't supported very well (at all?) in Fabric. We use this pattern quite a bit where we do something like...

const textRef = useRef<Text>();
useEffect(() => {
  // call native module to do something to native component
  if (textRef.current) {
    MyCustomNativeModule.setExtraTextProperty(findNodeHandle(textRef.current));
  }
  
  return () => {
    // call native module to cleanup what was done to native component
   MyCustomNativeModule.cleanupExtraTextProperty(findNodeHandle(textRef.current));
  }
});
...
return (
  <Text ref={textRef} ...>...</Text>
)

For example, we've done this to build a custom native module that extends the ContextFlyout for TextBlock and TextBox ( and ) with extra options and also to add JS events for handling TextBox.Paste native events.

It's not ideal to do something like this for trimming / stacking strategy as you'd want these properties to be correct on initial rendering (to avoid re-rendering the text), but it could work.

In general, RN lacks APIs to extend the prop set / events for core components, however at least on iOS and Android, you theoretically have the capability to derive from core view managers and either replace the core implementations that include an extended property set or at least create another view manager that does not need to copy the code from the core version.

@rozele
Copy link
Collaborator

rozele commented Jan 11, 2022

That being said, it could be a good contribution to core, but I'm not sure how well it will work with the custom measure functions for text on iOS and Android (sounds like you might have already done some digging on iOS).

@marlenecota marlenecota added the Recommend: Not Planned Recommend that issue should be given Not Planned milestone. label Aug 29, 2023
@chrisglein chrisglein closed this as not planned Won't fix, can't repro, duplicate, stale Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Layout Area: Text enhancement Needs: Dev Design Partner: Xbox (label may be applied by bot based on author) Recommend: Not Planned Recommend that issue should be given Not Planned milestone.
Projects
None yet
Development

No branches or pull requests

4 participants