Fix focus rectangle for label.

Fix focus rectangle for label.

The current focus rectangle logic is wrong -- it's always drawn
outside of local bounds, which is not drawn actually. The expected
behavior is to draw the focus rectangle around the actual text.

BUG=467510
R=sky@chromium.org
TEST=the new test case covers

Committed: https://crrev.com/26abb0bf78b3e67e11b7592d057a7f536bb40f3d
Cr-Commit-Position: refs/heads/master@{#320943}

[sky, 2015-03-16 21:29:57]

https://codereview.chromium.org/1018463004/diff/1/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1018463004/diff/1/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:466: Vector2d GetLineOffset(size_t line_number);
Move implementation to match new position.

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc#...
ui/views/controls/label.cc:340: if (HasFocus()) {
What if the label has focus, but is empty? What will focus_bounds be?

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc#...
ui/views/controls/label.cc:341: focus_bounds.Inset(-kFocusBorderPadding,
-kFocusBorderPadding);
What if kFocusBorderPadding is > current size?

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc#...
ui/views/controls/label.cc:345: return gfx::Rect();
Is there a reason not to always return focus_rect? And why is this returning the
focus rect in the first place? If you say so you can get the focus rect can we
move that into a separate function?

[Jun Mukai, 2015-03-16 22:05:38]

https://codereview.chromium.org/1018463004/diff/1/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/1018463004/diff/1/ui/gfx/render_text.h#newcod...
ui/gfx/render_text.h:466: Vector2d GetLineOffset(size_t line_number);
On 2015/03/16 21:29:57, sky wrote:
> Move implementation to match new position.

Done.

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc#...
ui/views/controls/label.cc:340: if (HasFocus()) {
On 2015/03/16 21:29:57, sky wrote:
> What if the label has focus, but is empty? What will focus_bounds be?

Good point, I believe a thin rectangle (line-height with no width or 1px width)
should be drawn. Added the handler code and a test case.

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc#...
ui/views/controls/label.cc:341: focus_bounds.Inset(-kFocusBorderPadding,
-kFocusBorderPadding);
On 2015/03/16 21:29:57, sky wrote:
> What if kFocusBorderPadding is > current size?

Fixed to be capped inside of the size.

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc#...
ui/views/controls/label.cc:345: return gfx::Rect();
On 2015/03/16 21:29:57, sky wrote:
> Is there a reason not to always return focus_rect? And why is this returning
the
> focus rect in the first place? If you say so you can get the focus rect can we
> move that into a separate function?

Changed to keep the focus bounds as a field |focus_bounds_| rather than
returning here.
The motivation was not to keep an additional field just for tests though.

[sky, 2015-03-16 22:56:09]

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc#...
ui/views/controls/label.cc:345: return gfx::Rect();
On 2015/03/16 22:05:37, Jun Mukai wrote:
> On 2015/03/16 21:29:57, sky wrote:
> > Is there a reason not to always return focus_rect? And why is this returning
> the
> > focus rect in the first place? If you say so you can get the focus rect can
we
> > move that into a separate function?
> 
> Changed to keep the focus bounds as a field |focus_bounds_| rather than
> returning here.
> The motivation was not to keep an additional field just for tests though.

Why use a field at all for this? The focus rect is only needed if the label has
focus. I prefer a standalone function called from PaintText that calculates it.
Your test can call the same function.

[Jun Mukai, 2015-03-17 00:03:47]

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc
File ui/views/controls/label.cc (right):

https://codereview.chromium.org/1018463004/diff/1/ui/views/controls/label.cc#...
ui/views/controls/label.cc:345: return gfx::Rect();
On 2015/03/16 22:56:09, sky wrote:
> On 2015/03/16 22:05:37, Jun Mukai wrote:
> > On 2015/03/16 21:29:57, sky wrote:
> > > Is there a reason not to always return focus_rect? And why is this
returning
> > the
> > > focus rect in the first place? If you say so you can get the focus rect
can
> we
> > > move that into a separate function?
> > 
> > Changed to keep the focus bounds as a field |focus_bounds_| rather than
> > returning here.
> > The motivation was not to keep an additional field just for tests though.
> 
> Why use a field at all for this? The focus rect is only needed if the label
has
> focus. I prefer a standalone function called from PaintText that calculates
it.
> Your test can call the same function.

I see. separated as GetFocusBounds().

[sky, 2015-03-17 17:12:44]

LGTM

[Jun Mukai, 2015-03-17 17:13:40]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2015-03-17 17:14:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1018463004/40001

[commit-bot: I haz the power, 2015-03-17 18:11:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-17 18:14:31]

Patchset 3 (id:??) landed as
https://crrev.com/26abb0bf78b3e67e11b7592d057a7f536bb40f3d
Cr-Commit-Position: refs/heads/master@{#320943}