search

carbon-design-system / carbon

A design system built by IBM
https://www.carbondesignsystem.com
Apache License 2.0
7.86k stars 1.82k forks source link

BIDI - Support base text direction #7264

Closed adir01 closed 1 year ago

adir01 commented 4 years ago

Base text direction is an integral part of the overall Bidi support and enables correct editing and readability of dynamic data for BIDI languages ( Arabic, Hebrew, Urdu)

Carbon code allows to specify following settings for the text direction, using textDir property :

Examples:

  1. Base text direction is not applied on Rich text editor

Steps: Apply RTL style on Carbon Set textDir property = rtl Text inside editor still has LTR text direction image

  1. Base text direction is not applied on combo box Steps: Apply RTL style on Carbon Set textDir property = auto Text at combo has RTL text direction

image

@mattrosno , @joshblack FYI

adir01 commented 4 years ago

One could find more information about text direction on https://www.w3.org/International/articles/inline-bidi-markup/

wajnberg commented 3 years ago

Just one precision. The property textdir exists only in https://github.ibm.com/BusinessAnalytics/ba-ui-carbon-toolkit which is an IBM repo It doesn't exist for this repo carbon-design-system We would need to add this property here.

adir01 commented 3 years ago

@wajnberg 1) PR that you submitted is not expected to cause closure of this issue. As we agreed it will be only the prototype which is expected to help Carbon developers provide similar feature support in other Carbon components. 2) Could you also please go through other components and ensure that similar solution is possible there ?

@joshblack : 1) could you please estimate when all other Carbon components can be supported 2) will it be possible to supply testing framework for this feature support ?

adir01 commented 3 years ago

@joshblack I just noticed there is missing description of expected rendering of Hebrew text In Example #1 it would be

image

Can be easily recreated in Notepad :

1) Paste following text 2) Right Mouse click and select Right-to-Left reading order

adir01 commented 3 years ago

Example 2 tries show the problem with English text. Expected text would be

please select an item ....

and not

....please select and item

Also: Item1 !!! and not !!! Item 1

joshblack commented 3 years ago

Hey @adir01! ๐Ÿ‘‹

could you please estimate when all other Carbon components can be supported

Could you share more about what you're looking for support on? I believe text direction can be set on the product side, for things like layout bugs definitely happy to address them ๐Ÿ‘ I think having a list handy for components that have the directional bugs would be a first step to planning how many items need to be addressed and how long. Hope this makes sense!

adir01 commented 3 years ago

@joshblack

Please find my answers: 1) I believe text direction can be set on the product side - It looks like discussion about that is under https://github.com/carbon-design-system/carbon/pull/7683 and my understanding is still that without changes in components itself ( at least some of them) it will not be possible to support text direction. My understanding would be that we should have the same approach ( and same user coding ) for different components, which would mean that change will be needed for all components that currently miss support for the text direction 2) I think having a list handy for components that have the directional bugs -

Here is a list of components in which we would like to see correct text direction processing:

Accordeon Alerts: Global Toast Checkbox ComboBox ContextMenu Dialog Expandable Tile; Flyout Global Banner Grid List Menu MultiLine Truncated Text Paragraph Radio Select Group Simple Table Tabs (for content) Tag Input TextArea Toggle Toolbar Tree

3) I think @wajnberg will be able to provide the code for the most of above components assuming that your team will:

a) review his changes in a short time period b) if your team will provide testing infrastructure for these changes we will also perform the testing

joshblack commented 3 years ago

@adir01 thanks so much for putting the list together! From the discussion that you noted in #7683, it seems like the important thing for our team to remember is that there are two requirements:

  1. Ability to target text and its direction (but not a layout change)
  2. Ability to target a layout change (aka mirroring the layout)

And that these two requirements can individually be enabled or both enabled together. Is this correct?

