Closed fshafiee closed 3 years ago
Hey, this looks good! One question I have in particular with the link back to outline/outline#2201 is how this prop would be controlled. I think the ideal would be:
rtl
prop to standard dir
and default it to "auto"Hi @tommoor, thanks for reviewing this.
Change rtl prop to standard dir and default it to "auto"
Sure. I will change the
Update: It's done. Keep in mind that the effect of rtl
value to dir
. I was not sure which way you guys would lean.dir=auto
is minimal (identical IMO) to ltr
. However, since it's a supported value in the standard HTML, it's a good practice to support it and clarify the behavior.
Use the calculated dir value for the first text node to set the rtl ui for the rest of the editor
The work I have in progress, adds a direction
field to document resource, which can be toggled via a key combination (Ctrl+Shift+L
at the moment, open for suggestions). I have already done most of the work. A few more tests, adding the entry to Keymap legend, and I'll be done. Do you think this is something you can accept for now? Since I have no idea how stable it would be in its current state, keeping it low key and optional has the added benefit of protecting current behavior of the application.
About your proposal, it's nice if we could set the direction automatically, but we still need a way to manually override it. In mixed language documents (technical subjects in specific) it's not uncommon to start a paragraph with a technical term in English but continue the sentence in another language. This throws off some browsers and the result is less than ideal in my experience. The situation gets worse if the input contains numeric letters. Which is why I believe the next order of business in terms of RTL support should be per-node direction control.
There are also other approaches we could entertain. For example, we could hint the user about the option once we have a guess for RTL documents. The hinting part and promoting the feature happens automatically, but changing the direction remains optional. The hint could include a button and/or key-binding for educating users for future instances.
Just to state it straight up – I would like to get a version of this merged both here and in Outline, but I don't have any experience of writing RTL so will be totally relying on the community (you) to direct. I am very protective over the user experience in Outline however.
The situation gets worse if the input contains numeric letters. Which is why I believe the next order of business in terms of RTL support should be per-node direction control.
This is going to be very difficult as documents are stored as Markdown, it really feels like the browser auto-detect is the way to achieve this as auto would allow each paragraph to flow a different direction without any database migrations or additional fields stored.
I do understand that this isn't always perfect and will sometimes guess incorrectly as you mention but that occasional tradeoff seems like something manageable to get the functionality with so few changes in the Outline codebase. I would have thought that folks writing RTL are quite used to seeing that sort of behavior and have workarounds?
e.g.
I am very protective over the user experience in Outline however.
@tommoor, I respect your position. ✌🏻
Don't get me wrong. The part you quoted, was me, thinking ahead and thinking out loud. I have no intention on following up on it at the moment. It just seemed like a missing feature on Outline compared to other editors (which RTL users are familiar with). Sorry if I wasn't clear about it.
To address your concerns, the changes I have made so far, only relies on browser to handle the text direction correctly, as is your preference. It's just a single CSS attribute. (see this)
I would like to get a version of this merged both here and in Outline
I have already opened a draft PR on Outline (outline/outline#2213) to give you a better idea around the scope of the changes. I'd appreciate it if you could review that one as well.
the changes I have made so far, only relies on browser to handle the text direction correctly, as is your preference
Do you think it's possible to have the editor position the controls correctly when dir="auto"
?
We can use
getComputedStyle(document.getElementById('foo')).direction
to get the calculated text direction
I'll look into it. Thanks for the pointer. 👍🏻
Hey, @tommoor.
I managed to position the editor controls based on browser computed value with your tip.
However, it breaks up if doc starts with non-RTL characters.
With dir='auto'
:
With dir='rtl'
:
With dir='auto'
:
This issue is inconsequential for the package itself, but should be taken into consideration for Outline integration. As I said, it's not uncommon for technical and scientific articles to be mixed.
This issue is inconsequential for the package itself, but should be taken into consideration for Outline integration. As I said, it's not uncommon for technical and scientific articles to be mixed.
Agreed, it literally uses the first strongly-typed character so it makes sense in your example that the layout would be incorrect. I am thinking of leaning towards an initial integration that just uses auto
despite this limitation as it'll vastly reduce the changes needed and avoid a database migration as you have laid out in the Outline PR. Folks can just tweak their titles to get the correct behavior.
Oh, it's also worth noting that the title and body are separate fields in Outline, the title doesn't use this editor.
Seeing a couple of odd behaviors in RTL mode that need addressing…
https://user-images.githubusercontent.com/380914/121838390-90b1ef00-cc8c-11eb-9f77-b60b43ac2215.mp4
I am thinking of leaning towards an initial integration that just uses auto despite this limitation as it'll vastly reduce the changes needed and avoid a database migration as you have laid out in the Outline PR. Folks can just tweak their titles to get the correct behavior.
I'll open a separate PR and limit the changes to UI-only fixes.
Oh, it's also worth noting that the title and body are separate fields in Outline, the title doesn't use this editor.
Yup. I am aware of that. Title and navigation tree need to be fixed separately.
Seeing a couple of odd behaviors in RTL mode that need addressing…
I'll look into it. Thanks for pointing them out.
@tommoor I basically had to get rid of the bidi CSS directive in order to fix the block menu position. It's no longer required since dir=auto
allows the browser to handle it.
About the cursor position, the issue seems to be limited to Chrome. I was testing the changes on Safari and Firefox and these two had no issues. Chrome behavior is a bit random. In some scenarios it's positioned correctly. I think it is caused by some inconsistencies in Chrome DOM APIs and might resolve itself in upcoming versions. Do you think it's worth digging deeper into? I worry it might be a rabbit hole and require weird hacks to get rid of.
Agreed that it seems like a browser bug, I've come across a few with the intricacies of the non-editable areas of the editor. contenteditable
is certainly forever full of edge cases, we could live with it for now.
What are your thoughts on adding the dir
value to the individual paragraph tags? If we pass the prop through and apply it to each rendered paragraph then it's possible to have mixed content with dir=auto
, I'm not sure if that's desirable but it seems like it would be?
What are your thoughts on adding the dir value to the individual paragraph tags? If we pass the prop through and apply it to each rendered paragraph then it's possible to have mixed content with dir=auto, I'm not sure if that's desirable but it seems like it would be?
Seems plausible. However, it's a bit more complex than might initially appear. Remember the main challenge is to correctly calculate CSS properties, as text direction is handled by the browser. Now, if we want to delegate the logic to each node, there are a couple of things to consider:
Given how CSS rules are structured at the moment, which is one big styled-component, we should either:
Position of the block menu becomes conditional on direction of the active node vs. the editor.
I consider both of these issues to be design decisions, and there are more than one solution for each. I believe it's better and more efficient if you or other maintainers who have the vision, authority, and experience with the code to handle it.
Enhances the editor to work with RTL documents. This is not a full fledged per-node direction support. It just allows the whole rendering to be compatible with the direction of the main, which should cover most of the use-cases for now. Should the need for per-paragraph direction override arise, we still need the elements of the editor to be rendered respective to the direction of the primary language, which means that this is a necessary steps towards that feature.
If this PR is accepted, it will be followed by another one for the outline app to bring RTL support to the platform. Some changes and tweaks to the API and the web app are required to fully fix the issues with RTL/mixed content presentation (which is already a work in progress).
Fixes outline/outline#2201