Fix RTL: Omnibar - message "Press Tab to search Google" doesn't show correctly in a "New

This CL fixes issue 2780 -- RTL: Omnibar - message "Press Tab to search Google" doesn't show correctly in a "New Tab" for RTL locales

Because of the bug in PosFromChar() (see bug report for detailed information), we actually calculate the width of the string ourselves using font.GetStringWidth().

BUG=http://crbug.com/2780
TEST=
1 Start Chrome with a *new user data directory* and make sure the UI language is Hebrew.
2 Type www.google.com in the omnibox and press Enter.
3 Close and re-open the browser.
4 Type character "h" in Ominibox
5 Message "Press Tab to search Google" should show correctly, not only "Tab" is displayed

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

[xji, 2009-07-20 19:37:23]

Hi Peter,

Thanks for the suggestion!

Xiaomei

[Peter Kasting, 2009-07-20 19:54:07]

Besides the comments below, two other things:

* You should audit the rest of the omnibox code (at least) for uses of
PosFromChar() and eliminate them.
* You should ensure that GetStringWidth() always returns the right value
compared to what the omnibox is actually displaying; e.g. try some mixed
Hiragana and ASCII, or some Malayam text, or something.

http://codereview.chromium.org/155789/diff/1/2
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/155789/diff/1/2#newcode488
Line 488: int text_width = font_.GetStringWidth(location_entry_->GetText());
Nit: You might want a small comment here saying that we don't use
location_entry_->PosFromChar() due to the particular bug we encountered.

http://codereview.chromium.org/155789/diff/1/2#newcode491
Line 491: return text_width + scroll_position.x;
Is doing "+ scroll_position.x" the right thing in both LTR and RTL?  My mind
doesn't seem to want to think through this very well, but it seems like it isn't
right... for example, if in LTR we're scrolled 100 px into a 400 px string, then
the display width ought to be 400 - 100 = 300, not 400 + 100 = 500.  This is
because GetStringWidth() doesn't do the same thing as PosFromChar().

[xji, 2009-07-20 22:37:00]

Hi Peter,

Thanks for your review and valuable comments.
I have some basic questions below.

On 2009/07/20 19:54:07, pkasting wrote:
> Besides the comments below, two other things:
> 
> * You should audit the rest of the omnibox code (at least) for uses of
> PosFromChar() and eliminate them.

PosFromChar() does not always return wrong information.
It is wrong in this case when there is autocomplete match. I first thought the
wrong computation is because that the autocompleted part is highlighted, but
seems not. And if you explicitly type something in omnibox, the computation is
correct.

There are several places in Omnibox use PosFromChar(), and I do not feel
confident to change them at this point. How about I file a bug to track and
cleanup later?

> * You should ensure that GetStringWidth() always returns the right value
> compared to what the omnibox is actually displaying; e.g. try some mixed
> Hiragana and ASCII, or some Malayam text, or something.
>

What you mean "compare to what omnibox is actually displaying"? do you mean the
omnibox might show part of the clipped text, but GetStringWidth() should always
return the width of the whole string? Or the mixed font thing?
 
> http://codereview.chromium.org/155789/diff/1/2
> File chrome/browser/views/location_bar_view.cc (right):
> 
> http://codereview.chromium.org/155789/diff/1/2#newcode488
> Line 488: int text_width = font_.GetStringWidth(location_entry_->GetText());
> Nit: You might want a small comment here saying that we don't use
> location_entry_->PosFromChar() due to the particular bug we encountered.
> 
> http://codereview.chromium.org/155789/diff/1/2#newcode491
> Line 491: return text_width + scroll_position.x;
> Is doing "+ scroll_position.x" the right thing in both LTR and RTL?  My mind
> doesn't seem to want to think through this very well, but it seems like it
isn't
> right... for example, if in LTR we're scrolled 100 px into a 400 px string,
then
> the display width ought to be 400 - 100 = 300, not 400 + 100 = 500.  This is
> because GetStringWidth() doesn't do the same thing as PosFromChar().

I think I misunderstood scroll_position.x with scroll bar width.

I did not find that we ever use scroll bar in our omnibox,
But there is a case (although I can not reproduce) that scroll_position.x is not
zero. My question is why and in which case scroll_position.x is not equal to
zero?

Looks like you are right if TextDisplayWidth() returns the displayed width of
the (clipped) text in omnibox. In LTR,
the display width should be the total text width minus the scroll position. 
Without figuring out how scroll_position.x could be none zero, I do not know how
the GetScrollPos works in RTL context, whether the x coordinates returned is
relative to left edge of the omnibox or the right edge. The computation should
be the same if it is relative to the right edge, otherwise, it should be
"text_width - (location_entry_view_.width() - scroll_position.x)" only when
scroll_position.x is not zero.


