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

BIDI - Support base text direction #7264

Closed
Tracked by #14477
adir01 opened this issue Nov 10, 2020 · 38 comments
Closed
Tracked by #14477

BIDI - Support base text direction #7264

adir01 opened this issue Nov 10, 2020 · 38 comments
Labels
component: combobox package: components carbon-components package: react carbon-components-react planning: umbrella Umbrella issues, surfaced in Projects views type: bidi ↔️ type: enhancement 💡

Comments

@adir01
Copy link

adir01 commented Nov 10, 2020

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 :

  • auto
  • ltr
  • rtl
    However most of Carbon components does not apply text direction processing expected to happen according to this property settings and as a result of that , actual text direction inherits the page direction (which is by default LTR and is RTL when RTL style is applied), independently on textDir property setting

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
Copy link
Author

adir01 commented Nov 11, 2020

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

@wajnberg
Copy link

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
Copy link
Author

adir01 commented Jan 31, 2021

@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
Copy link
Author

adir01 commented Feb 11, 2021

@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
Copy link
Author

adir01 commented Feb 11, 2021

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
Copy link
Contributor

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
Copy link
Author

adir01 commented Feb 22, 2021

@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 feat: add textDir property to FormLabel #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

  1. 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
Copy link
Contributor

@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:

  • Alerts: Global Toast, is this referring to our Toast Notification Component?
  • ContextMenu: this is currently in a Pull Request by a community member
  • Dialog: is this for our Modal/ComposedModal components?
  • Flyout: I'm not sure which component this is referring to, could you share a link?
  • Global Banner: is this referring to the UI Shell header component?
  • Menu: is this referring to the OverflowMenu component?
  • MultiLine Truncated Text: is this referring to a specific component? If so, could you share a link?
  • Paragraph: is this referring to a specific component? If so, could you share a link?
  • Select Group: Is this referring to our Select component?
  • Toolbar: I'm not sure which component this is referring to, could you share a link?
  • Tree: this component is currently experimental and is not intended to be used by teams

@wajnberg
Copy link

wajnberg commented May 9, 2021

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
Copy link

wajnberg commented May 12, 2021

@joshblack
Copy link
Contributor

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
Copy link

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 #7683 (comment)
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
Copy link
Author

adir01 commented May 25, 2021

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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 👍

@joshblack
Copy link
Contributor

joshblack commented Jun 23, 2021

@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: #9013

@TazmanianDI
Copy link

@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
Copy link

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
Copy link
Author

adir01 commented Sep 12, 2021

@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
Copy link
Author

adir01 commented Sep 12, 2021

@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 #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 tay1orjones moved this to 🏗 In Progress in Design System Nov 9, 2021
@tay1orjones tay1orjones changed the title Bidi support: Base text direction is not supported in Carbon components [Umbrella]: BIDI - Support base text direction Nov 9, 2021
@tay1orjones tay1orjones moved this from 🏗 In Progress to ⏱ Backlog in Design System Nov 15, 2021
@tay1orjones tay1orjones moved this from ⏱ Backlog to 🏗 In Progress in Design System Dec 10, 2021
@tay1orjones tay1orjones added the planning: umbrella Umbrella issues, surfaced in Projects views label Dec 10, 2021
@tay1orjones tay1orjones changed the title [Umbrella]: BIDI - Support base text direction BIDI - Support base text direction Dec 10, 2021
@tay1orjones tay1orjones moved this from 🏗 In Progress to ⏱ Backlog in Design System Dec 13, 2021
@tay1orjones tay1orjones moved this from ⏱ Backlog to 🕵️‍♀️ Triage in Design System Feb 28, 2022
@tay1orjones tay1orjones added this to the 2022 Q2 milestone Mar 15, 2022
@tay1orjones tay1orjones removed this from the 2022 Q2 milestone Jun 17, 2022
@sstrubberg sstrubberg moved this to Triage in Roadmap Dec 19, 2022
@sstrubberg sstrubberg moved this from Triage to Icebox in Roadmap Dec 19, 2022
@tay1orjones
Copy link
Member

tay1orjones commented Sep 14, 2023

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 #7264 (comment)

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

There may be more now since that list was created

@tay1orjones
Copy link
Member

With the latest PR, #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!

@github-project-automation github-project-automation bot moved this from Icebox to Completed in Roadmap Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: combobox package: components carbon-components package: react carbon-components-react planning: umbrella Umbrella issues, surfaced in Projects views type: bidi ↔️ type: enhancement 💡
Projects
Archived in project
Development

No branches or pull requests

6 participants