Dijit mirroring defects

[ramys@…]

Fixing the following mirroring defects in Dojo widgets:

InlineEditBox?: Vertical bar in dijit.InlineEditBox? ComboBox?: Vertical bar in dijit.form.ComboBox? popup: Vertical Bar Progressbar: Problem with Indeterminate Progress bar InlineEditBox?: The programmatically created dijit.InlineEditBox? is not mirrored when activated ComboBox?: Wrong display of mirrored dijit.form.ComboBox? when text is longer than its width SplitContainer?: Wrong mirroring of dijit.layout.ContentPane? Tree (ForestStoreModel?): The programmatically created dijit.Tree is not mirrored correctly when dir is specified on widget level Menu, MenuItem?: Wrong place of the arrow of the mirrored dijit.PopupMenuItem? Menu, MenuItem?: Problem with mirrored dijit.CheckedMenuItem? Menu, MenuItem?: Wrong place of the icom of the mirrored dijit.PopupMenuItem? Menu, MenuItem?: Wrong display of mirrored dijit.PopupMenuItem? with shortcut StackContainer?: Wrong display of mirrored dijit.layout.StackContainer? when dir is specified on widget level Menu: Text Alignment ComboBox?: Menu Position InlineEditBox?: Menu Position

[bill]

Pasting my notes from email before they get lost:

You've made a lot of changes, so what I'd like you to do is to resubmit them as a series github pull request against the latest code in ​https://github.com/dojo/dijit, one pull request per widget (except the Menu and MenuItem? changes can be combined), and one for the popup change. Each pull request should explain what the change is for. For example, the ProgressBar? pull request should say something like "reverse direction of indeterminate progress bar animation, to sweep from right to left in RTL mode", instead of just "Problem with Indeterminate Progress bar" like you wrote here.

Normally I write feedback in github, as responses to the pull requests, but I'll give some initial feedback now. You can address these questions in the individual pull requests for each file.

1. Your patch contains changes not listed here, about ctrl-shift:
   • RichText? - Ctrl + Shift shortcut used to change the page orientation
   • _TextBoxMixin - Apparently the same thing as richtext? Except the comment is wrong because it says "change the page orientation" when clearly it is only affecting that widget.

These sound like enhancements rather than defects, but anyway I need some explanation about what they are for. When you make the pull requests for Editor and TextBox?, please explain what you are enhancing, and include automated tests.

2. Your changes to claro need to be made to the .less files (too). The .css files are generated from the .less files using IIRC "node compile.js".

3. Your changes to Widget#isLeftToRight() seem unnecessary. Dijit already has code to read all declarative attributes, include "dir". Do you have a test case that's failing w/out this change?

4. The change to InlineEditBox? is mysterious. Why doesn't the current code work? Presumably this.dir == this.domNode.dir.

5. The change to MenuItem? need a lot of work:
   • the formatting of the function declaration is non standard
   • you can't call getParent() in postCreate(); you can't access other widgets (including the parent widget) until startup()
   • the call to require("dojo/dom-construct") is weird and it won't work if dojo/dom-construct is not already loaded; that dependency should be listed at the top of the file
   • I'm uneasy w/the whole change. Is this an attempt to support a dir=rtl MenuItem? inside a dir=ltr Menu? And in that case you think the arrow and icon should be in a different direction from the other MenuItems??

6. popup.js: you claim that your changes to popup to js are for "Vertical bar". I don't know what that means, but your changes are clearly for something else.

7. Test files: it looks like all your test files are new? I didn't look at your test file changes completely, but surely you don't need a new 10 page test file for ComboBox?. I would hope you could reuse the existing test files in _BidiSupport. Likewise for all the other test files.

Regarding formatting:

8. make sure that you use tabs throughout, to avoid misalignment issues like:

        // Bidi Support
                this.dir = domStyle.getComputedStyle(this.domNode).direction;

9. In dojo V1 code, else should be written as }else{ not w/the extra newlines

// Bidi Support
if(!this.isLeftToRight()){
    slider.style.right = pos + 'px';
}
else{
    slider.style.left = pos + 'px';
}

10. no space before semicolons:

this.domNode.dir = info.selected.domNode.dir ; 

[bill]

Bumping since no response.

[ramys@…]

Please find below our responses for each of your comments:

• We modified the comment in the two files to be "change widget orientation"
• These two fixes are for fixing the "ctrl+shift" behavior. Without these fixes, Ctrl+Shift does not change the position of the cursor as expected.
• The automated test file is: _BidiSupport/mirroring/test_Editor.html

2. Done in Menu_rtl.less and ProgressBar_rtl.less

3. The code in _WidgetBase.js is for handling programmatic creation of the widget with the dir attribute in the div.

If we do not write dir attribute during programmatic creation and put the dir attribute only on the div, the widget will not be affected without this fix. Please refer to item3wrong and item3expecetd in the attached images.

4. We set the dir of the "editorWrapper" with this.domNode.dir. To handle the case where for example Body is LTR and Widget is RTL.

For example, before Editing the widget is RTL as expected, but after editing it becomes LTR which is wrong. After the fix, the widget will be RTL before and after editing as expected. Please refer to item4beforefix and item4afterfix in the attached images, each of them has the initial widget in the above half and the widget after editing in the bottom half.

• Modifications are done in MenuItem?.js
• I changed the method from postCreate() to startup()
• I added "dojo/dom-construct" at the top of the file.
• Yes this fix is to support a dir=rtl MenuItem? inside a dir=ltr Menu and Yes I think the arrow and icon should be in a different direction from the other MenuItems?, please refer to item5 in the attached images.

6. The code in popup.js is for Vertical bar and popup position. Please refer to item6wrong and item6expecetd in the attached images.

7. There is only 1 test file for ComboBox?.

There is only one test file per widget and they are used also for automated testing

8. Done in SplitContainer?.js

9. Done in SplitContainer?.js and DropDownMenu?.js

10. Done in StackController?.js

[ramys@…]

Images for items #3, #5 and #6

[ramys@…]

Images for item #4

[bill]

OK, thanks, but again, you've made a lot of changes, so what I'd like you to do is to resubmit them as a series github pull request against the latest code in ​​https://github.com/dojo/dijit, one pull request per widget (except the Menu and MenuItem? changes can be combined), and one for the popup change.

[Bill Keese <bill@…>]

In 6c3b2f20b94e45d772149f4d8e06d0762a616d5f/dijit:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Bill Keese <bill@…>]

In 54c2c3a72e380dceffa274e4fc47ac9a0c591fff/dijit:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[bill]

I'm closing this meta-ticket since I never got the PR's as I requested. Please open individual tickets for any remaining issues that you still want to fix.