And another question I do not understand is: why the display width need to be
increased by scroll bar position if PosFromChar() computed is the actual
(clipped) display width?

[Peter Kasting, 2009-07-20 23:03:32]

On 2009/07/20 22:37:00, xji wrote:
> PosFromChar() does not always return wrong information.

That doesn't matter; it returns wrong answers sometimes, therefore we can't ever
trust it.

> There are several places in Omnibox use PosFromChar(), and I do not feel
> confident to change them at this point. How about I file a bug to track and
> cleanup later?

I guess, though I always prefer fixing bugs to noting them.

> > * You should ensure that GetStringWidth() always returns the right value
> > compared to what the omnibox is actually displaying; e.g. try some mixed
> > Hiragana and ASCII, or some Malayam text, or something.
>
> What you mean "compare to what omnibox is actually displaying"? do you mean
the
> omnibox might show part of the clipped text, but GetStringWidth() should
always
> return the width of the whole string? Or the mixed font thing?

I mean, the CRichEditCtrl's layout algorithm isn't the same code as
GetStringWidth(), so who knows if it gives the same results?  Try some edge
cases, measure the pixel width onscreen, and ensure the new code agrees.

> I think I misunderstood scroll_position.x with scroll bar width.
> 
> I did not find that we ever use scroll bar in our omnibox,
> But there is a case (although I can not reproduce) that scroll_position.x is
not
> zero. My question is why and in which case scroll_position.x is not equal to
> zero?

scroll_position doesn't have to do with scrollbars.  If the edit contains more
text than it can show at once, it auto-scrolls such that the caret is visible. 
So all you need to do is type or paste enough text in that the beginning of the
edit scrolls out of view.

> And another question I do not understand is: why the display width need to be
> increased by scroll bar position if PosFromChar() computed is the actual
> (clipped) display width?

I don't know; maybe the old code is wrong.  Maybe this function is not supposed
to return what I thought it was.  You should check what the caller(s) of this
code actually expect, and ensure the new code does it :)

[xji, 2009-07-21 01:31:37]

1. I prefer to clean PosFromChar() in another CL.
2. Tested font_.GetStringWidth() using Chinese, English, Russian, and the
mixture. Looks like it works correctly.
3. added comments and fixed the bug as you pointed out.

Thanks,
Xiaomei


On 2009/07/20 23:03:32, pkasting wrote:
> On 2009/07/20 22:37:00, xji wrote:
> > PosFromChar() does not always return wrong information.
> 
> That doesn't matter; it returns wrong answers sometimes, therefore we can't
ever
> trust it.
> 
> > There are several places in Omnibox use PosFromChar(), and I do not feel
> > confident to change them at this point. How about I file a bug to track and
> > cleanup later?
> 
> I guess, though I always prefer fixing bugs to noting them.
> 
> > > * You should ensure that GetStringWidth() always returns the right value
> > > compared to what the omnibox is actually displaying; e.g. try some mixed
> > > Hiragana and ASCII, or some Malayam text, or something.
> >
> > What you mean "compare to what omnibox is actually displaying"? do you mean
> the
> > omnibox might show part of the clipped text, but GetStringWidth() should
> always
> > return the width of the whole string? Or the mixed font thing?
> 
> I mean, the CRichEditCtrl's layout algorithm isn't the same code as
> GetStringWidth(), so who knows if it gives the same results?  Try some edge
> cases, measure the pixel width onscreen, and ensure the new code agrees.
> 
> > I think I misunderstood scroll_position.x with scroll bar width.
> > 
> > I did not find that we ever use scroll bar in our omnibox,
> > But there is a case (although I can not reproduce) that scroll_position.x is
> not
> > zero. My question is why and in which case scroll_position.x is not equal to
> > zero?
> 
> scroll_position doesn't have to do with scrollbars.  If the edit contains more
> text than it can show at once, it auto-scrolls such that the caret is visible.

> So all you need to do is type or paste enough text in that the beginning of
the
> edit scrolls out of view.
> 
> > And another question I do not understand is: why the display width need to
be
> > increased by scroll bar position if PosFromChar() computed is the actual
> > (clipped) display width?
> 
> I don't know; maybe the old code is wrong.  Maybe this function is not
supposed
> to return what I thought it was.  You should check what the caller(s) of this
> code actually expect, and ensure the new code does it :)

[Peter Kasting, 2009-07-21 01:37:25]

LGTM

http://codereview.chromium.org/155789/diff/1005/1006
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/155789/diff/1005/1006#newcode489
Line 489: // PosFromChar(location_entry_->GetTextLength()) because PosFromChar()
might
Nit: "might not return the correct information" -> "is apparently buggy" (I
think this is a little clearer)

