Add auto language direction in reader

[yurynix]

This pull request addresses the issue of people using calypso in LTR interface ( English )
and reading blogs in RTL language ( such as Hebrew )

Before	After

Testing instructions

1. Run git checkout add/auto-direction and start your server, or open a live branch
2. Open the Home page
3. Visit reader's following stream and assert you see the posts in the correct directionality
4. Visit reader's full post view and assert you see the post and comments in the correct directionality
5. Start typing in the comments textarea, it should change directionality according to what you type ( RTL / LTR lang )

Reviews

• Code
• Product

@Automattic/sdev-feed
/cc @yoavf @bgbg @ranh @danieldanilov

[matticbot]

Test live: https://calypso.live/?branch=add/auto-direction

[blowery]

you'll need to add https://github.com/twitter/RTLtextarea/blob/master/LICENSE to https://github.com/Automattic/wp-calypso/blob/master/CREDITS.md

[blowery]

This is really cool! How does it impact rendering perf?

[blowery]

Is there any way to implement a meaningful shouldComponentUpdate for AutoDirection? Seems like a lot of work to repeat every time a child updates.

[yurynix]

I don't think so, because every time a child updates we need to consider whether it has RTL or not ( consider a textarea for example ).

I'm open to suggestions though =)

[blowery]

we're running a regexp match for every character in the content? So for a 100,000 character post body, we'd run 100,000 regexp matches?

[yurynix]

Yep.
We can avoid the regex here, but it'll be less readable, i'll create a benchmark to check out if it worth it, thanks!

[blowery]

it looks like twitter is running the regexp against the entire text, then checking the length of the returned array. We could do something like that, but cap the text length at, say, 1000 characters?

[blowery]

I'm thinking about capping because we likely don't need to check the entire text to get a significant sample, and there's no good reason to build up a matches array whose size matches the entirety of the body content.

[yurynix]

That's a great suggestion, I've implemented a cap and refactored the function to not use regex.

[yurynix]

This is really cool! How does it impact rendering perf?
For components that don't update much like static texts, imo it doesn't have much overhead, unless you put it on a component with a lot of children ( which shouldn't be done ).

For components that update a lot like textarea it does add some overhead, but improves user experience imo =)

BTW, FB has something similar at their status input field, check it out =)

[drewblaisdell]

This is great. It worked well in my tests. I had a couple questions and a minor note about function naming.

As far as I can tell, there is a ~20% performance impact when going from a list of posts in the Reader to a full page post. We can probably get that number down quite a bit by decreasing RTL_THRESHOLD (if that doesn't impact UX too much). Someone with more profiling experience in DevTools might do a better job determining the performance of this.

[drewblaisdell]

Can we use stripHTML instead?

[drewblaisdell]

For my own understanding: where did this number come from? It seems like half or even less would be sufficient, and it has a direct impact on performance.

[yurynix]

That's just what @blowery suggested, but I guess we can get it safely to something like 100, I'll do that now.

[drewblaisdell]

Super minor: in general, the whole word seems to be used instead of char:

wp-calypso (add/auto-direction) ☯ spot -i client/ character | wc -l
     262

[yurynix]

I've measured with MAX_LENGTH_OF_TEXT_TO_EXAMINE = 100 using:

window.YuryTime = 0;
const PerformanceMonitorHOC = ( Component ) => {
		return ( props ) => {
			const t0 = performance.now();
			const result = Component( props );
			const t1 = performance.now();

			window.YuryTime += t1 - t0;
			return result;
		};
};
export default PerformanceMonitorHOC( AutoDirection );

After the stream was loaded:
window.YuryTime === 8.919999999992797

with MAX_LENGTH_OF_TEXT_TO_EXAMINE = 1000, the result was 12.71999999999025

I've measured with the debug version running locally.

[gziolo]

Cool hack day idea ;)

[drewblaisdell]

@yurynix Thanks for benchmarking that. :)

If you're cool with the commit I added (feel free to squash it), the code LGTM.

