Widgets: Add RTL support #1682

AhmedMustafa · 2016-03-04T13:06:46Z

A starting point for UI mirroring support in jquery-ui.

mention-bot · 2016-03-04T13:06:48Z

By analyzing the blame information on this pull request, we identified @scottgonzalez, @jzaefferer and @arschmitz to be potential reviewers

jquerybot · 2016-03-04T13:06:58Z

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.

If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

scottgonzalez · 2016-03-04T14:21:10Z

themes/base/accordion.css

+
+/* RTL support */
+.ui-accordion-rtl {
+	direction: rtl;


Why does this need to exist? The direction is already set or we wouldn't be adding this class.

That is correct for the current case. But it would be required if there is a parent widget with dir ='rtl' & its child widget exists somewhere in the DOM with no dirattribute (e.g. autocomplete & menu).
I know it doesn't make any sense her, but I added it in case we need it at some point & for the mirroring support to be consistent across all the widgets.

We would never need this for accordion, so it doesn't make sense to add it.

arschmitz · 2016-03-04T14:51:03Z

If i'm not mistaken, we don't actually need any changes to the widget or css here. Unless i'm missing something the Demo works with no changes except adding the demo.

I reverted all changes except adding the demo and it does not change the display at all.

AhmedMustafa · 2016-03-05T00:05:50Z

@arschmitz Accordion & some other widgets don't require extensive modifications to support ui mirroring. However, a little changes should be made & in this case we have to make sure that the icons displayed correctly in rtl layout.

AhmedMustafa · 2016-03-06T18:40:23Z

Based on your comments, Accordion doesn't need special handling to support ui-mirroring (except a minor css change).
So, I did the required changes & I am going to step to the next widget.

scottgonzalez · 2016-03-08T15:22:36Z

ui/widgets/autocomplete.js

@@ -539,9 +542,10 @@ $.widget( "ui.autocomplete", {
 		// Size and position menu
 		ul.show();
 		this._resizeMenu();
+		var pos = this.isRtl() ? { my: "right top", at: "right bottom", collision: "none" } : this.options.position;


This can't be hard-coded like this. It makes the option impossible to use.

Should I create _getPositionRtl: function( position ) which replace every left with right & vice versa when this.isRtl() == true ?
If so, should I put it in widget.js as it might be used within others widgets ?

No, you should assume the user has the option set to what they actually want.

If we do anything with position, we would change the default to null, and on create, we would set a value based on the direction if the value is still null.

That would be consistent with our null value options approach in other places

The browser will not change that, but someone have to change it to be acceptable for bidi-users.
You are suggesting that the user should make the changes himself. But I prefer the changes to be made by the library.
So instead of letting every user writing another version of his app for rtl-layout, the library will do that for him by just setting dir=rtl. And he still have the ability to disable or override that behavior.

I agree with you, we have to respect the user's choice & that is what we actually do in ltr-layout.
The user will always get what he expects in ltr-layout. But when he choose to enable mirroring(by setting dir=rtl), the library should do the magic for him with no or with minimal modification from his side.
The pros of this approach is:
1- The user don't have to write another app or make any major changes to enable ui mirroring.
2- Users don't have to have a deep knowledge about mirroring to make their apps mirrored correctly.
3- The user still have the ability to disable or override that behavoir.

The browser will not change that, but someone have to change it to be acceptable for bidi-users.

That not always true you can choose to have something in the same position regardless of your layout this is application specific and gives developers a way to force position regardless of direction if thats what their app requires.

The user still have the ability to disable or override that behavoir.

Please explain to me with your approach how you would set the position to be the same REGARDLESS of direction. As far as i can tell this is not possible with your approach. Also if someone was making a RTL first app do you really expect them to set position left when they really want right? this seems completely wrong an counter intuitive and will only lead to confusion.

It's also very easy to write a plugin that does the mirroring for you. Widget functionality is never a black and white choice of the built-in or implemented as a one-off for every single user. Creating a position mirroring extension would allow users to have the mirroring logic without writing it themselves, but I still don't agree that automatic mirroring for position is correct.

You are right. I should revert it back.

jzaefferer · 2016-03-09T17:14:01Z

Btw. the CLA check failed because the commit author (AhmedMustafa) doesn't match the CLA signature (Ahmed Mohamed Mustafa).

AhmedMustafa · 2016-03-27T11:25:20Z

@arschmitz @scottgonzalez I am done here & waiting your feedback.

ashensis · 2016-06-30T17:07:39Z

I found some GUI mirroring issues while preparing demo for https://bugs.jqueryui.com/ticket/14477.
Specifically in demos/widget/textDir.html, the Menu, Select menu and Tabs doesn't seemto be properly mirrorred
in right column that has wrapping 'div' element with "rtl" dir attribute that apparently should suffice
to ensure the proper GUI mirroring of corresponding widgets. 9as illustrated by attached snapshot GUI_Mirroring_JQuery_widgetspng.png

ashensis · 2016-06-30T17:26:31Z

AhmedMustafa · 2017-10-26T12:25:25Z

It has been a while since I receive any feedback from your side regarding this PR.
If you still convinced that some widgets don't need such a support and that the library is flexible enough to render correctly in RTL pages, then it will be great to mention that somewhere in jQueryUI documentation. This will provide the bidirectional-languages users with the support they need to mirror their pages easily & quickly.
For example, This could be part of the i18n section.

AhmedMustafa · 2017-10-26T12:28:03Z

And for the other widgets (e.g. The Slider), I think you have to consider reviewing the code I have submitted above.
ref https://bugs.jqueryui.com/ticket/10658

mrashad2017 · 2017-11-06T16:38:03Z

Dears,
Based on AhmedMustafa comment above can you advise where to add RTL documentation. This will provide bidirectional-languages users with the support they need to mirror their pages easily & quickly. And will avoid to do the same effort and same investigation.

mrashad2017 · 2017-11-15T08:47:01Z

Dears @arschmitz @jzaefferer ,

We do some investigation in mirroring, and we have valuable feedback to help the bidirectional-languages users with the support they need to mirror their pages easily & quickly.

This will minimize any effort to do mirroring as we can support them with code details.

Appreciate your guide to make the document in the proper format and the proper place, and may be after your review, you can reference to it in case of mirroring for bidirectional-languages users.

Attached file contain sample of our change to make mirroring
jQueryUI_mirror_document.docx

dmethvin · 2019-08-01T00:09:59Z

Closing as a stale branch.

Accordion: Adding RTL support

2c9539a

jquerybot added the CLA: Error label Mar 4, 2016

scottgonzalez reviewed Mar 4, 2016
View reviewed changes

Accordion: updating rtl support in ui.accordion

34004ae

Autocomplete: Adding ui-mirroring support

8f86527

scottgonzalez reviewed Mar 8, 2016
View reviewed changes

AhmedMustafa added 13 commits March 15, 2016 15:39

Autocomplete: ui mirroring support

8ba1210

Button: Adding ui mirroring support

1946111

Controlgroup: Adding ui mirroring support

1a47821

Checkboxradio: Adding rtl demo

6a00fb0

Dialog: Adding ui mirroring support

23cc693

Draggable: Adding ui mirroring demo

543aaea

Droppable: Adding ui mirroring demo

e3581d0

Menu: Adding ui mirroring support

fc01eb6

Progressbar: Adding ui mirroring demo

43b3c2f

Selectable: Adding ui mirroring demo

d1f0301

Selectmenu: Adding ui mirroring support

10deed0

Spinner: Adding ui mirroring support

6999e35

Tabs: Adding ui mirroring support

0a71279

AhmedMustafa added 2 commits March 20, 2016 23:15

Slider: Adding ui mirroring support

c92c299

Resizable: Adding ui mirroring support

1a1f8b7

scottgonzalez mentioned this pull request Mar 30, 2016

Spinner: Support RTL #1412

Closed

arschmitz changed the title ~~Accordion: Adding RTL support~~ Widgets: Add RTL support Jun 22, 2016

arschmitz mentioned this pull request Jun 22, 2016

Right-to-Left Reading Order Support jquery-archive/jquery-mobile#5167

Closed

scottgonzalez mentioned this pull request Jun 30, 2016

jquery-ui:Base text direction support #1715

Open

JSFOwner removed the CLA: Error label Nov 5, 2016

dmethvin closed this Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Widgets: Add RTL support #1682

Widgets: Add RTL support #1682

AhmedMustafa commented Mar 4, 2016

mention-bot commented Mar 4, 2016

jquerybot commented Mar 4, 2016

scottgonzalez Mar 4, 2016

AhmedMustafa Mar 4, 2016

scottgonzalez Mar 6, 2016

arschmitz commented Mar 4, 2016

AhmedMustafa commented Mar 5, 2016

AhmedMustafa commented Mar 6, 2016

scottgonzalez Mar 8, 2016

AhmedMustafa Mar 10, 2016

scottgonzalez Mar 10, 2016

scottgonzalez Mar 10, 2016

arschmitz Mar 10, 2016

AhmedMustafa Mar 14, 2016

AhmedMustafa Mar 14, 2016

arschmitz Mar 14, 2016

scottgonzalez Mar 14, 2016

AhmedMustafa Mar 15, 2016

jzaefferer commented Mar 9, 2016

AhmedMustafa commented Mar 27, 2016

ashensis commented Jun 30, 2016 •

edited

Loading

ashensis commented Jun 30, 2016

AhmedMustafa commented Oct 26, 2017

AhmedMustafa commented Oct 26, 2017 •

edited

Loading

mrashad2017 commented Nov 6, 2017 •

edited

Loading

mrashad2017 commented Nov 15, 2017

dmethvin commented Aug 1, 2019

Widgets: Add RTL support #1682

Widgets: Add RTL support #1682

Conversation

AhmedMustafa commented Mar 4, 2016

mention-bot commented Mar 4, 2016

jquerybot commented Mar 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arschmitz commented Mar 4, 2016

AhmedMustafa commented Mar 5, 2016

AhmedMustafa commented Mar 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzaefferer commented Mar 9, 2016

AhmedMustafa commented Mar 27, 2016

ashensis commented Jun 30, 2016 • edited Loading

ashensis commented Jun 30, 2016

AhmedMustafa commented Oct 26, 2017

AhmedMustafa commented Oct 26, 2017 • edited Loading

mrashad2017 commented Nov 6, 2017 • edited Loading

mrashad2017 commented Nov 15, 2017

dmethvin commented Aug 1, 2019

ashensis commented Jun 30, 2016 •

edited

Loading

AhmedMustafa commented Oct 26, 2017 •

edited

Loading

mrashad2017 commented Nov 6, 2017 •

edited

Loading