jquery-ui:Base text direction support #1715

ashensis · 2016-06-30T15:10:40Z

As per discussion in https://bugs.jqueryui.com/ticket/14477, current code submit contains Base text direction support for UI widgets.
New option is introduced for relevant widgets 'textDir' with values:
'ltr' - Left-to-right text direction
'rtl' - Right-to-left text direction
'auto' - Contextual text direction that gets resolved to either 'ltr' or 'rtl' according to first strong character ('rtl' if it is Arabic/Hebrew character, 'ltr' otherwise).
Text direction option shouldn't affect the mirroring aspects of widgets having assimmetric GUI, it only impacts the text direction of contained text.
The corresponding demo textDir.html illustrates the issue for all relevant widgets.
When current approach will be approved (or modified according reviewer remaarks) documentation and unitests will be provided.

mention-bot · 2016-06-30T15:10:41Z

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

scottgonzalez · 2016-06-30T15:13:16Z

Thanks. See also #1682.

Just FYI: We won't have any time to discuss the approach until after our releases are done, which is why #1682 has been sitting for a long time.

tomerm · 2016-06-30T20:41:12Z

No problem. Will be waiting for your feedback / comments.

One clarification with your kind permission:

#1682 is about fixing issues with GUI mirroring (relevant for several widgets). GUI mirroring is strongly associated with translation of UI to Arabic / Hebrew. At the same time, GUI mirroring is mostly concerned with graphical layout of screen and widget.

As opposed to that, this specific PR is concerned with text only (which can appear as part of widget). Thus this PR is completely orthogonal to GUI direction and #1682 .