For the list of components, it will help out a ton to have steps to reproduce (ideally through our Codesandbox template) both to verify the issue and have a test case to make sure the issue has been addressed. I totally understand if this is a general problem, though, and just having a template that demonstrates it with a handful would be more than sufficient ๐Ÿ‘

One note: some of the components in that list seem to potentially be named differently or might not exist in the system. Some of the ones I noticed:

wajnberg commented 3 years ago

Hello @joshblack ,

I am sorry for the late answer

@adir01 thanks so much for putting the list together! From the discussion that you noted in #7683, it seems like the important thing for our team to remember is that there are two requirements:

1. Ability to target text and its direction (but not a layout change)

2. Ability to target a layout change (aka mirroring the layout)

This is correct.

And that these two requirements can individually be enabled or both enabled together. Is this correct? This is correct

For the list of components, it will help out a ton to have steps to reproduce (ideally through our Codesandbox template) both to verify the issue and have a test case to make sure the issue has been addressed. I totally understand if this is a general problem, though, and just having a template that demonstrates it with a handful would be more than sufficient ๐Ÿ‘

One note: some of the components in that list seem to potentially be named differently or might not exist in the system. Some of the ones I noticed:

* Alerts: Global Toast, is this referring to our Toast Notification Component?

Yes

  • ContextMenu: this is currently in a Pull Request by a community member

  • Dialog: is this for our Modal/ComposedModal components? I would remove this component for now because it unlikely contains user originated data.

  • Flyout: I'm not sure which component this is referring to, could you share a link? You can safely remove this one.

  • Global Banner: is this referring to the UI Shell header component? We can also remove it for now because it actually contains other UI components inside. It would be better to enforce text direction on the sub components.

  • Menu: is this referring to the OverflowMenu component? This is right

  • MultiLine Truncated Text: is this referring to a specific component? If so, could you share a link? This is a mistake.

  • Paragraph: is this referring to a specific component? If so, could you share a link? This is a mistake.

  • Select Group: Is this referring to our Select component? Right.

  • Toolbar: I'm not sure which component this is referring to, could you share a link? We can also remove it for now because it actually contains other UI components inside. It would be better to enforce text direction on the sub components.

  • Tree: this component is currently experimental and is not intended to be used by teams OK

wajnberg commented 3 years ago

Hello @joshblack ,

To sum up please find below the list of components for which text direction should be supported.

@adir01 FYI

joshblack commented 3 years ago

Thanks for following up @wajnberg!

Is one possible resolution here supporting dir="auto" on text nodes or is it similar to the textDir prop in the previous PRs that have been made?

In general, I think we'd be hesitant to add specific props per component for this kind of capability. It'd be great to come up with something general purpose that also handles the use-cases that are being brought up.

wajnberg commented 3 years ago

Thanks for following up @wajnberg!

Is one possible resolution here supporting dir="auto" on text nodes or is it similar to the textDir prop in the previous PRs that have been made?

