Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#15531 closed enhancement (fixed)

Button: control of Base text direction

Reported by: wajnberg Owned by: Douglas Hays
Priority: undecided Milestone: 1.9
Component: Dijit - Form Version: 1.7.2
Keywords: Cc: sashash
Blocked By: Blocking: #15734

Description (last modified by bill)

Support for textDir property in Button, ToggleButton?, DropDownButton? and ComboButton?, same as for form widget. The core support for base text direction (via textDir) was introduced via #12367.

Attachments (2)

ButtonTxtDir.patch (23.6 KB) - added by bill 7 years ago.
updated patch from Moshe Wajnberg
15531_15654.patch (49.2 KB) - added by Douglas Hays 7 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by bill

Description: modified (diff)
Milestone: tbd2.0

I am noticing a lot of duplication across dijit, with lots of _setTextDirAttr methods that are basically the same. It seems like it would be better to have a single method in _BidiSupport.js that handled simple cases like Button and MenuItem.

Changed 7 years ago by bill

Attachment: ButtonTxtDir.patch added

updated patch from Moshe Wajnberg

comment:2 Changed 7 years ago by bill

Milestone: 2.01.9
Summary: Button: control of Base text direction inButton: control of Base text direction

comment:3 Changed 7 years ago by bill

The patch can be simplified by removing the second argument to applyTextDir() (see [29559]).

Also, same as #15654, likely the _setTextDirAttr() functionality in Button.js should instead be moved into _BidiSupport::_setTextDirAttr(), by either generalizing that existing method to apply to this.containerNode||this.focusNode (if this.displayNode is not set), or by perhaps setting up a new data-dojo-attach-point called textDirNode (or something like that).

Last edited 7 years ago by bill (previous) (diff)

Changed 7 years ago by Douglas Hays

Attachment: 15531_15654.patch added

comment:4 Changed 7 years ago by Douglas Hays

Cc: sashash added

I attached a patch that needs review/testing. I changed the MenuItem/textDir references from focusNode to containerNode, and created a generalized _setTextDirAttr in _BidiSupport.js

comment:5 Changed 7 years ago by bill

I forget why/when that code to set align is necessary, and why it would be needed exclusively for this.displayNode (but not this.containerNode etc.).

Also, one minor thing is that (at least in theory) you shouldn't need the if(this.textDir === "auto") switch because I updated _BidiSupport:: applyTextDir() to not do anything when textDir=="", and hopefully not do anything unwanted when textDir=="ltr" or "rtl".

But other than that the patch looks good to me.

comment:6 Changed 7 years ago by bill

Blocking: 15734 added

(In #15734) Needs #15531 for StackContainer? buttons.

comment:7 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: newclosed

In [29586]:

Fixes #15531, #15654. Proxy commit for Alex Shensis (IBM,CCLA). Create a unified _setTextDirAttr method in _BidiSupport, removing same from MenuItem?. Create a custom _setTextDirAttr method for Button, and add automated testcases.

comment:8 Changed 7 years ago by bill

Resolution: fixed
Status: closedreopened

I'm getting failures in the menu test after this. One problem is that you didn't update MenuBarItem.html to have a textDirNode attach point, but even after fixing that I get failures.

comment:9 Changed 7 years ago by Douglas Hays

Resolution: fixed
Status: reopenedclosed

In [29594]:

Fixes #15531. Add textDirNode to other MenuItem? widgets. Change tests to use textDirNode instead of focusNode. Lots of whitespace cleanup.

comment:10 Changed 7 years ago by bill

In [29598]:

Need to call this._set() so watch() notifications work, refs #15531, #15734, #15764 !strict.

Note: See TracTickets for help on using tickets.