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

Fix #2644 Use "dir" attribute instead of "direction" CSS style #2668

Closed
wants to merge 5 commits into from

Conversation

JiuqingSong
Copy link
Collaborator

@JiuqingSong JiuqingSong commented May 29, 2024

According to https://developer.mozilla.org/en-US/docs/Web/CSS/direction, the HTML attribute "dir" is preferred than CSS "direction" for RTL. So let's switch to HTML attribute in our format applier.

@JiuqingSong JiuqingSong changed the title Fix #2644 Fix #2644 Use "dir" attribute instead of "direction" CSS style May 29, 2024
@JiuqingSong JiuqingSong marked this pull request as ready for review May 29, 2024 17:45
@@ -14,7 +14,7 @@ export const directionFormatHandler: FormatHandler<DirectionFormat> = {
},
apply: (format, element) => {
if (format.direction) {
element.style.direction = format.direction;
element.dir = format.direction;
Copy link
Contributor

Choose a reason for hiding this comment

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

dir supports rtl | ltr | auto. auto is converted to ltr in this format handler

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, in that case, it causes a problem that our code does not know if it is in ltr or rtl, but only browser knows. In content model we want everything to be controlled and specified within content model, but not let browser do dynamically determination.

Note that although roosterjs is working on top of HTML, it doesn't mean all HTML tags/attributes are supported. We only support those tags/attributes that are suitable for editing, because we want user to write content, but not "building a web page".

What is the actual requirement here?

Copy link
Collaborator Author

@JiuqingSong JiuqingSong Jun 6, 2024

Choose a reason for hiding this comment

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

I feel "auto" should be mostly used by building a web page, but not editing content.

For editor, the style should be predictable, which means editor should have full control of its style, but not leverage browser's magic behavior. When set to "auto", will the direction automatically changed while user is typing in some RTL language? Say I just type some English, then switch to type Arabic, then suddenly the layout is changed since browser thinks there are more Arabic than English?

So I'll say, in roosterjs, we don't support "dir=auto". The direction should be either ltr or rtl, explicitly. Just like MFC won't wrap all Windows API, RoosterJs won't wrap all HTML.

If you really want to use dir=auto, add it to editor's container DIV element, but not in user's content. It does not make sense that only some paragraph has dir="auto" but other paragraph does not have. They should either all have (then it is the same when apply to root DIV) or all not have.

Copy link
Contributor

@haven2world haven2world Jun 7, 2024

Choose a reason for hiding this comment

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

Sorry I missed the message.

We want to support auto because on android, due to the custom keyboards, it's hard to know what language the users are using, so we have to depend on the browser to decide the text direction.

We supported different text direction by paragraphs. To implement it, we set auto on new paragraphs.

About more detail about android side, @Rain-Zheng could you help provide more information?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @haven2world for helping explain this! Since Android IME doesn't provide any API for getting the input language, we have to use auto as a workaround. By setting this, Chrome will automatically adjust the layout of a new paragraph according to the language. If the paragraph already contains some content and users switch to another language with a different direction, the layout won't be changed.

I tested applying the dir="auto" to editor container div, unfortunately it doesn't work for Android:

HTML Result
image image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you try to test each minimum repro in what case it work and what case it doesn't?

For example

<div dir="auto">[RTL text here]</div>

<div dir="auto"><span>[RTL text here]</span></div>

<div dir="auto"><div>[RTL text here]</div></div>

<div dir="auto"><table><tr><td>[RTL text here]</td></tr></table></div>

<div dir="auto"><blockquote>[RTL text here]</blockquote></div>

<div dir="auto"><ol><li>[RTL text here]</li></ol></div>

<div dir="auto">[RTL text here]<div>[RTL text here]</div>[RTL text here</div></div>

<div dir="auto">[RTL text here]<br>regular text here</div>

<div dir="auto">regular text here<br>[RTL text here]</div>

I want to get deeper understand about this property

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On the other hand, you can always override format handler using EditorOptions to pass in your customized format handler. That allows you create you own rendering/parsing logic

Copy link
Contributor

Choose a reason for hiding this comment

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

I created a fiddle here: https://jsfiddle.net/pjnq1o9c/

According to the results, these cases are workinig as expected:

<div dir="auto">[RTL text here]</div>

<div dir="auto"><span>[RTL text here]</span></div>

<div dir="auto"><div>[RTL text here]</div></div>

<div dir="auto"><table><tr><td>[RTL text here]</td></tr></table></div>

<div dir="auto"><blockquote>[RTL text here]</blockquote></div>

<div dir="auto"><ol><li>[RTL text here]</li></ol></div>

<div dir="auto">[RTL text here]<div>[RTL text here]</div>[RTL text here]</div>

And the following cases are problematic:

<div dir="auto">[RTL text here]<br>regular text here</div>

<div dir="auto">regular text here<br>[RTL text here]</div>

<div dir="auto"><div>regular text here</div><div>[RTL text here]</div></div>

Currently we are overriding the rooster's default direction handler as a workaround.

Copy link
Collaborator Author

@JiuqingSong JiuqingSong Jun 7, 2024

Choose a reason for hiding this comment

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

Ok. I think the principal here is, browser may count the all character under "dir=auto", then decide the direction for this whole container, but not for each child element.

Since the overriding works, I'll treat this as solved. RoosterJs only provide support for the most common scenarios. For each specific feature, if the common support is not enough, you need to create your override or extension. In OWA we also have a lot of override that only needed by OWA, but not common roosterjs. The most we can do is to provide enough entry point to override, but not take every specific requirement into roosterjs. That is why we support plugin and override.

@Andres-CT98
Copy link
Contributor

I found three instances of the following pattern being used:
getComputedStyle([Element]).direction

Would this change affect these instances?

@JiuqingSong
Copy link
Collaborator Author

I found three instances of the following pattern being used: getComputedStyle([Element]).direction

Would this change affect these instances?

No. When retrieve result from CSS, we can get the same result from "dir=..." and "style=direction:...". If they have different value, we will get the CSS one.

@JiuqingSong JiuqingSong closed this Jun 7, 2024
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.

4 participants