Of course dir="auto" on text nodes would work perfectly on almost every browser types (except from IE. but since it would be sunset in 2022 you probably don't care of it ). However given that textDir and dir are 2 different features (textDir and mirroring) as explained at https://github.com/carbon-design-system/carbon/pull/7683#issuecomment-777800587 the addition of the textDir prop might be unavoidable.

In general, I think we'd be hesitant to add specific props per component for this kind of capability. It'd be great to come up with something general purpose that also handles the use-cases that are being brought up.

adir01 commented 3 years ago

I would say dir=auto could be used in principal if textDir can not be used , but in general case it is not good solution. dir=auto decides about text direction based on the first strong character, which is definitely not good decision in many cases. There is no doubt that enforcing of text direction by application or end user is much preferable. Why not use it , especially if we already have dir property for static text direction control ? I assume ( @wajnberg - please correct me if I am wrong) similar code changes will be required for both

joshblack commented 3 years ago

In general, I think we'd prefer to avoid adding a textDir prop to each component that renders text on the page. Instead, it'd be nice to figure out a common abstraction that could be used across components.

To try and illustrate why, we could start with a component with several text nodes being rendered by that component.

function TestComponent() {
  return (
    <>
      <span>Text node A</span>
      <span>Text node B</span>
      <span>Text node C</span>
    </>
  );
}

In a situation where textDir is supplied to the component, we would need to make sure that text nodes in the component use dir={textDir}. 

function TestComponent({ textDir }) {
  return (
    <>
      <span dir={textDir}>Text node A</span>
      <span dir={textDir}>Text node B</span>
      <span dir={textDir}>Text node C</span>
    </>
  );
}

Thankfully this situation is not bad at all, we could implement this as-is, add some tests, and call it a day.

As components start to grow, or more text nodes are added, you could imagine that having to make sure these keep in sync can be tedious in terms of updating component code, tests, or making sure that all components pass around textDir as a prop in the case of a component rendering another component that renders text.

function TestComponent({ textDir }) {
  return (
    <>
      <span dir={textDir}>Text node A</span>
      <span dir={textDir}>Text node B</span>
      <span dir={textDir}>Text node C</span>
      <AnotherTestComponent textDir={textDir} />
    </>
  );
}

Over time, we would need to manage textDir through potentially a chain of components and make sure we had tests added in for text direction inside of components. Part of this flow starts to mirror aspects of React's Context feature, in ways.

This process prompted the idea of a common Text component that is used to render text in every component in our project. By default, it would use dir="auto" but it would also pair with a TextDirection component that would allow setting the direction of text in a given sub-tree of React components.

This idea is based on Facebook's TetraText component that they seem to use in their codebase in their new website.

Would love to hear what you all think, just sharing my train of thought when it comes to the textDir prop itself and trying to find a way to generalize this to make sure text direction is simple/easy to configure for components in Carbon.

wajnberg commented 3 years ago

Hello @joshblack

That could be a great idea ๐Ÿ‘ . However please keep in mind that text direction should be enforced only for user originated data (artifact name ...). So all fixed strings coming along with the product should not enforce text direction. So Text component should only be used for those components containing user originated data.

joshblack commented 3 years ago

that text direction should be enforced only for user originated data

Could you share more information / examples about this requirement? I'm not quite sure what this means in the context of this thread and would appreciate any additional info you could offer for this topic ๐Ÿ‘€

For example, in the above message, there is Accordion listed as a component that needs support. I would imagine most of the time this component contains fixed strings versus user-generated content, but could definitely understand if it is dynamic sometimes.

If this is the case, would there be a case for potentially any component in the system having a text node that is user-generated content?

I could also be totally misunderstanding this so please let me know if I'm off here ๐Ÿ‘

wajnberg commented 3 years ago

Hello @joshblack ,

You are not off at all ๐Ÿ‘ Actually one cannot foresee which text node would contain user originated data. So this is what I propose:

This is an example:

const props = () => ({ id: text('Combobox ID (id)', 'carbon-combobox-example'), placeholder: text('Placeholder text (placeholder)', 'Filter...'), titleText: text('Title (titleText)', 'Combobox title'), helperText: text('Helper text (helperText)', 'Optional helper text here'), ),

});

const appliesOn= ["items"]; export const Playground = () => ( <div style={{ width: 300 }}> <TextDirection dir="rtl" appliesOn={appliesOn}> <ComboBox items={items} itemToString={(item) => (item ? item.text : '')} {...props()}> </TextDirection> </div> ); This way text direction should be applied only on the "items" and not on all the other text components.

Does it make sense ?

joshblack commented 3 years ago

@wajnberg thanks for taking the time to confirm and for the example!

Given the need to potentially scope where the text direction gets applied, would something like this work: https://codesandbox.io/s/modest-dew-mg5b6?file=/src/App.js

In this example, we use the getTextDirection prop on TextDirection to allow a consumer to derive text direction from the text provided to a Text component.

