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

Fix relative icon position with dir='rtl' #1021

Merged
merged 1 commit into from
Oct 12, 2020
Merged

Fix relative icon position with dir='rtl' #1021

merged 1 commit into from
Oct 12, 2020

Conversation

varjolintu
Copy link
Member

@varjolintu varjolintu commented Sep 26, 2020 •
edited

#840 broke the icon position with right-to-left pages.

Also adds text-align: right and dir=rtl detection to Icon constructor.

Fixes #1019.

@varjolintu varjolintu added the bug label Sep 26, 2020
@varjolintu varjolintu added this to the 1.7.2 milestone Sep 26, 2020
@varjolintu varjolintu mentioned this pull request Sep 27, 2020
Closed
@ghost
Copy link

ghost commented Sep 27, 2020 •
edited by ghost

I've tested this on several RTL login pages and it does much better than current master. 👍

The current code will fail in the (weird case) of <body dir=rtl> with text-input:left: it would position the icon on the left, and text entry will also begin on the left side of the input.

Remember that RTL is actually about the order in which characters are displayed and only implicitly about text alignment, but the correct icon position depends only on the text-alignment.

I've done some tests about mechanisms and relative priorities of doing RTL with HTML/CSS.
Note that I've seen actual examples of each of these on one or more of the high-traffic sites
I've checked.

From highest priority to lowest:

  1. text-align value
  2. dir attribute on input element
  3. dir attribute on body
  4. dir attribute on html

For example, one site has input dir='ltr' with text-align:right with the result that the text is right-aligned. So the current PR works only because it doesn't check for dir attribute on the input element itself (another issue). If it did and than ignored the text-align like it does with <body dir=_>, it would position the icon in the wrong place.

@droidmonkey
Copy link
Member

Nothing like consistency!

@varjolintu
Copy link
Member Author

varjolintu commented Sep 27, 2020 •
edited

@anon324875693 One more issue: if the input field has a parent div with dir=rtl that is not handler either. That must be fixed too.

EDIT: But this is gonna cause multiple number of getComputedStyle() calls, so it's not a wise decision to traverse all the parents for each icon. Lets skip that for now.

@ghost
Copy link

ghost commented Sep 27, 2020 •
edited by ghost

Lets skip that for now.

I think that's reasonable. Though again, I believe you'd only need to do the parent traversal once per input since directions/alignment should be fixed after load.

By and large all the RTL sites I've seen so far always have an explicit text-align property on the input element. probably to protect against any cross-browser weirdness.

@ghost
Copy link

ghost commented Oct 11, 2020

Merge?

@varjolintu varjolintu merged commit 657783c into develop Oct 12, 2020
@varjolintu varjolintu deleted the fix/relative_icon_position branch October 12, 2020 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droidmonkey droidmonkey droidmonkey approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
1.7.2
Development

Successfully merging this pull request may close these issues.

Icon obstructs input text on RTL forms that rely on text-align:right
2 participants
@varjolintu @droidmonkey