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

Add RTL support to Textbox, Markdown, Chatbot #4933

Merged
merged 18 commits into from Jul 17, 2023
Merged

Add RTL support to Textbox, Markdown, Chatbot #4933

merged 18 commits into from Jul 17, 2023

Conversation

abidlabs
Copy link
Member

@abidlabs abidlabs commented Jul 16, 2023

Adds support for right-to-left languages, such as Arabic or Hebrew, to gr.Textbox, gr.Markdown, gr.Chatbot

I realized halfway through that this is a bit tricky, because there are two different aspects of an RTL language:

  • The text should be right-aligned
  • The direction of the typing should be on the left (the cursor, and new characters should appear on the left)

The way I've implemented this is as follows:

  • The Textbox component now takes a boolean parameter rtl which sets direction to RTL and right-aligns the text
  • In case someone wants to align the text but not change the directionality, there's a separate text_align parameter that only aligns the text
  • Markdown and Chatbot also take the rtl parameter. Since they don't accept text, they don't have a separate text_align parameter.

Closes: #4325

Test code:

import gradio as gr

with gr.Blocks() as demo:
    gr.Textbox("شلاؤ abc", interactive=True)
    gr.Textbox("شلاؤ abc", interactive=True, text_align="right")
    gr.Textbox("شلاؤ abc", interactive=True, rtl=True)
    gr.Markdown("شلاؤ" + " abc")
    gr.Markdown("شلاؤ" + " abc", rtl=True)
    gr.Chatbot([[
        "شلاؤ" + " abc",
        "شلاؤ" + " abc"
    ]])
    gr.Chatbot([[
        "شلاؤ" + " abc",
        "شلاؤ" + " abc"
    ]], rtl=True)
    
demo.launch()

@vercel
Copy link

vercel bot commented Jul 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
gradio ✅ Ready (Inspect) Visit Preview Jul 17, 2023 1:47pm

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Jul 16, 2023

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4933-all-demos


You can install the changes in this PR by running:

pip install https://gradio-builds.s3.amazonaws.com/6d40cdc7c516434d498d6b1bf129cfaa1d5a7eb3/gradio-3.36.1-py3-none-any.whl

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Jul 16, 2023

🎉 Chromatic build completed!

There are 11 visual changes to review.
There are 0 failed tests to fix.

@abidlabs
Copy link
Member Author

I also added a few stories for visual testing of Textbox, Markdown, and Chatbot while I was at it. I didn't worry about tabulating the full parameter list (since they will eventually be populated programmatically)

Overall, it was very pleasant to add the visual tests!