Totally understand if it does not work for this use case, basically what I'm trying to avoid is the need to annotate each individual text node with some kind of identifier to allow scoping for text direction. This could cause some downstream impacts that could cause broad changes.

As a result, I am trying to figure out the best way to generalize this that works for the common use case and the use-cases that are being brought up here ๐Ÿ‘

wajnberg commented 3 years ago

Hello @joshblack ,

Thank you for putting together this sandbox! You are absolutely right: The goal is to scope where text direction should be applied. However if you refer to the above combobox sample. How end user would specify that text direction should be applied on the items and not on on the titletext for instance ? All end user can do is to surround the <Combobox> with the <TextDirection> direction. But then it would apply on all the <Text> components inside which is not correct.

Could you please elaborate ?

Thank you so much for your help

joshblack commented 3 years ago

@wajnberg the idea was that getTextDirection is an escape hatch that could be used for any text that is rendered by a Text component.

Specifically, it would be a function called with every string in the tree rendered within TextDirection. Then, you could determine what direction you would like the text to be displayed in.

// Example render
<TextDirection
  getTextDirection={(text) => {
    // This would be called with: 'Field label' and then 'ืฉืœื•ื Hello !!'
    // It then returns one of: `ltr, rtl, auto`
    return detectTextDirection(text);
  }}>
  <MyCustomComponent />
</TextDirection>

function MyCustomComponent() {
  return (
    <fieldset>
      <legend>
        <Text>Field label</Text>
      </legend>
      <input type="checkbox" />
      <label>
        <Text>ืฉืœื•ื Hello !!</Text>
      </label>
    </fieldset>
  );
}
wajnberg commented 3 years ago

Hello @joshblack

Yes i perfectly understood the idea. But what about an end user who uses ComboBox and wants to apply text direction only for {items} (as shown in the previous sample) and not for any other Text. He cannot do that with this solution because he doesn't have access to tree rendered by ComboBox.

Am I missing anything ?

joshblack commented 3 years ago

@wajnberg I think youโ€™re totally right that you couldnโ€™t scope it to a group of items inside of Combobox. The hope with this solution was to provide a way in which one could accurately determine the text direction of a node without having to add props for targeting text, or groups of text, inside of a component.

This approach of adding props to target specific text or groups of text inside of a component is unfortunately not something that I think we could offer as part of the library.

Is there a situation with the combobox example that you mentioned where this strategy would produce incorrect behavior? Or an aspect about it that is not preferred?

wajnberg commented 3 years ago

Hello @joshblack ,

Let's assume we have a combo with 2 options and one title as follows:

const items = [
  {
    id: 'option-0',
    text:
      'ืฉืœื•ื ืขื•ืœื Hello !!',
  },
  {
    id: 'option-1',
    text: ''ืฉืœื•ื ืขื•ืœื Hello 2 !!',
  },
 }

export const Playground = () => (
  <div style={{ width: 300 }}>
    <ComboBox
      items={items}
      itemToString={(item) => (item ? item.text : '')}
      titleText:'my Title !!`
    />
  </div>
);

