feat(Text): introduce dir attribute to support RTL #5

kuzhelov · 2018-07-24T09:50:11Z

This proposal is aimed on introducing proper text rendering in RTL mode. Its idea consists of two points:

introduce dir="auto" attribute value to the text rendered by Text component
from now on ensure that all components that are about rendering text information as part of their presentation should use Text component to wrap this textual part

levithomason · 2018-07-24T16:26:10Z

As another option, could we just make dir=auto a default prop for all components? From a usability perspective, it would be ideal if we didn't require every string to use the Text component.

brianespinosa · 2018-07-24T16:58:25Z

As another option, could we just make dir=auto a default prop for all components? From a usability perspective, it would be ideal if we didn't require every string to use the Text component.

Yes. And lots of additional markup just to have RTL support in other components.

If we add dir prop to all components, it might also make sense to later make some sort of RTLProvider component that uses the context API where we can define the dir in the provider and all components will now have access to it.

levithomason · 2018-07-25T04:44:19Z

If we add dir prop to all components, it might also make sense to later make some sort of RTLProvider component that uses the context API where we can define the dir in the provider and all components will now have access to it.

Indeed, we actually have an rtl prop right now in the Provider itself. It determines whether the regular CSS in JS renderer is used or the RTL CSS in JS renderer.

kuzhelov · 2018-07-25T06:14:40Z

using Text (and, what is more important, dir="auto") is important for mainly components that contain text information - especially in those cases where this text is bidirectional (bidi). It won't be enough just to setup RTL Fela renderer for these cases.

jurokapsiar · 2018-07-25T12:41:46Z

Levi, do I understand correctly that after you proposed change, the dir='auto' would appear on each DOM rendered by the UIComponent? I don't think we want that. Sites typically use dir='rtl' on the <html> or <body> element to provide the default direction of the page. This will cause all elements to flip from left to right (typical example is a left-side pane becoming right-side, switching order of ok/cancel buttons and so on). On the elements that contain text we actually want to use dir='auto' which will then tell the browser to detect the direction based on the text itself.

levithomason · 2018-07-25T17:06:35Z

Can we define what "elements that contain text" means exactly?

Just Text

<span>Text only</span>

This element has text only, no child elements.

Text > empty inline child

<span>Text only with <i class="an icon"></i></span>

This element has text, but also has a i element which is common for icons.

Text > inline child > text

<span>Text only with <span>more text</span>.

This element has text, but also has a child element which contains only text.

Text > button child > text

<span>Text only with <button>technically text?</button>.

Are buttons also considered to be text?

Nearly every component contains text as some descendant. Where exactly should we be adding the dir attribute for proper accessibility in the tree?

jurokapsiar · 2018-08-02T02:22:25Z

typically if it is a combination of text+icon, the dir='rtl/ltr' on body takes care of properly setting the order of these two. dir='auto'`` should be used for text nodes:

If you don't know the direction of the phrase
When text will be added at run time to your HTML page you may not be able to predict the base direction of the injected text in advance. To handle this eventuality you have two options.

If the phrase is tightly wrapped by an element already, you could just add dir="auto" to that element. This directionally isolates the element's text and looks at the first strong character to determine what base direction to apply.

Otherwise, wrap the phrase in a bdi element (or in a span element with dir set to auto, if you prefer.) Without a dir attribute, the bdi element behaves as if dir="auto" had been applied.

https://www.w3.org/International/articles/inline-bidi-markup/

Text inside of the button might need dir='auto' for example in cases where you have team names as button texts. In that case the direction of the text itself need to be different depending on the content language.

Text inside of the button can be bidirectional, for example "Join team " where Join team would be translated to Hebrew/Arabic but the team name would be english. We are however not solving this case in this PR - most typically these texts come from parametrized translations, so that would be an advanced use case for our i18n story.

levithomason · 2018-08-02T02:44:13Z

The concern I have had with this PR is that it seems to conflate the styling purpose of the Text component with the RTL concerns. The Text component is about styling text. RTL to me seems to encompass more than just text and therefore perhaps its own mechanism or component.

Text inside of the button might need dir='auto' for example in cases where you have team names as button texts. In that case the direction of the text itself need to be different depending on the content language.

Would the markup then look like this?:

<button dir='auto'>
  Join Team
</button>

If we introduce the changes proposed in the PR, we'd end up with this:

<button>
  <span dir='auto'>Join Team</span>
</button>

jurokapsiar · 2018-08-02T07:03:19Z

From RTL point of view both examples are equal. However, the second one renders one more element which is unnecessary. Is there a generic way to achieve 2? And for that case, please note that we will also need to support

<button><span dir='auto'>Join</span><span dir='auto'>{team name in arabic/hebrew}</span><button>

levithomason · 2018-11-16T17:10:32Z

@jurokapsiar can you please post the final update here on this PR so we can implement it? Thanks...

jurokapsiar · 2018-11-19T12:47:31Z

In general, we need to get to a state where the text nodes are inside of elements that have dir='auto' attribute. This allows the browser to detect the text direction.

If the text is composed of multiple parts (typically a translation with parameters), each of the parts needs to be separated in <span dir='auto'> element. This is however out of scope for Stardust and should be handled in the i18n layer of the application.

Requirements:

use Text component to render any text inside of standard Stardust components
Text component renders <span dir='auto>{content}</span> if the type of content is a string
if user provides HTML/React nodes as an input for the component content, the user is responsible for adding dir='auto' by himself. This needs to be documented
revisit applying of truncateStyle in buttonStyles.ts - it needs to be applied even if the content contains <span> or other elements