http://codereview.chromium.org/155789/diff/1005/1006#newcode491
Line 491: // left-to-right layout, PosFromChar() might return 0 when i is
greater than
Nit: PosFromChar() -> PosFromChar(i)

http://codereview.chromium.org/155789/diff/1005/1006#newcode496
Line 496: return text_width - scroll_position.x;
So, this is the right formula in both LTR and RTL?  (I don't know what
GetScrollPos() does for RTL.)

[xji, 2009-07-21 19:05:09]

On 2009/07/21 01:37:25, pkasting wrote:
> LGTM
> 
> http://codereview.chromium.org/155789/diff/1005/1006
> File chrome/browser/views/location_bar_view.cc (right):
> 
> http://codereview.chromium.org/155789/diff/1005/1006#newcode489
> Line 489: // PosFromChar(location_entry_->GetTextLength()) because
PosFromChar()
> might
> Nit: "might not return the correct information" -> "is apparently buggy" (I
> think this is a little clearer)
> 

Done

> http://codereview.chromium.org/155789/diff/1005/1006#newcode491
> Line 491: // left-to-right layout, PosFromChar() might return 0 when i is
> greater than
> Nit: PosFromChar() -> PosFromChar(i)
> 

Done

> http://codereview.chromium.org/155789/diff/1005/1006#newcode496
> Line 496: return text_width - scroll_position.x;
> So, this is the right formula in both LTR and RTL?  (I don't know what
> GetScrollPos() does for RTL.)

Currently, omnibox in our RTL UI is in left-to-right layout.
GetScrollPos() returns the position relative to left-upper corner of the text
area in both our RTL UI or LTR UI. ( I do not know whether it is because our RTL
UI layout is still left-to-right or not).

But there is a problem using GetScrollPos() with GetStringWidth(), which applies
to both RTL and LTR UI.
If the text is an URL, GetScrollPos() works ok.
If the text is pure Hebrew, the reading order is actually right-to-left, in
which case, if the caret is at the end of the string and  the (logical)
beginning of the text is scrolled out of the text area, GetScrollPos() returns
(0,0). If the caret is at the begin of the text (so the end of the text is
scrolled out of the text area), GetScrollPos() returns correct information.

It is different from what we want. And I think the correct computation   of
display width should be the minimum of string width and view width.
(if scroll happens, part of the string will always be filled out the view).

[Peter Kasting, 2009-07-21 19:45:24]

On 2009/07/21 19:05:09, xji wrote:
> On 2009/07/21 01:37:25, pkasting wrote:
> > http://codereview.chromium.org/155789/diff/1005/1006#newcode496
> > Line 496: return text_width - scroll_position.x;
> > So, this is the right formula in both LTR and RTL?  (I don't know what
> > GetScrollPos() does for RTL.)
> 
> But there is a problem using GetScrollPos() with GetStringWidth(), which
applies
> to both RTL and LTR UI.
> If the text is an URL, GetScrollPos() works ok.
> If the text is pure Hebrew, the reading order is actually right-to-left, in
> which case, if the caret is at the end of the string and  the (logical)
> beginning of the text is scrolled out of the text area, GetScrollPos() returns
> (0,0). If the caret is at the begin of the text (so the end of the text is
> scrolled out of the text area), GetScrollPos() returns correct information.
> 
> It is different from what we want. And I think the correct computation   of
> display width should be the minimum of string width and view width.
> (if scroll happens, part of the string will always be filled out the view).

I think you're right.  One other thing to check would be to see whether callers
actually want the displayed width, or whether they're only getting it in order
to subtract from the view width to find out how much space remains.  If the
latter, we could change this function to just return that directly, via
something like:

return std::max(view width - string width, 0);

[xji, 2009-07-21 23:09:14]

The return value of TextDisplayWidth() is used in several places.
But all mainly for comparing the text display width with the location bar view
width to check whether there is enough length for just a  "tab" or "press tab to
search xxx" (through another function).

I think leave it as is is more clean.

From its usage, it does not matter whether the display width returned is the
width of the string in the edit box (including those scrolled out of view) or
the width of visible string (excluding those scolled out of the view).

