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.

Sign up for GitHub

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

Jump to bottom

Mechanism for detecting RTL inside components #773

Closed
claviska opened this issue Jun 1, 2022 · 4 comments
Closed

Mechanism for detecting RTL inside components #773

claviska opened this issue Jun 1, 2022 · 4 comments
Assignees
@claviska
Labels
feature Feature requests.

Comments

@claviska
Copy link
Member

claviska commented Jun 1, 2022 •
edited
Loading

In #768 we talked about the need for detecting RTL outside of CSS. This issue intends to track how we're going to do that.

Within CSS, this lets us use :dir(). The caveat is that, in JavaScript, we have to use Element.closest('[dir]') or something equivalent. I previously proposed a property called Element.currentDir to make this more efficient — give the issue a 👍 if you find it useful — but I still think the benefits of dir outweigh the cost. In fact, the logic can probably be rolled into a Reactive Controller and reused.

I recall seeing some clever ways to efficiently detect RTL, one using matchMedia() to test for :dir or something like that. This may be more efficient than DOM traversal.

Let's use this issue to explore the best way to do it.

Below are some outstanding issues that can be addressed by this. I'm reluctant to create separate tickets for them until we've established an acceptable solution for detecting RTL from inside a component's logic.

  • Range tooltip isn't positioned correctly when using RTL fixed in f6d3f79
  • Tab Group's active tab indicator doesn't align correctly when using RTL fixed in 70c97e2
@claviska claviska added the bug Things that aren't working right in the library. label Jun 1, 2022
@claviska claviska added this to the v2.0.0 release milestone Jun 1, 2022
@claviska claviska self-assigned this Jun 1, 2022
@claviska claviska pinned this issue Jun 1, 2022
@claviska claviska mentioned this issue Jun 1, 2022
Closed
@shaal
Copy link
Contributor

shaal commented Jun 1, 2022

This article has an interesting solution https://dev.to/madsstoumann/a-quick-guide-to-css-logical-properties-2fce

Things we should still fix -
Search&replace all (what a great surprise that there are only a few of these remain)

  • .style.transform = translateX
  • marginLeft, marginRight
  • margin-left, margin-right
  • padding-right, padding-left

with their Logical CSS equivalent.

Perhaps it would be useful to add a warning in the future, for any commit with these keywords?

@claviska
Copy link
Member Author

claviska commented Jun 7, 2022

This has been implemented in @shoelace-style/localize. It works the same way that lang currently works, which means the controller looks at the host element's dir attribute if it exists and falls back to <html dir>. For performance reasons, we don't allow ancestors other than <html> to control directionality.

Some components, such as Range and Tab Group, have already been updated to use it. Additional RTL bugs should be filed as new issues so we can track them accordingly.

@claviska claviska closed this as completed Jun 7, 2022
@claviska claviska unpinned this issue Jun 7, 2022
@claviska claviska added feature Feature requests. and removed bug Things that aren't working right in the library. labels Jun 7, 2022
@asmanp2012
Copy link
Contributor

asmanp2012 commented Jun 9, 2022 •
edited
Loading

This has been implemented in @shoelace-style/localize. It works the same way that lang currently works, which means the controller looks at the host element's dir attribute if it exists and falls back to <html dir>. For performance reasons, we don't allow ancestors other than <html> to control directionality.

Some components, such as Range and Tab Group, have already been updated to use it. Additional RTL bugs should be filed as new issues so we can track them accordingly.

Today I discussed this with @AliMD he believes this shouldn't be controlled with localizing package I searched about it and found getComputedStyle(this) that returns all applied CSS on the element and maybe its better to we use that to know the direction of the component.

Too I checked Can I use, and this has good support after 2012.

If you( @claviska ) agree with that tell me to apply it to the code.

@claviska
Copy link
Member Author

claviska commented Jun 9, 2022

It boils down to performance. I explored this extensively for lang and chose to handle dir the same way because they share the exact same problem: there's no performant nor reliable way to determine an ancestor's lang or dir attribute, especially when you're inside a shadow root.

You can read more about this decision here: #419 (comment)

TL;DR — it's expensive to do DOM traversal to find the closest element with a dir or lang attribute, and there's no other reliable way to get this information. I consider this a gap in the platform. This very issue led to my proposal for Element.currentDir and Element.currentLang.

Based on feedback from the RFC, we decided the limitation resulting from caring only about lang on <html> and/or the target component was acceptable. The reasoning was the large majority of use cases only render one translation per page, which can be handled efficiently with <html lang="...">.

We still wanted to support multiple translations, but due to the aforementioned issue, we decided to ignore ancestors and require users to explicitly add the lang attribute to any component that differs from the document's language. By design, it does not work for use cases like this:

<html lang="en">
  <div lang="fr">
    <sl-button>Whoops, I'm still in English</sl-button>
  </div>
</html>

This brings us back to the concept of directionality, which is very similar. In fact, the proposed getComputedStyle() solution is very much like one I explored early on for languages.

https://codepen.io/claviska/pen/VwWejBz

The problem with getComputedStyle() is it's expensive because it triggers a reflow and that leads to layout thrashing. There are numerous checks in various components to determine directionality, and we don't want them triggering reflows frequently.

We also care about reactivity. If the direction changes at any point, either on <html> or the component, we want the component to update. Using the current approach, we can do this efficiently with a highly targeted Mutation Observer on <html> and reactive lang properties on each component.

As far as I can tell, we wouldn't be able to do this using getComputedStyle() — we'd be stuck with the directionality that was set when the component first rendered.

If you search the source for the following string, you'll see it's used in a number of places:

this.localize.dir()

Sometimes it's used to toggle a RTL class in a template. Other times it's used to respond to keyboard and mouse events. Calling getComputedStyle() inside these event handlers, especially drag events, will cause significant lag.

Based on that, I don't think moving to getComputedStyle() is the right choice here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@claviska claviska

Labels
feature Feature requests.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@shaal @claviska @asmanp2012