Rtl labels support #5771

hodbauer · 2017-08-23T20:06:53Z

reference: #2543
For usage see Label documentation and Label sandcastle example.
Supports new lines
Currently supports Hebrew (should easily support other RTL languages)
Tasks:

Add support for more languages
Fix case when there is RTL characters, numbers and space (ex: משתמש10 )

…orrect direction

Adjusted the text position and used its setter in order to construct the Label Adjusted tests accordinglt

In order to prevent the function from running on each update if no change was made to the text

pjcozzi · 2017-08-23T23:33:54Z

Wow, very cool thanks for the nice contribution @hodbauer and @YonatanKra!

@ggetz could you take a first look at this then pass it on to @bagnell for a final review and merge?

ggetz

Thanks @hodbauer! I have a few comments.

ggetz · 2017-08-24T16:24:00Z

Apps/Sandcastle/gallery/Labels.html

@@ -157,6 +168,12 @@
        scaleByDistance();
        Sandcastle.highlight(scaleByDistance);
    }
+}, {
+    text : 'Set rtl',


Can you be more descriptive for the button label? Maybe Set label with right-to-left language

ggetz · 2017-08-24T16:26:11Z

CHANGES.md

@@ -2,7 +2,7 @@ Change Log
 ==========

 ### 1.37 - 2017-09-01
-
+* Added suppport for RTL labels


Make this a little more descriptive, maybe Added support for right-to-left languages in labels

ggetz · 2017-08-24T17:17:01Z

Source/Scene/Label.js

+            },
+            set : function(value) {
+                if (this._rtl !== value) {
+                    this._rtl = value;


I think you should call reverseRtl here in addition to when setting the text.

Thank you @ggetz for your comments. I will fix them after the weekend.
I have a specific question about this comment. Why to call reverseRtl here? If the text is written in rtl language, I don't see a reason to change the rtl flag while using it, the algorithm will work good even for ltr (left to right) languages.

Thanks @hodbauer!

If I understand correctly, if rtl is set to false, and you update text to an rtl language, then set rtl to true, it won't display correctly:

label.text = 'שלום'; label.rtl = true;

That may be confusing to people using this feature.

ggetz · 2017-08-24T17:18:16Z

Source/Scene/Label.js

+     * @param {String} text the text to parse and reorder
+     * @returns {String} the text as rtl direction
+     */
+    Label.prototype.reverseRtl = function(value) {


This function should be private, I don't think anyone should be calling this directly.

ggetz · 2017-08-24T17:19:23Z

Specs/Scene/LabelCollectionSpec.js

+        });
+
+        it('should reverse text when there is only hebrew characters and rtl is true', function() {
+           var text = 'שלום';


Can you add an additional tests that cover different scenarios like spaces and brackets in words?

ggetz · 2017-08-24T17:21:33Z

Source/Scene/Label.js

+                        result += subText.Word;
+                        splicePointer = result.length;
+                    }
+                    else if (subText.Type === types.WEAK || subText.Type ===types.BRACKETS) {


spacing after ===

and on line 1354

ggetz · 2017-08-24T17:22:39Z

Source/Scene/Label.js

+                        splicePointer += subText.Word.length;
+                    }
+                    else if (subText.Type === types.WEAK || subText.Type ===types.BRACKETS) {
+                        if (parsedText[wordIndex -1].Type === types.RTL) {


Add a space after the operator here and throughout.

* make `reverseRtl` private * fix multi-lines support * add complex test * fix some edge cases: * special characters after rtl characters when direction is not rtl * "Weak" characters after brackets at rtl direction

…orrect direction

# Conflicts: # Apps/Sandcastle/gallery/Labels.html # CHANGES.md # CONTRIBUTORS.md # Source/Scene/Label.js # Specs/Scene/LabelCollectionSpec.js

ggetz · 2017-09-06T13:50:42Z

Hi @hodbauer, I see you made updates. Are you ready for another look?

hodbauer · 2017-09-06T17:51:09Z

Hi @ggetz
I'll be glad if you go over it again.
I want to clarify why I didn't change the logic at the rtl setter. From a check that I've done it seems that the label ran the algorithm even if I didn't call the reverseRtl from the setter of rtl.

ggetz · 2017-09-07T14:45:34Z

@hodbauer, everything looks good except I still think you should reconsider the rtl setter.

In the sandcastle example, if I run this:

    var label = viewer.entities.add({
        position : Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
        label : {
            text : 'Master (אדון): Hello\nתלמיד (student): שלום'
        }
    });
    
    label.rtl = true;

The rtl portion of the text will be reversed, which is not the same results as this:

viewer.entities.add({
        position : Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
        label : {
            text : 'Master (אדון): Hello\nתלמיד (student): שלום',
            rtl : true
        }
    });

hodbauer · 2017-09-07T16:22:51Z

Ok @ggetz I'll check this case again and update the code according to the results

hodbauer · 2017-09-07T18:44:49Z

I have a question @ggetz.
At the sandcastle I ran a simple case like that:

var label = viewer.entities.add({
        position : Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
        label : {
            text : 'Hi'
        }
    });
    label.text = 'Bye';

I was expect that the label at Cesium will be Bye, but it was Hi.
If this case not work, how the case that you mention before will work for me? What I was missing?
(It also was checked at the official Sandcastle)

hodbauer · 2017-09-07T19:05:19Z

After another check at Sandcastle I've got the next solution.

    var entity = viewer.entities.add({
        position : Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
        label : {
            text : 'Master (אדון): Hello\nתלמיד (student): שלום'
        }
    });
    entity.label.rtl = true;

now it just work fine.
Waiting for your response @ggetz.

ggetz · 2017-09-07T19:50:28Z

@hodbauer My mistake, you're correct!

@bagnell You're up for final review and merge.

mramato · 2017-09-07T20:45:19Z

Are we okay with using rtl rather than rightToLeft, we usually don't use abbreviations in our API, but rtl is a fairly common one.

* fix some edge cases * change the regex that identify right-to-left languages to use unicode instead of hebrew chars for js engines that can't parse this letters (like jscoverage)

…els-support # Conflicts: # CHANGES.md

hodbauer · 2017-10-16T20:17:32Z

summary after another iteration:

Have we tried this on all browsers?

It tested on Chrome, firefox and edge

Please make sure that test coverage is high enough as there are a lot of branches.

hopefully all the branches covered by adding more tests. When Iv'e added the tests I fix some edge cases.
Another interesting thing that I found during this tests is that JScoverage lib not support hebrew characters, so I changed my regex that detect Hebrew from /[א-ת]/ to the unicode represent /[\u05D0-\u05EA]/.
Also I was tried to understand if I can support Arabic or other rtl languages and understand that if I cannot read it I shouldn't promise that it work fine...

pjcozzi · 2017-10-18T21:52:37Z

@hodbauer thanks for the diligent updates.

@mramato did you want to look at this again and merge when ready?

mramato · 2017-10-19T00:55:39Z

Sure, I'll try and get to this before the end of the week.

mramato · 2017-10-20T19:46:03Z

Source/Scene/Label.js

+     * @private
+     */
+    function reverseRtl(value) {
+        var rtlChars = /[\u05D0-\u05EA]/;


So if I'm understanding this code correctly, this regex detects whether the letter is RTL (in this case specifically Hebrew) but otherwise the rest of the code is the same for other languages? So if we update this regex for other known RTL character sets (perhaps thise discussed here and here Is that correct? If so that's awesome and we can easily update this to handle the other languages.

but otherwise the rest of the code is the same for other languages?

yes. it only necessary when writing a RTL language in Cesium.

we can easily update this to handle the other languages.

basically yes, I do not do that, because i cannot read other RTL languages.

Cool, we can merge this with just Hebrew and then I'll open a second PR with an updated regex to support additional languages.

mramato · 2017-10-20T19:47:15Z

Source/DataSources/LabelGraphics.js

+         * @type {Property}
+         * @default false
+         */
+        rightToLeft: createPropertyDescriptor('rightToLeft')


This is basically an optimization, right? If the string doesn't contain RTL characters it is a no-op?

Since this is just an optimization, how do you feel about having a single static Label.enableRightToLeftDetection variable that an application sets once globally rather than having to set it on every label? It will save memory when there are many labels and I doubt it will have a performance impact in RTL apps since I would expect most of the labels to be RTL in that case. Thoughts?

Sounds very interesting.
I'm not sure 100% how to implement it, but if it is the right way then I should do it as you offered

after a little bit of thinking I understand how to implement it, it will clean a lot of the work and it is nice.
my question is: how to declare the static variable so each application could manage it?

Just add a property directly on the Label object. Something like this should work.

/** * Enabled detection and support of right to left text. * * @type {Boolean} * @default false * * @example * //Set once at the start of the application * Cesium.Label.enableRightToLeftDetection = true; * * var myLabelEntity = viewer.entities.add({ * label: { * id: 'my label', * text: 'זה טקסט בעברית \n ועכשיו יורדים שורה' * } * }); */ Label.enableRightToLeftDetection = false;

This will show up as static in the documentation and users just set Cesium.Label.enableRightToLeftDetection = true once in their app to use it. In your own code, you would just check Label.enableRightToLeftDetection directly as well.

Thanks.

mramato · 2017-10-20T20:11:11Z

Source/Scene/Label.js

@@ -1196,5 +1249,184 @@ define([
        return false;
    };

+    function declareTypes() {
+        var TextTypes = {


I would just declare this at the top of the file instead of creating it every time. Then just use TextTypes everywhere

@mramato If I understand you, you want me to change the code to something like:

var TextTypes = freezeObject({ LTR : 0, RTL : 1, WEAK : 2, BRACKETS : 3 }); return TextTypes;

and put it inside the define near to the glyph functions as is (without a function scope)?

Yes, though you won't need the return statement. You can just put the declaration at the top of the file (above rebindAllGlyphs), it will be scoped to Label.js.

mramato · 2017-10-20T20:12:27Z

I merged in master for you and just have those few suggestions/questions.

Thanks again for the awesome work here, I'm sure a huge section of our community will be excited to have RTL support at last.

# Conflicts: # CHANGES.md

hodbauer · 2017-10-21T20:31:37Z

I don't know why but I've got this error while running travis ci:

Core/WebGLConstants
 ✗ freezes render states
	Expected function to throw an exception.

Any help about this issue, it not part of my changes

ggetz · 2017-10-23T13:28:49Z

@hodbauer Seems to be failing across multiple branches, I opened #5924

hpinkos · 2017-10-23T15:43:13Z

@hodbauer I fixed that test in master, so if you merge that into your branch CI should pass. Thanks!

hodbauer · 2017-10-23T18:16:07Z

@ggetz @hpinkos @mramato thanks for solving the problem!
@mramato we could continue with the PR

hodbauer · 2017-10-23T22:12:01Z

As @mramato offered, I changed the way to enable the right-to-left algorithm.
The new way is by setting a static parameter of Label, currently named Label.enableRightToLeftDetection to true, then every update to any label will run the algorithm.
The only issue about this as you can see is that the tests failed.
for example:

            Label.enableRightToLeftDetection = true;
            var text = 'שלום';
            var label = labels.add({
                text : text
            });

            scene.renderForSpecs();

            expect(label.text).not.toEqual(text);
            expect(label.text).toEqual(text.split('').reverse().join(''));

This test failed although I enable the right-to-left algorithm. moreover, when I've checked the sandecastle it worked as expected.
So, how to use this parameter during tests?

mramato · 2017-10-24T17:26:38Z

So, how to use this parameter during tests?

It looks like your code should be activating it, so I'm not sure what's going on. I can dig in a little further and let you know what I find.

…all text setter

hodbauer · 2017-10-24T20:20:13Z

The code was OK, the problem was that during creation of a Label nothing calls to the text setter, so nothing run the algorithm.
The mandatory change that I've done at the last commit was to call the text setter in the constructor:

this.text = defaultValue(options.text, '');

my friend @YonatanKra that help me with this PR done it before. and me, by mistake, removed it in the last changes.

mramato · 2017-10-25T13:31:43Z

Source/Scene/Label.js

+                        return;
+                    }
+                    this._originalValue = value;
+                    value = reverseRtl(value);


I just noticed that we are mutating the value supplied by the user so that the getter is actually the reversed text. I think this is undesired behavior. With few exceptions, a simple setter/getter should always return the value assigned to it. I can submit a change to prevent this, but if you feel strongly about it, please let me know.

mramato · 2017-10-25T14:04:06Z

Awesome work here @hodbauer and @YonatanKra! I submitted the tweak I mentioned, but otherwise I think this is ready so I'm going to merge.

If you find any issues or I accidentally broke something with my tweaks, please let me know.

Thanks again!

hodbauer and others added 13 commits May 4, 2017 17:19

support rtl characters (currently, only hebrew) at label to show in c…

60da6aa

…orrect direction

add example of supporting rtl characters to sandcastle

fc812bc

add unit tests to check rtl (unfortunately, not working yet)

8762607

RTL labels now work in constructor

64fd8d2

Adjusted the text position and used its setter in order to construct the Label Adjusted tests accordinglt

Merge remote-tracking branch 'AnalyticalGraphicsInc/master'

e3c601f

Merge branch 'master' into rtl-labels-support

db98c03

Upadated Changes.md

5826c7e

Updated contributors

775e637

Support for \n in RTL

f09b883

Cache RTL original string

70c3ab6

In order to prevent the function from running on each update if no change was made to the text

Prevent glyphs parser from running for nought

8d18aa3

Documentation examples

52dc64f

Updated contributors list

8350668

pjcozzi requested review from bagnell and ggetz August 23, 2017 23:31

ggetz requested changes Aug 24, 2017

View reviewed changes

hodbauer added 5 commits September 3, 2017 10:18

* fix descriptions

a0f0c1a

* make `reverseRtl` private * fix multi-lines support * add complex test * fix some edge cases: * special characters after rtl characters when direction is not rtl * "Weak" characters after brackets at rtl direction

support rtl characters (currently, only hebrew) at label to show in c…

41cea83

…orrect direction

add example of supporting rtl characters to sandcastle

84354e4

add unit tests to check rtl (unfortunately, not working yet)

c22adcf

Merge branch 'master' into rtl-labels-support

f91e96d

# Conflicts: # Apps/Sandcastle/gallery/Labels.html # CHANGES.md # CONTRIBUTORS.md # Source/Scene/Label.js # Specs/Scene/LabelCollectionSpec.js

hodbauer added 2 commits October 16, 2017 22:55

* add more tests for better code coverage

df1ddea

* fix some edge cases * change the regex that identify right-to-left languages to use unicode instead of hebrew chars for js engines that can't parse this letters (like jscoverage)

Merge remote-tracking branch 'origin/rtl-labels-support' into rtl-lab…

80b370b

…els-support # Conflicts: # CHANGES.md

Merge branch 'master' into rtl-labels-support

0020337

mramato reviewed Oct 20, 2017

View reviewed changes

Merge branch 'master' into rtl-labels-support

abc7b9e

mramato reviewed Oct 20, 2017

View reviewed changes

Merge remote-tracking branch 'origin/master' into rtl-labels-support

fac9512

# Conflicts: # CHANGES.md

Merge remote-tracking branch 'origin/master' into rtl-labels-support

11b485e

hodbauer added 2 commits October 23, 2017 23:39

declare textTypes of rightToLeft mechanism as object and not a function

32c5657

manage rightToLeft as a static property of Label

118abe4

fix tests by set text property also in the Label constructor, which c…

e109e98

…all text setter

mramato reviewed Oct 25, 2017

View reviewed changes

mramato added 2 commits October 25, 2017 09:59

Tweaks to right-to-left label handling.

c2aa9cc

Tweak Sandcastle

6bca30d

mramato merged commit 9933716 into CesiumGS:master Oct 25, 2017

hpinkos mentioned this pull request Dec 1, 2017

Labels incorrect for right-to-left languages #2543

Closed

OmKoren mentioned this pull request Jan 23, 2019

Cesium label RTL bug #7501

Closed

Rtl labels support #5771

Rtl labels support #5771

Conversation

hodbauer commented Aug 23, 2017 • edited Loading

pjcozzi commented Aug 23, 2017

ggetz 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

ggetz commented Sep 6, 2017

hodbauer commented Sep 6, 2017

ggetz commented Sep 7, 2017

hodbauer commented Sep 7, 2017

hodbauer commented Sep 7, 2017

hodbauer commented Sep 7, 2017

ggetz commented Sep 7, 2017

mramato commented Sep 7, 2017

hodbauer commented Oct 16, 2017

pjcozzi commented Oct 18, 2017

mramato commented Oct 19, 2017

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

mramato commented Oct 20, 2017

hodbauer commented Oct 21, 2017

ggetz commented Oct 23, 2017

hpinkos commented Oct 23, 2017

hodbauer commented Oct 23, 2017

hodbauer commented Oct 23, 2017

mramato commented Oct 24, 2017

hodbauer commented Oct 24, 2017

Choose a reason for hiding this comment

mramato commented Oct 25, 2017

hodbauer commented Aug 23, 2017 •

edited

Loading