But from the function declaration ("Returns the width in pixels of the contents
of the edit") in header file, and if we consider PosFromChar() in original
implementation returns the correct information, the returned value is "the width
of the string in the edit box (including those scrolled out of view)".

So, I simplified the code to just return the GetStringWith() without any further
adjustment.

Could you please review it again?
Thanks very much!

Xiaomei





On 2009/07/21 19:45:24, pkasting wrote:
> On 2009/07/21 19:05:09, xji wrote:
> > On 2009/07/21 01:37:25, pkasting wrote:
> > > http://codereview.chromium.org/155789/diff/1005/1006#newcode496
> > > Line 496: return text_width - scroll_position.x;
> > > So, this is the right formula in both LTR and RTL?  (I don't know what
> > > GetScrollPos() does for RTL.)
> > 
> > But there is a problem using GetScrollPos() with GetStringWidth(), which
> applies
> > to both RTL and LTR UI.
> > If the text is an URL, GetScrollPos() works ok.
> > If the text is pure Hebrew, the reading order is actually right-to-left, in
> > which case, if the caret is at the end of the string and  the (logical)
> > beginning of the text is scrolled out of the text area, GetScrollPos()
returns
> > (0,0). If the caret is at the begin of the text (so the end of the text is
> > scrolled out of the text area), GetScrollPos() returns correct information.
> > 
> > It is different from what we want. And I think the correct computation   of
> > display width should be the minimum of string width and view width.
> > (if scroll happens, part of the string will always be filled out the view).
> 
> I think you're right.  One other thing to check would be to see whether
callers
> actually want the displayed width, or whether they're only getting it in order
> to subtract from the view width to find out how much space remains.  If the
> latter, we could change this function to just return that directly, via
> something like:
> 
> return std::max(view width - string width, 0);

[Peter Kasting, 2009-07-21 23:18:37]

On 2009/07/21 23:09:14, xji wrote:
> The return value of TextDisplayWidth() is used in several places.
> But all mainly for comparing the text display width with the location bar view
> width to check whether there is enough length for just a  "tab" or "press tab
to
> search xxx" (through another function).

Then we should change the return to be that remaining width.

> I think leave it as is is more clean.

I disagree.

> From its usage, it does not matter whether the display width returned is the
> width of the string in the edit box (including those scrolled out of view) or
> the width of visible string (excluding those scolled out of the view).

Only because the edit doesn't scroll until it's completely full, and the callers
don't actually care about what the function is documented to return, they really
care about something else (whether there is space for an additional UI element).

We should change the function as I described in my previous comment.

[xji, 2009-07-22 01:03:02]

I've uploaded a CL as you suggested.

But I still feel previous version is better because:
1. we can not further consolidate the functions, such as UsePref(), in which the
width checking is done, since the 1st parameters are different for different
callers. And we do not want to inline RemainingWidthExceptText() in UsePref() to
avoid to call GetStringWidth() multiple times.
2. RemainingWidthExceptText() does not return values using the view_width -
string_width, it depends on the passing in view's max width parameter, which
makes it less reusable.
3. due to the above 2 reason, code wise, I think the previous version is more
readable and has better functionality division. Performance wise, current
version does not save much.

But it is your call.

Thanks,
Xiaomei


On 2009/07/21 23:18:37, pkasting wrote:
> On 2009/07/21 23:09:14, xji wrote:
> > The return value of TextDisplayWidth() is used in several places.
> > But all mainly for comparing the text display width with the location bar
view
> > width to check whether there is enough length for just a  "tab" or "press
tab
> to
> > search xxx" (through another function).
> 
> Then we should change the return to be that remaining width.
> 
> > I think leave it as is is more clean.
> 
> I disagree.
> 
> > From its usage, it does not matter whether the display width returned is the
> > width of the string in the edit box (including those scrolled out of view)
or
> > the width of visible string (excluding those scolled out of the view).
> 
> Only because the edit doesn't scroll until it's completely full, and the
callers
> don't actually care about what the function is documented to return, they
really
> care about something else (whether there is space for an additional UI
element).
> 
> We should change the function as I described in my previous comment.

[Peter Kasting, 2009-07-22 02:17:02]

LGTM

http://codereview.chromium.org/155789/diff/1014/11
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/155789/diff/1014/11#newcode423
Line 423: const int remaining_width = RemainingWidthExceptText(max_edit_width);
Nit: just roll this call into the line below that uses it instead of creating a
temp.

http://codereview.chromium.org/155789/diff/1014/11#newcode550
Line 550: int remaining_width, gfx::Rect* bounds) {
Nit: I think it'd be better if each argument got its own line.

http://codereview.chromium.org/155789/diff/1014/12
File chrome/browser/views/location_bar_view.h (right):

http://codereview.chromium.org/155789/diff/1014/12#newcode386
Line 386: // Returns the remaining width in pixels left for a view in edit.
Nit: This comment and function name make no sense.  How about:

// Returns the amount of horizontal space (in pixels) out of
|location_bar_width| that is not taken up by the actual text in
|location_entry_|.

(Then you can rename the various args below |available_width|.)
int AvailableWidth(int location_bar_width);

http://codereview.chromium.org/155789/diff/1014/12#newcode389
Line 389: // Returns true if the preferred size should be used for a view whose
width
Nit: How about rewriting this unclear comment as follows:

// Returns whether the |available_width| is large enough to contain a view with
preferred width |pref_width| at its preferred size.