We want to display the options in RTL direction i.e we want to get the following: !! Hello ืฉืœื•ื ืขื•ืœื but the title should be displayed my Title (because it is not user originated data. Without defining the scope of text direction inside the ComboBox we cannot achieve that.

joshblack commented 3 years ago

@wajnberg it might help to separate this from something like combobox and find a minimal example that breaks in the situations that you're describing.

As an example, I updated that demo with the approach mentioned above. It has a component that's Combobox-esque, using the title and options that you provided:

<SampleComponent
  title="My Title !!"
  options={[
    {
      id: 'option-0',
      text: 'ืฉืœื•ื ืขื•ืœื Hello !!',
    },
    {
      id: 'option-1',
      text: 'ืฉืœื•ื ืขื•ืœื Hello 2 !!',
    },
  ]}
/>

To render the following image:

image

Link: https://codesandbox.io/s/modest-dew-mg5b6?file=/src/App.js

Is this matching what you're hoping the component would be able to tackle? If not, what limitations are they / what situations would cause the issues to occur?

wajnberg commented 3 years ago

Hello @joshblack ,

It's working because you use the auto direction and My Title!! is displayed LTR when textDir=auto But can you manage to display ื”ื›ื•ืชืจืช ืฉืœื™ !! for the title ?

const items = [
  {
    id: 'option-0',
    text:
      'ืฉืœื•ื ืขื•ืœื Hello !!',
  },
  {
    id: 'option-1',
    text: ''ืฉืœื•ื ืขื•ืœื Hello 2 !!',
  },
 }

export const Playground = () => (
  <div style={{ width: 300 }}>
    <ComboBox
      items={items}
      itemToString={(item) => (item ? item.text : '')}
      titleText:'ื”ื›ื•ืชืจืช ืฉืœื™!!`
    />
  </div>
);

More generally the solution you propose is to refactor the ComboBox. But what about an application (i.e Cognos or whatever) which already uses the ComboBox from Carbon. With your solution you should rewrite completely the ComboBox component.

joshblack commented 3 years ago

@wajnberg I updated that codesandbox with the title you mentioned:

<SampleComponent
  title="ื”ื›ื•ืชืจืช ืฉืœื™!!"
  options={[
    {
      id: 'option-0',
      text: 'ืฉืœื•ื ืขื•ืœื Hello !!',
    },
    {
      id: 'option-1',
      text: 'ืฉืœื•ื ืขื•ืœื Hello 2 !!',
    },
  ]}
/>

To get the following result:

image

Link: https://codesandbox.io/s/modest-dew-mg5b6?file=/src/App.js

Per your note about existing usage of Combobox, it seems like we have two directions we could take:

Let me know if I'm missing a situation, it seemed like you are concerned about a potential rewrite. My intent here is to, ideally, support this use-case without any Component API changes to Combobox.

When looking at the overall impact of each direction, it seemed like the latter would have fewer changes while still supporting the same use-cases. If there is a use-case that it doesn't support that you're looking for, it would help out a ton to have a specific example and I can try and see how we can accomodate it.

wajnberg commented 3 years ago

Hello @joshblack ,

The title should appear ื”ื›ื•ืชืจืช ืฉืœื™!! and not !!ื”ื›ื•ืชืจืช ืฉืœื™. Thank you

joshblack commented 3 years ago

Thanks @wajnberg! That helps a ton. To force the ltr on the title, I updated the example to:

<TextDirection
  getTextDirection={(text) => {
    if (text === title) {
      return 'ltr';
    }
    return 'auto';
  }}>
  <SampleComponent
    title={title}
    options={[
      {
        id: 'option-0',
        text: 'ืฉืœื•ื ืขื•ืœื Hello !!',
      },
      {
        id: 'option-1',
        text: 'ืฉืœื•ื ืขื•ืœื Hello 2 !!',
      },
    ]}
  />
</TextDirection>

To get the following image:

image

LInk: https://codesandbox.io/s/modest-dew-mg5b6?file=/src/App.js

wajnberg commented 3 years ago

Hello @joshblack ,

Yes it's working now. So let's assume we have an application which uses ComboBox from Carbon. And we want to achieve exactly what you did in the sample you created. Applying TextDirection directive is irrelevant because ComboBox does not have Text component inside (it's rather <div>' element.) So the only solution is to extendComboBoxand enforce textDirection toitems` only. Can you confirm that ?

joshblack commented 3 years ago

@wajnberg that's totally right ๐Ÿ‘ The change on our end would be to update components, including ComboBox, to use the Text component which then would unlock these kinds of capabilities.

This is an alternative approach to the other strategy mentioned above where props would need to be added per component for determining text direction of things like labels, options, etc.

wajnberg commented 3 years ago

Hello @joshblack

I think I got the point now. When do you think the the components update would start ? Do you think we can contribute to these changes ?

Thank you so much for your help

joshblack commented 3 years ago

@wajnberg great question, I am trying to get a consensus now with the team. This type of change ultimately has a broad impact across the codebase and could impact parts of the code that downstream frameworks like Angular/Svelte/Vue who use our HTML as a reference.

For transparency, the team is also working on our v11 release so we are trying to balance work there with support and types of issues like this that may require broader changes.

I went ahead and opened up a draft PR to better summarize and discuss the changes brought up here and apply them to components to view in the storybook: https://github.com/carbon-design-system/carbon/pull/9013

TazmanianDI commented 3 years ago

@joshblack I'm not sure if this necessarily belongs in this issue since I don't know if this qualifies as a text direction (textDir) issue but I wanted to give another concrete example where I think Carbon isn't doing the right thing.

Here is what a Button looks like: image

If the page uses dir=rtl this button does not change appearance even though the text really should be shifted to the right. The reason for this is that the Button has the following padding:

calc(0.875rem - 3px) 63px calc(0.875rem - 3px) 15px

This is how you get the asymmetric appearance because you've got 63px on the right and 15px on the left. But if the page is rtl, those two values need to be flipped and we would get

calc(0.875rem - 3px) 15px calc(0.875rem - 3px) 63px

and the button would look like this: image

Basically any time you have any css values with a left or right values that don't match, I think you need a [dir=rtl] rule that reverses them.

wajnberg commented 3 years ago

Hello @TazmanianDI

Thank you for your comments. The issue you describe has nothing to do with text direction It is rather related to GUI mirroring Text direction is the ability to write the text from left to right or from right to left For instance "hello!" is for LTR dispaly "!hello" is for RTL display

adir01 commented 3 years ago

@TazmanianDI As @wajnberg said: the problem that you describe is related to mirroring and has nothing to do with text direction which is addressed by this issue. However (unfortunately) the name of attribute that controls direction of static components ( i.e. Button) is textDir which is quite confusing. ( @joshblack probably it would be better rename it to something else (i.e compDir ?) But for now : the problem that you describe is a real problem and I suggest that you will open a separate issue on it, and ensure that correct CSS is used when textDir is rtl. This issue is intended to provide correct handling of BIDI text direction ( not based on textDir)

adir01 commented 3 years ago

@joshblack my understanding is that the first submission that is expected to resolve this issue and provide BIDI support for text direction for Accordion, Button, Checkbox, Combobox and Contentswitcher according to design agreed upon above is https://github.com/carbon-design-system/carbon/pull/9493 which is currently waiting your review. Can you please provide ETA for this PR and for remaining components processing? If you need @wajnberg help with remaining components processing, please open issue and assign to him

tay1orjones commented 1 year ago

It's been a while since we've looked at this, but there are still some components that have not been updated to use Text internally. The original list is in this comment above https://github.com/carbon-design-system/carbon/issues/7264#issuecomment-839870634

  • [x] Notifications
  • [x] OrderedList
  • [x] OverflowMenu
  • [x] ProgressIndicator
  • [x] RadioButton
  • [x] Search
  • [x] Select
  • [x] Slider
  • [x] StructuredList
  • [x] Tabs
  • [x] Tag
  • [x] TextArea
  • [x] TextInput
  • [x] Tile
  • [x] Toggle
  • [x] Tooltip
  • [x] TooltipDefinition
  • [x] UnorderedList

There may be more now since that list was created

tay1orjones commented 1 year ago

With the latest PR, https://github.com/carbon-design-system/carbon/pull/14679, the initial list identified here is now complete and supports bidi via using Text within the component source.

If there's additional components or further work that needs to happen in relation to this issue, please open a new issue and reference/link here. Thank you!