[ranh]

Great work @yurynix! This would be a big win for the Reader. Product LGTM to me, other than one small issue.

It looks like neutral characters are always treated as LTR. This means that an RTL-only text can be identified as LTR even if it contains no LTR characters.

To reproduce, just type a few characters in Hebrew or another RTL script into a comment field, and start adding spaces.

You can probably just ignore any character that doesn't have "strong" directionality.

---

Regardless of this specific issue, direction switching in the comment field surprised me a bit. When writing a short comment with mixed directionality, the writing direction can switch rapidly back and forth between LTR and RTL as the balance of the mix changes. This can get confusing and disorienting.

For example:

It looks like this could be further tweaked/optimized in a later revision. I wonder though how much of this is in fact related to the neutral character issue.

I tried this in a few other places and noticed that it's a common pattern. Twitter and Google+ work pretty much the same way. Since this is relying on a Twitter library I guess that makes sense :)

Facebook seems to rely only on the first character to determine directionality, and it doesn't shift around while you type. However Facebook determines directionality separately for each paragraph, so it's not really comparable.

Don't have any answers here. Not sure I even have a question actually. Personally I'm not a fan of how this approach makes things jump around. But even with this side effect, it's still a huge improvement over the current situation.

[blowery]

rebased this to pick up recent changes on master, and added a06f6e0 which makes it deal with cases where there's only one child of AutoDirection and that child has no children.

[blowery]

i'm ...minorly worried .. about this. we have some pretty big contents in the reader and copying the entire thing isn't going to be cheap. maybe we truncate it to our max examine * 2 before stripping?

[yurynix]

Thank you for the suggestion 👍 , I'll take care of it next week

[blowery]

@ranh could you give this another shot? I added support for some basic neutrals (whitespace, 0-9, dash).

[ranh]

@blowery looks much better now. Ignoring spaces solves the biggest problem I think.

But the same issues would apply to all punctuation marks, symbols, emoji, etc.

[yurynix]

I've updated the code to use a different approach, to look at a strong directionality characters according to unicode spec.

I've generated the characters ranges with this code:

const fetch = require( 'node-fetch' );
const MultiRange = require('multi-integer-range').MultiRange;

const UNICODE_DATA_URL = 'http://www.unicode.org/Public/UNIDATA/UnicodeData.txt';
const CATEGORIES_OF_INTEREST = [ 'L', 'R', 'AL' ];
const bidi_categories = {};


fetch( UNICODE_DATA_URL )
    .then( ( res ) => res.text() )
    .then( body => body.split( '\n' ) )
    .then( ( charcterLines ) => charcterLines.map( ( line ) => {
        const lineParts = line.split( ';' );
        return {
            code: parseInt( lineParts[ 0 ], 16 ),
            name: lineParts[ 1 ],
            bidi_category: lineParts[ 4 ]
        };
    } ) )
    .then( ( charcters ) => {
        charcters.forEach( char => {
            if ( ! Array.isArray( bidi_categories[ char.bidi_category ] ) ) {
                bidi_categories[ char.bidi_category ] = [];
            }

            bidi_categories[ char.bidi_category ].push( char.code );
        } );

        Object.keys( bidi_categories )
        .filter( key => CATEGORIES_OF_INTEREST.indexOf( key ) > -1 )
        .forEach( key => {
            const range = new MultiRange( bidi_categories[ key ] );
            const ranges = range.getRanges();
            console.log( key, ':', bidi_categories[ key ].length, ranges.length );
            ranges.forEach( iteratedRange => console.log( `[ 0x${iteratedRange[ 0 ].toString( 16 )}, 0x${iteratedRange[ 1 ].toString( 16 )} ],` ) );
        } );
    } );

[blowery]

do we still need this?

[yurynix]

We don't, I'll remove it.

[blowery]

Could just be a Set instead. We don't care about the value.

[blowery]

Squashed the commits by author, going to land this

[blowery]

🚢

[yurynix]

thank you @blowery !