Just FYI. My team and our colleagues from Egypt Bidi center (who authored #1682) work together to address variety of Bidi requirements in open source tools / libraries.

arschmitz · 2016-07-01T17:38:03Z

I have not reviewed this at all yet simply read the discription. Just one minor thing i noticed from that is any time we have an "auto" option like this that determines the value used based on DOM etc, we always use the special value null to denote it. so in this case the 3 values should be "ltr", "rtl", and null. null should be the default value. When the value null is used and rtl or ltr chosen based on markup during init, the option value should be set as well. In this way when a developer checks the value of the option it should always return rtl or ltr and never null

tomerm · 2017-05-15T14:56:21Z

@scottgonzalez may I kindly inquire which release are we waiting for to start the review ? I believe 3.2 was out in March 2017.

scottgonzalez · 2017-05-24T14:54:37Z

Let's focus on a single widget to have our discussions. I'll pick button since it's the first widget in the textDir.html demo and is fairly simple. Please explain exactly what the button widget is accounting for with text direction that isn't already handled natively by dir="rtl".

tomerm · 2017-05-25T19:37:23Z

@scottgonzalez button is not the best example to illustrate the benefit of textDir parameter. This is mostly since button has horizontally symmetrical graphics (just a rectangular area without any shade) and thus it looks (from graphical perspective) exactly the same with both LTR and RTL directions. However even for the button there is a minor benefit. Please observe that textDir is meant to affect text only. Currently if you specify dir="ltr" or dir="rtl" for button you don't cover auto direction (aka contextual or first stong) for text label displayed as part of the button. You can specify dir="auto" for button but this won't work at least not on IE. Thus having textDir even in the case of button has some benefit.

In general textDir has striking benefit for widgets which are horizontally asymmetrical. For example, combo box, list box (with scroll bar), check box, tree, grid etc. When you want to flip horizontally such widgets you usually specify dir="ltr" or dir="rtl". However, this by itself does not allow you to enforce text direction for text displayed in such widgets independently from GUI direction. For example if you specify dir="rtl" for combo box, all text inside such combo box will be displayed with RTL direction. However if you wish this text to be displayed with LTR direction you have no such option. Having textDir parameter allows you to control text direction of text inside a widget independently from direction of widget graphics.

For the sake of example, please observe the behavior of native controls on iOS / Android platforms. Text in such controls is displayed by default with textDir = "auto" regardless of direction of control's graphics (dir ="ltr" or dir ="rtl"). By the way majority of desktop technologies (including native mobile libraries) allow to control direction of text independently from widget (or GUI) direction. For example:

Direction of GUI (or widget) is called by Android layoutDirection (https://developer.android.com/reference/android/view/View.html#setLayoutDirection%28int%29) . It is equivalent to HTML dir specified for a widget.
Text direction (for text displayed inside a widget) is called by Android textDirection (https://developer.android.com/reference/android/view/View.html#setTextDirection%28int%29). It is equivalent to HTML dir specified on the text inline element itself.

scottgonzalez · 2017-06-05T17:24:08Z

Thanks for the explanation on the two different direction settings. Now that I have a better understanding of the specific goal trying to be accomplished, I will take another look at the changes.

scottgonzalez

Initial round of review.

scottgonzalez · 2017-06-05T17:41:07Z

demos/widget/textDir.html

@@ -0,0 +1,283 @@
+<!doctype html>


This demo will need to be removed since it's not actually a demo of the widget factory.

scottgonzalez · 2017-06-05T17:42:01Z

tests/unit/tooltip/common.js

@@ -19,6 +19,7 @@ common.testWidget( "tooltip", {
 		},
 		show: true,
 		track: false,
+		textDir: null,


These options are alphabetized. All files will need to be updated for the correct order.

scottgonzalez · 2017-06-05T17:42:10Z

ui/widget.js

@@ -280,6 +280,8 @@ $.Widget.prototype = {
 		classes: {},
 		disabled: false,

+//		textDir: null,


Removed commented out code.

scottgonzalez · 2017-06-05T17:42:37Z

ui/widget.js

+
+	_getTextDir: function( text ) {
+		if ( this.options.textDir === "auto" ) {
+			var matcher = /[A-Za-z\u05d0-\u065f\u066a-\u06ef\u06fa-\u07ff\ufb1d-\ufdff\ufe70-\ufefc]/.exec( text );


Please add a comment about which set of characters this is searching for.

scottgonzalez · 2017-06-05T17:42:52Z

ui/widget.js

+		}
+		return this.options.textDir;
+	},
+	_applyTextDir: function( param ) {


Blank line between methods.

scottgonzalez · 2017-06-05T17:52:09Z

ui/widgets/autocomplete.js

+			if ( this.options.textDir === "auto" ) {
+				var that = this;
+				this.element.bind( "keyup", function() {
+					var fld = $( this );


s/fld/element/

But that's not needed once you switch to ._on() anyway.

Indeed, changed

scottgonzalez · 2017-06-05T17:54:43Z

ui/widgets/button.js

@@ -88,6 +89,10 @@ $.widget( "ui.button", {

 		this.hasTitle = !!this.element.attr( "title" );

+		if ( this.options.textDir ) {
+			this.options.label = this._applyTextDir( this.options.label );


Doesn't this break HTML inside the button?

As a matter of fact inserted UCC constitutes just text node in HTML markup and doesn't break it, while having desirable impact.
However I made changes accounting to possibility of HTML content of button widget and now the element 'direction' style gets changed instead of insertion of directional UCC.

scottgonzalez · 2017-06-05T17:56:02Z

ui/widgets/button.js

@@ -242,6 +248,10 @@ $.widget( "ui.button", {
 				this._updateTooltip();
 		}

+		if ( this.options.textDir && ( key === "label" || key === "textDir" ) ) {


What about handling when textDir changes? This seems like a place where we probably need to leverage _setOptions() instead of _setOption().

Moved code to '_setOptions'

scottgonzalez · 2017-06-05T17:59:38Z

ui/widgets/controlgroup.js

+					var dir = that.options.textDir ?
+						"dir='" + that._getTextDir( element.text() ) + "' " : "";
+
+					element.contents().wrapAll


Don't break between method names and their invoking parens.

scottgonzalez · 2017-06-05T18:03:45Z

ui/widgets/tooltip.js

@@ -265,6 +266,9 @@ $.widget( "ui.tooltip", {
 		tooltipData = this._find( target );
 		if ( tooltipData ) {
 			tooltipData.tooltip.find( ".ui-tooltip-content" ).html( content );
+			if ( this.options.textDir ) {
+				this._applyTextDir( tooltipData.tooltip.find( ".ui-tooltip-content" ) );


Doesn't this break HTML inside the tooltip?

By no means. In this case the tooltip.find returns JQuery object and '_applyTextDir' sets its 'direction' style

scottgonzalez · 2017-06-12T18:22:33Z

@ashensis You can just push updates to this branch and keep the PR open.

adir01 · 2017-06-13T13:02:12Z

@scottgonzalez Can you please reopen this PR ?

jsf-clabot · 2017-06-13T13:38:41Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

jsf-clabot · 2017-06-13T13:38:41Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

ashensis · 2017-06-14T09:40:59Z

@scottgonzalez All comments in initial round of review had been addressed, please have a look

adir01 · 2017-07-02T18:05:10Z

@scottgonzalez Are you ok with @ashensis changes ? Did he address all your comments ? Is anything else needed to allow his code to be merged ?

adir01 · 2017-11-15T12:51:40Z

@arschmitz and @jzaefferer who is in charge to review this PR . @scottgonzales is not answering a long time

jquerybot added the CLA: Error label Jun 30, 2016

JSFOwner removed the CLA: Error label Nov 5, 2016

scottgonzalez suggested changes Jun 5, 2017

View reviewed changes

ashensis closed this Jun 12, 2017

ashensis force-pushed the master branch from 68cd8d7 to 74f8a0a Compare June 12, 2017 18:03

ashensis added 2 commits June 13, 2017 14:22

jquery-ui: BTD support 1

254fd68

jquery-ui: BTD support 2

cb09255

scottgonzalez reopened this Jun 13, 2017

Harry-81 approved these changes Jun 21, 2017

View reviewed changes

Base automatically changed from master to main February 19, 2021 19:58

fnagel added the Feature label Sep 25, 2021

jquery-ui:Base text direction support #1715

Are you sure you want to change the base?

jquery-ui:Base text direction support #1715

Conversation

ashensis commented Jun 30, 2016 • edited Loading

mention-bot commented Jun 30, 2016

scottgonzalez commented Jun 30, 2016

tomerm commented Jun 30, 2016

arschmitz commented Jul 1, 2016

tomerm commented May 15, 2017

scottgonzalez commented May 24, 2017

tomerm commented May 25, 2017

scottgonzalez commented Jun 5, 2017

scottgonzalez left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottgonzalez commented Jun 12, 2017

adir01 commented Jun 13, 2017

jsf-clabot commented Jun 13, 2017 • edited Loading

jsf-clabot commented Jun 13, 2017

ashensis commented Jun 14, 2017

adir01 commented Jul 2, 2017 • edited Loading

adir01 commented Nov 15, 2017

ashensis commented Jun 30, 2016 •

edited

Loading

jsf-clabot commented Jun 13, 2017 •

edited

Loading

adir01 commented Jul 2, 2017 •

edited

Loading