review request: fix_5926

This change list fix issue 5926 -- RTL: Text in [New Tab] should be truncated from RightToLeft instead of LeftToRight.
(http://crbug.com/5926)

Basically, the issue is that in RTL locales, the thumbnail title etc. text will always be truncated from left, even if they are pure English. For example, "mail.yahoo.com" whose title is "Yahoo! the best web based email!" will be truncated to "best web-based email!".
We should be able to truncate the title as "Yahoo! the be...".

The fix is to set the direction style of each title in the [New Tab] html page for title to be displayed and truncated correctly.

The fix consists 2 part: new_tab_ui.cc in backend and new_tab.html in frontend.

1. new_tab_ui.cc
For thumbnail title, recent bookmark title, and recently closed tab titles, originally, we set "title" and "url" into the data we pass to JS in new_tab.html. The fix added the title's "direction" to the data. The direction is by default  "ltr" and is set as "rtl" when there is character having strong directionality in the title.

2. new_tab.html
2.1 set text-alignment in thumbnail-title, bookmark container, and recently closed container to be "right" for RTL locales.
2.2 explicitly set title's directionality in the above 3 sections.


Test:
1. open Chrome in RTL locales.
2. open the following pages:
http://yahoo.com
http://gmail.com
http://mail.yahoo.com
http://wikipedia.com
http://msdn.microsoft.com/en-us/default.aspx
http://arabic.arabia.msn.com/default.aspx
http://he.wikipedia.org/
3. bookmark, close some of them.
4. open [New Tab]





Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9129

[jeremy, 2009-01-31 05:28:02]

lgtm

w/some minor nits.

http://codereview.chromium.org/19738/diff/1/3
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/19738/diff/1/3#newcode134
Line 134: // title of http://msdn.microsoft.com/en-us/default.aspx is "MSDN:
Micorsoft
nit: MSDN: Microsoft

http://codereview.chromium.org/19738/diff/1/3#newcode141
Line 141: // directionality as "lrt".
lrt->ltr
in most of the above paragraph 'title' should be 'the title'.

http://codereview.chromium.org/19738/diff/1/2
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/19738/diff/1/2#newcode547
Line 547: */
>80 chars
How about:
Set the title's directionality independently of overall page directionality.

We need to do this since a purely LTR title should always have it's direction
set as ltr.  We only set the title direction to rtl if it contains a strong RTL
character.  See Issue 5926 for details.

http://codereview.chromium.org/19738/diff/1/2#newcode690
Line 690: */
How about:
"Set the bookmark title's directionality independently of the page, see comment
about setting div_title.style.direction above for details"

http://codereview.chromium.org/19738/diff/1/2#newcode786
Line 786: */
IMHO you can also reference the comment above here.

[xji, 2009-01-31 07:06:55]

On 2009/01/31 05:28:02, jeremy wrote:
> lgtm
> 
> w/some minor nits.
> 
> http://codereview.chromium.org/19738/diff/1/3
> File chrome/browser/dom_ui/new_tab_ui.cc (right):
> 
> http://codereview.chromium.org/19738/diff/1/3#newcode134
> Line 134: // title of http://msdn.microsoft.com/en-us/default.aspx is "MSDN:
> Micorsoft
> nit: MSDN: Microsoft

done

> 
> http://codereview.chromium.org/19738/diff/1/3#newcode141
> Line 141: // directionality as "lrt".
> lrt->ltr

done

> in most of the above paragraph 'title' should be 'the title'.

done

> 
> http://codereview.chromium.org/19738/diff/1/2
> File chrome/browser/resources/new_tab.html (right):
> 
> http://codereview.chromium.org/19738/diff/1/2#newcode547
> Line 547: */
> >80 chars
> How about:
> Set the title's directionality independently of overall page directionality.
> 
> We need to do this since a purely LTR title should always have it's direction
> set as ltr.  We only set the title direction to rtl if it contains a strong
RTL
> character.  See Issue 5926 for details.

done

> 
> http://codereview.chromium.org/19738/diff/1/2#newcode690
> Line 690: */
> How about:
> "Set the bookmark title's directionality independently of the page, see
comment
> about setting div_title.style.direction above for details"
> 

done

> http://codereview.chromium.org/19738/diff/1/2#newcode786
> Line 786: */
> IMHO you can also reference the comment above here.

done

[idana, 2009-02-01 02:16:27]

LGTM, but I have a few nit comments/questions.

Also, once everyone else LGTM the change, can you please have timsteele take a
look at it as well? He is the engineer who implemented the RTL code for the New
Tab page and he is a better person to review the code than I am...

http://codereview.chromium.org/19738/diff/401/403
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/19738/diff/401/403#newcode139
Line 139: // D...". For example, the title of http://yahoo.com is "Yahoo!". In
RTL
"For example," -> "As another example,"

http://codereview.chromium.org/19738/diff/401/403#newcode146
Line 146: // left to right strings. Simply set "dir" attribute of the title
works fine
"Simply set "dir" attribute of the title" -> "Simply setting the title's "dir"
attribute"

http://codereview.chromium.org/19738/diff/401/403#newcode147
Line 147: // for rendering and truncating the title. But it does not work for
showing
"But it does not" -> "However, is does not"

http://codereview.chromium.org/19738/diff/401/403#newcode149
Line 149: // LRE-PDF pair, move over title "Yahoo!" will render "!Yahoo".
I am not sure I understand the issue with the mouse over. Can you please explain
it to me?

http://codereview.chromium.org/19738/diff/401/402
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/19738/diff/401/402#newcode540
Line 540: /* Set the title's directionality independently of overall page
"of overall" -> "of the overall"

[xji, 2009-02-02 18:44:03]

On 2009/02/01 02:16:27, idana wrote:
> LGTM, but I have a few nit comments/questions.
> 
> Also, once everyone else LGTM the change, can you please have timsteele take a
> look at it as well? He is the engineer who implemented the RTL code for the
New
> Tab page and he is a better person to review the code than I am...
> 

will send to timsteele for review.

> http://codereview.chromium.org/19738/diff/401/403
> File chrome/browser/dom_ui/new_tab_ui.cc (right):
> 
> http://codereview.chromium.org/19738/diff/401/403#newcode139
> Line 139: // D...". For example, the title of http://yahoo.com is "Yahoo!". In
> RTL
> "For example," -> "As another example,"

done

> 
> http://codereview.chromium.org/19738/diff/401/403#newcode146
> Line 146: // left to right strings. Simply set "dir" attribute of the title
> works fine
> "Simply set "dir" attribute of the title" -> "Simply setting the title's "dir"
> attribute"
> 

done


> http://codereview.chromium.org/19738/diff/401/403#newcode147
> Line 147: // for rendering and truncating the title. But it does not work for
> showing
> "But it does not" -> "However, is does not"

done

> 
> http://codereview.chromium.org/19738/diff/401/403#newcode149
> Line 149: // LRE-PDF pair, move over title "Yahoo!" will render "!Yahoo".
> I am not sure I understand the issue with the mouse over. Can you please
explain
> it to me?

For every title link in the [New Tab] page, if you mouse over the title, the
completed title will be displayed in the tooltip.
we need to add LRE-PDF pair around LTR title, otherwise, the completed title
showed in tooltip is not correct, such as "Yahoo!" will be showed as "!Yahoo". 

> 
> http://codereview.chromium.org/19738/diff/401/402
> File chrome/browser/resources/new_tab.html (right):
> 
> http://codereview.chromium.org/19738/diff/401/402#newcode540
> Line 540: /* Set the title's directionality independently of overall page
> "of overall" -> "of the overall"

done

[xji, 2009-02-02 18:47:24]

Hi Tim,

Idan suggested that I ask you for the code review for the RTL fix in [New Tab]
page.

Glen,

Could you please also review the new_tab.html part?

Thanks,
Xiaomei

[idana, 2009-02-03 06:48:21]

LGTM (just a few more nit comments)

http://codereview.chromium.org/19738/diff/207/15
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/19738/diff/207/15#newcode135
Line 135: // Microsoft developer network". In RTL locales, in [New Tab] page, if
the
"in [New Tab]" -> "in the [New Tab]"

http://codereview.chromium.org/19738/diff/207/15#newcode140
Line 140: // RTL locales, in [New Tab] page, the title will be rendered as
"!Yahoo"
"in [New Tab] page" -> "in the [New Tab] page"

http://codereview.chromium.org/19738/diff/207/15#newcode141
Line 141: // without specifying its directionality as "ltr".
"without specifying its directionality as "ltr"." -> "if its "dir" attribute is
not set to "ltr"."

http://codereview.chromium.org/19738/diff/207/15#newcode148
Line 148: // showing the completed title when mouse over the title link. For
example,
"completed title when mouse over the title link." -> "entire title within a
tooltip when the mouse is over the title link."

http://codereview.chromium.org/19738/diff/207/15#newcode149
Line 149: // without LRE-PDF pair, mouse over title "Yahoo!" will render
"!Yahoo".
"mouse over title "Yahoo!" will render "!Yahoo"." -> "the title "Yahoo!" will be
rendered as "!Yahoo" within the tooltip when the mouse is over the title link.

[Glen Murphy, 2009-02-03 21:30:45]

LGTM, one nit.

http://codereview.chromium.org/19738/diff/207/14
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/19738/diff/207/14#newcode684
Line 684: */
This comment block needs to be indented by one space.

[xji, 2009-02-03 23:43:22]

changed all of them per suggestion.

Thanks for the review,
Xiaomei

On 2009/02/03 06:48:21, idana wrote:
> LGTM (just a few more nit comments)
> 
> http://codereview.chromium.org/19738/diff/207/15
> File chrome/browser/dom_ui/new_tab_ui.cc (right):
> 
> http://codereview.chromium.org/19738/diff/207/15#newcode135
> Line 135: // Microsoft developer network". In RTL locales, in [New Tab] page,
if
> the
> "in [New Tab]" -> "in the [New Tab]"
> 
> http://codereview.chromium.org/19738/diff/207/15#newcode140
> Line 140: // RTL locales, in [New Tab] page, the title will be rendered as
> "!Yahoo"
> "in [New Tab] page" -> "in the [New Tab] page"
> 
> http://codereview.chromium.org/19738/diff/207/15#newcode141
> Line 141: // without specifying its directionality as "ltr".
> "without specifying its directionality as "ltr"." -> "if its "dir" attribute
is
> not set to "ltr"."
> 
> http://codereview.chromium.org/19738/diff/207/15#newcode148
> Line 148: // showing the completed title when mouse over the title link. For
> example,
> "completed title when mouse over the title link." -> "entire title within a
> tooltip when the mouse is over the title link."
> 
> http://codereview.chromium.org/19738/diff/207/15#newcode149
> Line 149: // without LRE-PDF pair, mouse over title "Yahoo!" will render
> "!Yahoo".
> "mouse over title "Yahoo!" will render "!Yahoo"." -> "the title "Yahoo!" will
be
> rendered as "!Yahoo" within the tooltip when the mouse is over the title link.

[xji, 2009-02-03 23:43:57]

On 2009/02/03 21:30:45, Glen Murphy wrote:
> LGTM, one nit.
> 
> http://codereview.chromium.org/19738/diff/207/14
> File chrome/browser/resources/new_tab.html (right):
> 
> http://codereview.chromium.org/19738/diff/207/14#newcode684
> Line 684: */
> This comment block needs to be indented by one space.

Done.

Thanks for the review!
Xiaomei

[tim (not reviewing), 2009-02-04 01:15:21]

http://codereview.chromium.org/19738/diff/207/14
File chrome/browser/resources/new_tab.html (right):

http://codereview.chromium.org/19738/diff/207/14#newcode515
Line 515: <div class="thumbnail-title"
style="background-image:url(faviconurl);direction:ltr">gmail.com</div>
nit- you could put the style attribute on the line below the class. that's all!

[tim (not reviewing), 2009-02-04 01:15:39]

On 2009/02/04 01:15:21, timsteele wrote:
> http://codereview.chromium.org/19738/diff/207/14
> File chrome/browser/resources/new_tab.html (right):
> 
> http://codereview.chromium.org/19738/diff/207/14#newcode515
> Line 515: <div class="thumbnail-title"
> style="background-image:url(faviconurl);direction:ltr">gmail.com</div>
> nit- you could put the style attribute on the line below the class. that's
all!

forgot to say LGTM

[xji, 2009-02-04 02:25:27]

On 2009/02/04 01:15:21, timsteele wrote:
> http://codereview.chromium.org/19738/diff/207/14
> File chrome/browser/resources/new_tab.html (right):
> 
> http://codereview.chromium.org/19738/diff/207/14#newcode515
> Line 515: <div class="thumbnail-title"
> style="background-image:url(faviconurl);direction:ltr">gmail.com</div>
> nit- you could put the style attribute on the line below the class. that's
all!

Done.
Thanks for the review!

Xiaomei