codecov · 2018-11-21T14:55:11Z

Codecov Report

Merging #5 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master       #5      +/-   ##
==========================================
+ Coverage    88.3%   88.32%   +0.01%     
==========================================
  Files          42       42              
  Lines        1454     1456       +2     
  Branches      186      212      +26     
==========================================
+ Hits         1284     1286       +2     
  Misses        165      165              
  Partials        5        5

Impacted Files	Coverage Δ
src/components/Text/Text.tsx	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14ea899...6cf6ac2. Read the comment docs.

kuzhelov · 2018-11-23T09:41:53Z

button styles are left for the separate PR as this is the issue not directly related to this PR. Also, we need the following PR to be merged first to ensure that styles will be applied to children Text: #496

Dedicated issue for Button styles is created: #520 .

miroslavstastny · 2018-11-23T12:34:28Z

src/components/Text/Text.tsx

+
+    const hasChidlren = childrenExist(children)
+
+    const maybeDirAuto = !hasChidlren && typeof content === 'string' ? { dir: 'auto' } : null


hasChidlren -> hasChildren

introduce dir attribute to support proper rtl text rendering

93ce45b

kuzhelov requested review from dvdzkwsk and levithomason as code owners July 24, 2018 09:50

kuzhelov added the needs author feedback Author's opinion is asked label Aug 22, 2018

levithomason added this to kuzhelov in Core Team Nov 16, 2018

Merge branch 'master' into rfc/text-rtl-support

4850234

alinais requested review from miroslavstastny and mnajdova as code owners November 21, 2018 14:47

kuzhelov and others added 4 commits November 22, 2018 16:57

Merge branch 'master' into rfc/text-rtl-support

cc71e59

update approach suggested

4ca1c77

address comments

94e885e

update changelog

9bd7da5

kuzhelov added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Nov 23, 2018

jurokapsiar approved these changes Nov 23, 2018

View reviewed changes

sophieH29 approved these changes Nov 23, 2018

View reviewed changes

miroslavstastny reviewed Nov 23, 2018

View reviewed changes

introduce necessary description for RTL aspects

fb1dded

miroslavstastny approved these changes Nov 23, 2018

View reviewed changes

Merge branch 'master' into rfc/text-rtl-support

6cf6ac2

kuzhelov changed the title ~~[RFC] feat(Text): introduce dir attribute to support RTL~~ feat(Text): introduce dir attribute to support RTL Nov 23, 2018

kuzhelov merged commit f190e0b into master Nov 23, 2018

kuzhelov deleted the rfc/text-rtl-support branch November 23, 2018 13:32

jurokapsiar mentioned this pull request Dec 11, 2018

RTL text rendering in components #594

Closed

3 tasks

mnajdova mentioned this pull request Jan 11, 2019

feat(rtl): add span element with dir: 'auto' for the strings used in the Stardust components #704

Merged

This was referenced Mar 11, 2020

Adding 7 new icons microsoft/fluentui#12212

Merged

Updated 'raise hand' icons after bug bash comments microsoft/fluentui#12342

Merged

DustyTheBot mentioned this pull request Nov 16, 2021

fix color for dropdown checkable indicator in hc theme microsoft/fluentui#20621

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Text): introduce dir attribute to support RTL #5

feat(Text): introduce dir attribute to support RTL #5

kuzhelov commented Jul 24, 2018

levithomason commented Jul 24, 2018

brianespinosa commented Jul 24, 2018

levithomason commented Jul 25, 2018

kuzhelov commented Jul 25, 2018

jurokapsiar commented Jul 25, 2018

levithomason commented Jul 25, 2018

jurokapsiar commented Aug 2, 2018

levithomason commented Aug 2, 2018

jurokapsiar commented Aug 2, 2018 •

edited

levithomason commented Nov 16, 2018

jurokapsiar commented Nov 19, 2018

codecov bot commented Nov 21, 2018 •

edited

kuzhelov commented Nov 23, 2018 •

edited

miroslavstastny Nov 23, 2018


		const hasChidlren = childrenExist(children)

		const maybeDirAuto = !hasChidlren && typeof content === 'string' ? { dir: 'auto' } : null

feat(Text): introduce dir attribute to support RTL #5

feat(Text): introduce dir attribute to support RTL #5

Conversation

kuzhelov commented Jul 24, 2018

levithomason commented Jul 24, 2018

brianespinosa commented Jul 24, 2018

levithomason commented Jul 25, 2018

kuzhelov commented Jul 25, 2018

jurokapsiar commented Jul 25, 2018

levithomason commented Jul 25, 2018

Just Text

Text > empty inline child

Text > inline child > text

Text > button child > text

jurokapsiar commented Aug 2, 2018

levithomason commented Aug 2, 2018

jurokapsiar commented Aug 2, 2018 • edited

levithomason commented Nov 16, 2018

jurokapsiar commented Nov 19, 2018

codecov bot commented Nov 21, 2018 • edited

Codecov Report

kuzhelov commented Nov 23, 2018 • edited

miroslavstastny Nov 23, 2018

Choose a reason for hiding this comment

jurokapsiar commented Aug 2, 2018 •

edited

codecov bot commented Nov 21, 2018 •

edited

kuzhelov commented Nov 23, 2018 •

edited