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

Change RTL detection to rely on unicode-bidi paragraph by paragraph #14573

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

Gargron
Copy link
Member

@Gargron Gargron commented Aug 14, 2020

@@ -821,6 +822,7 @@
a {
color: $secondary-text-color;
text-decoration: none;
unicode-bidi: isolate;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this shouldn't be applied for all links but links such as mentions and hashtags.

Copy link
Member Author

Choose a reason for hiding this comment

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

But all links are ascii (https://...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily. Mastodon allows links with arbitrary text from other software.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would wager that the majority of links would be ascii URLs and we'll have consistently better results by not making them affect the directionality of the whole post.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to bring a live instance with these changes so I test multiple possible scenarios?

@ClearlyClaire
Copy link
Contributor

It might not be well-supported in Safari: https://caniuse.com/#feat=mdn-css_properties_unicode-bidi_plaintext

@Gargron
Copy link
Member Author

Gargron commented Sep 1, 2020

Okay, so I see two options here. Maybe we can get away with this not working in Safari, who knows how many Safari users we have that would be affected by this (directionality of the posts would be affected by interface language, so if you use the UI in a right-to-left language, you'd see posts right-to-left as well). Or maybe we keep the flawed detection code and the dir attributes and hopefully the unicode-bidi property overrides that when the browser supports it.

@ClearlyClaire
Copy link
Contributor

Okay, so I see two options here. Maybe we can get away with this not working in Safari, who knows how many Safari users we have that would be affected by this (directionality of the posts would be affected by interface language, so if you use the UI in a right-to-left language, you'd see posts right-to-left as well).

It could be ok, but I'm not sure…

Or maybe we keep the flawed detection code and the dir attributes and hopefully the unicode-bidi property overrides that when the browser supports it.

If I understand the specs correctly, the plaintext and isolate values would indeed override the dir attributes where supported.

That being said, the spec says “Authors should not use unicode-bidi in HTML documents.”

An alternative could be to not set the direction on toots, and wrap them and each of their <p> in a <bdi>

@Gargron
Copy link
Member Author

Gargron commented Nov 27, 2020

I have heard that dir="auto" takes care of detecting text direction in input elements. So, rtl.js can be fully removed.

I'm in favour of relying on browser support here.

@ahangarha
Copy link
Contributor

Using 'dir=auto' will handle everything but excluding mentions in the beginning of the paragraph.

@Gargron
Copy link
Member Author

Gargron commented Nov 27, 2020

My motivation here is that #15218 seems to indicate some kind of recursion error in the isRtl JavaScript function (though I fail to see it). This PR is a one stone two birds kind of deal in that regard.

@Gargron Gargron merged commit 1f56405 into master Dec 15, 2020
@Gargron Gargron deleted the fix-rtl-render branch December 15, 2020 11:57
@ahangarha
Copy link
Contributor

Great. Counting seconds to see this change on instances.

Thank you

@ahangarha
Copy link
Contributor

ahangarha commented Dec 21, 2020

I have noticed a little issue when the language is set to Persian (and for sure other RTL ones) which is making little issue with links starting with LTR characters.

This can be solved by adding direction: initial; to .reply-indicator__content p, .status__content p and set unicode-bidi: initial; for .reply-indicator__content a, .status__content a

Test this toot: https://mas.to/web/statuses/105419096662730287

on RTL layout

on LTR layout

and it seems we need this as well to make sure mentions would render properly:

.h-card {
    unicode-bidi: plaintext;
}

and this is the result in both RTL and LTR direction:

image

sum up:

These are the fix:

.reply-indicator__content p, .status__content p {
    direction: initial;
}

.reply-indicator__content a, .status__content a {
    unicode-bidi: initial;
}

.h-card {
    unicode-bidi: plaintext;
}

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.

None yet

4 participants