OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.37k stars 2.38k forks source link

Markdown rich editor corrupts Arabic text in OC 1.4 #12172

Open dodyg opened 2 years ago

dodyg commented 2 years ago

Describe the bug

The Arabic text in this rich editor is corrupted

mde-markdown

This is the text on the same exact field without the rich editor

mde-markdown-1

Skrypt commented 2 years ago

Have you tried directly with the same text and the markdown editor on their website demo?

dodyg commented 2 years ago

Good idea let me try it

dodyg commented 2 years ago

This is the corrupted data in EasyMDE

corrupt

This is the actual rendering

https://try.orchardcore.net/wmuhsxod/blog/post-1

The website is https://try.orchardcore.net/wmuhsxod Username: admin Password: ?B002t-L

If anyone wants to replicate

dodyg commented 2 years ago

برنامج مؤسسة ساويرس للحصول على الدبلوم الفني الفندقي من المدرسة الألمانية الفندقية بالجونة

There is no problem with the EasyMDE itself (https://easy-markdown-editor.tk/)

easy-mark

But at OC14, the text becomes corrupted easy-mark-2

dodyg commented 2 years ago

The editor in OC 1.4 is the same version as https://easy-markdown-editor.tk/ easy-mark-3

Skrypt commented 2 years ago

I see the text is reversed in the editor. That would be a styling issue.

Skrypt commented 2 years ago

I just found this repository which probably indicates that EasyMDE has issues with RTL. https://github.com/imAbdelhadi/easymde-rtl

Skrypt commented 2 years ago

@dodyg I think we could bundle the RTL version in OC and switch to it based on CultureInfo RTL. Feel free to open a pull request.

hishamco commented 2 years ago

Seems I come late to fix the Arabic issue ;) from the first time I saw the text, the letters in the words have been revered, so we could fix this as @Skrypt suggested before

@dodyg is it working before OC 1.4 or the issue from the beginning because no one tried Arabic text ;)

dodyg commented 2 years ago

@hishamco I can't remember but the site was already English/Arabic since OC 1.0 and this was the first complain regarding reverse text.

dodyg commented 2 years ago

OC-3

@hishamco OK this is another site of mine running on OC 1.3. It works fine here.

Skrypt commented 2 years ago

Maybe a BS 5 upgrade regression. If this can be fixed by changing only styles then it will be better. I didn't find where it is reversing the text.

hishamco commented 2 years ago

I might look into this

dodyg commented 2 years ago

The culprit is in TheAdmin.css

style-1 style-2

dodyg commented 2 years ago

I found the problem.

ltr here problem-1

Changing it to rtl fix it. problem-2

dodyg commented 2 years ago

@hishamco

This is most likely the problematic css in TheAdmin.css file. There is a comment about rtl:ignore. I am not sure what the reason is. Maybe people more familiar with the theme can review this.

html[data-theme=default] pre,
html[data-theme=default] code,
html[data-theme=default] kbd,
html[data-theme=default] samp {
  font-family: var(--bs-font-monospace);
  font-size: 1em;
  direction: ltr /* rtl:ignore */;
  unicode-bidi: bidi-override;
}

https://raw.githubusercontent.com/OrchardCMS/OrchardCore/main/src/OrchardCore.Themes/TheAdmin/wwwroot/css/TheAdmin.css

hishamco commented 2 years ago

I think from the style author, unless @Skrypt did it for a reason

dodyg commented 2 years ago

bidi

The direction is really not necessary if the unicode-bidi: bidi-override; is removed.

  direction: ltr /* rtl:ignore */;
  unicode-bidi: bidi-override;
dodyg commented 2 years ago

This thing is like hydra. Once the unicode-bidi removed, the codemirror.min.css needs to be dealt with.

code-mirror

dodyg commented 2 years ago

One possible solution is changing

html[data-theme=default] pre,
html[data-theme=default] code,
html[data-theme=default] kbd,
html[data-theme=default] samp {
  font-family: var(--bs-font-monospace);
  font-size: 1em;
  direction: ltr /* rtl:ignore */;
  unicode-bidi: bidi-override;
}

to


html[data-theme=default] pre,
html[data-theme=default] code,
html[data-theme=default] kbd,
html[data-theme=default] samp {
  font-family: var(--bs-font-monospace);
  font-size: 1em;
}

html[dir=ltr][data-theme=default] pre,
html[dir=ltr][data-theme=default] code,
html[dir=ltr][data-theme=default] kbd,
html[dir=ltr][data-theme=default] samp {
    direction: ltr !important;
}

html[dir=rtl][data-theme=default] pre,
html[dir=rtl][data-theme=default] code,
html[dir=rtl][data-theme=default] kbd,
html[dir=rtl][data-theme=default] samp {
    direction: rtl !important;
}
dodyg commented 2 years ago

Above solution works on Microsoft Edge

edge

Firefox firefox

but not Google Chrome

chrome

Skrypt commented 2 years ago

Have you tried removing only the /*rtl:ignore*/ comment? Also, make sure you test with different RTL cultures afterward because it seems like it works for other people.

dodyg commented 2 years ago

Hebrew is also reversed

hebrew-1 hebrew-2

Site: https://try.orchardcore.net/2z5kxzxk username: admin password: _Y73koqP

dodyg commented 2 years ago

This is how the Hebrew posting supposed to be. I had to modify the RTL directly.

supposed-to-be

I am not sure what's the purpose of removing /*rtl:ignore*/ because it doesn't really show up at the inspector.

dodyg commented 2 years ago

I don't understand why the unicode-bidi: bidi-override; is required. Because if you remove this, the direction can be removed.


html[data-theme=default] pre,
html[data-theme=default] code,
html[data-theme=default] kbd,
html[data-theme=default] samp {
  font-family: var(--bs-font-monospace);
  font-size: 1em;
  direction: ltr /* rtl:ignore */;
  unicode-bidi: bidi-override;
}
Skrypt commented 2 years ago

Good question. The issue is that it needs to be tested with more than just the Markdown editor. These classes are applied to base HTML tags. That means that at some point we decided to override the direction of these texts and also remove the unicode-bidirectional ability and force it to be LTR.

So, the proper fix would be to use a CSS class that wraps around the Markdown editor only and that will override these styles that affect it. And also, keep these base HTML styles because they seems to be appropriate. You generally don't want to reverse a <code></code> example in a page.

dodyg commented 2 years ago

Yeah that makes sense. Let me tinker with this later so it only targets EasyMDE.

Piedone commented 4 months ago

@hishamco could you please check this out?

hishamco commented 4 months ago

Sure, also I need to check RTL in the monacco editor

Piedone commented 4 months ago

Thank you!

hishamco commented 4 months ago

If there are RTL issues you can assign them to me directly

Piedone commented 4 months ago

OK then :).

dodyg commented 2 months ago

Is there any chance of this getting fixed?

hishamco commented 2 months ago

@dodyg is this still a valid issue in the latest bits? If yes I need to check this one before 2.0.0 release