@@ -89,6 +91,8 @@ def __init__(
elem_id: An optional string that is assigned as the id of this component in the HTML DOM. Can be used for targeting CSS styles.
elem_classes: An optional list of strings that are assigned as the classes of this component in the HTML DOM. Can be used for targeting CSS styles.
type: The type of textbox. One of: 'text', 'password', 'email', Default is 'text'.
text_align: How to align the text in the textbox. One of: 'left', 'center', 'right', 'justify'. Default is 'left'. Can only be changed if type=='text'.
rtl: If True and type=='text', sets the direction of the text to right-to-left (text is aligned right, and cursor appears on the left of the text). Takes precendence over the `text_align` parameter. Default is False, which renders cursor on the right.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo precedence


<Story
name="Simple inline Markdown"
args={{ }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i think we can just remove the args param if not in use, it's not required

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't the right way to handle rtl, it isn't just about alignment. We should be using dir=rtl in all of these cases. It is supported by pretty much every element and handles alignment by default.

We will also want to support rtl for all components i think, because everything has some text or label. Components like HighlightedText should also support rtl, for which alignment won't work.

We also want to consider if we should support rtl at the document level, to make the whole gradio app rtl.

There is more info on this here: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/dir

The CSS property should generally be avoided in favour of the html attribute.

@pngwn
Copy link
Member

pngwn commented Jul 17, 2023

Would we not also want to flip the orientation of our layouts for global rtl content?

@abidlabs
Copy link
Member Author

abidlabs commented Jul 17, 2023

This isn't the right way to handle rtl, it isn't just about alignment. We should be using dir=rtl in all of these cases. It is supported by pretty much every element and handles alignment by default.

For the gr.Textbox(), which can be interactive, I did use the CSS property direction which is equivalent to dir="rtl". I can change to use the HTML attribute, if that's preferable

I also added text_align as a separate property because there may be instances when users want to change the alignment, but not the direction

Since gr.Markdown() and gr.Chatbot() do not accept user input, RTL is equivalent to setting the text to be right-aligned. At least as far as I can tell. Is there a difference?

We will also want to support rtl for all components i think, because everything has some text or label. Components like HighlightedText should also support rtl, for which alignment won't work.

We also want to consider if we should support rtl at the document level, to make the whole gradio app rtl.

Would we not also want to flip the orientation of our layouts for global rtl content?

We can certainly add RTL support to gr.HighlightedText(), etc. as well as to the whole Blocks() but we can add them in a future PR. I prioritized these components because of recent interest in chatbots and upcoming announcement of gr.ChatInterface

@pngwn
Copy link
Member

pngwn commented Jul 17, 2023

I also added text_align as a separate property because there may be instances when users want to change the alignment, but not the direction

Can't they do that with a theme?

@abidlabs
Copy link
Member Author

Disagree a bit @pngwn @hannahblair -- applying RTL to components vs. the entire Blocks are two separate issues.

Applying RTL to the whole app is a lot more work since we have to modify entire layouts and is out of scope of this PR.

But there are cases where only specific component (e.g. a gr.Textbox()) is expected to take e.g. Arabic and should be rendered RTL whereas the other parameters are in English. Here's kind-of an example of this: https://huggingface.co/spaces/bkhmsi/Font-To-Sketch

@pngwn
Copy link
Member

pngwn commented Jul 17, 2023

Since gr.Markdown() and gr.Chatbot() do not accept user input, RTL is equivalent to setting the text to be right-aligned. At least as far as I can tell. Is there a difference?

As @hannahblair said, rtl handles more than just alignment. It will also lay out adjacent elements right to left, which alignment won't. This is a little more reliable, especially if we were to expand any of these components in the future to have more complex markup. With dir=rtl, it would 'just work' whereas alignment won't in many cases.

@abidlabs
Copy link
Member Author

Can't they do that with a theme?

No a theme would apply to the entire app -- whereas this property applies on a component level

@pngwn
Copy link
Member

pngwn commented Jul 17, 2023

I've mentioned before that we could pass theme properties into components to have them only apply to that component. I'd argue that text alignment is stylistic (so a theme should handle it) but rtl is semantic.

In the example space you linked, what would we do about the table elements/ examples?

The example does help clarify though, this is less about localisation and more about supporting multilingual apps with bits of rtl text.

@abidlabs
Copy link
Member Author

The example does help clarify though, this is less about localisation and more about supporting multilingual apps with bits of rtl text.

Yes exactly! We could support full RTL too, but that's for a future PR :)

In the example space you linked, what would we do about the table elements/ examples?

The examples actually look great -- since the only thing that needs to be rendered RTL is the text in the "text" column, which is already the case

@abidlabs
Copy link
Member Author

abidlabs commented Jul 17, 2023

Ok so maybe just to summarize the changes that are needed on this PR (as opposed to things we could include in a future PR):

  • We should use the HTML dir attribute instead of the CSS direction
  • We should use that in the gr.Chatbot() and gr.Markdown() as well -- rename the Python parameter from text_align to rtl and implement it using the the dir attribute

Is that correct?

@hannahblair
Copy link
Collaborator

hannahblair commented Jul 17, 2023

+1 thank you for that demo example, that helps me better understand the potential usage of this :)

We could apply dir="rtl" (/the dynamic dir value) to the parent div (line 554) in js/app/src/Blocks.svelte so everything is applied in a Block, and if someone wants a different component to be left-to-right they can create a new Block with dir="ltr". That also would be straightforward to apply dynamically. Then we wouldn't have to worry about applying this to individual components as this formatting would be inherited throughout that block perhaps?

@pngwn
Copy link
Member

pngwn commented Jul 17, 2023

@hannahblair The only issue with applying it to the parent is that it would rtl the labels and explanation text etc. Which i don't think is the intention of this PR. In order to only rtl granularly, we can just add dir=rtl to things that currently have the text_align thing.

@abidlabs
Copy link
Member Author

Ok great, thanks @pngwn @hannahblair for the feedback! I'll make the changes requested

@abidlabs
Copy link
Member Author

All righty, backend, frontend, and stories have been updated to reflect the new parameters. I also updated the test code snippet at the top of the PR if I could get another pass!

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for fixing this @abidlabs!

Copy link
Collaborator

@hannahblair hannahblair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@abidlabs
Copy link
Member Author

Thanks folks!

@abidlabs abidlabs merged commit b16732f into main Jul 17, 2023
12 checks passed
@abidlabs abidlabs deleted the rtl branch July 17, 2023 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RTL support in gr.Textbox
4 participants