focus reverse traversal was not working for TextfieldViews.

focus reverse traversal was not working for TextfieldViews.

 Both Textfield and TextfieldViews were focusable and
alt-tab was setting the focus back to the Textfield, which
sets the focus to the its TextfieldViews. This fixes the issue
by making Textfield non focusable.

BUG=none
TEST=FocusTest is added to unittest.

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

[sky, 2011-01-05 00:34:06]

If you do this, what happens if you invoke RequestFocus on textfield?

[oshima, 2011-01-05 18:37:44]

On 2011/01/05 00:34:06, sky wrote:
> If you do this, what happens if you invoke RequestFocus on textfield?

Good point. It used to work via ::Focus method, but it will stop working with
this change. How would you recommend to fix this?
Here are alternatives I can think of:

1) Delegate RequestFocus to wrapper.
2) Keep the focus on Textfield and forward all events from Textfield to wrapper.
3) Allow a view to return the view that events should be forwarded to when it
has a focus. (like Views::GetEventForwardView()).
4) Make Textfield class abstract and have a factory method that returns
NativeTextfield or TextfieldViews implementation.

any other? (May be using focus group or FocusTraversal?)

[sky, 2011-01-05 19:13:46]

On Wed, Jan 5, 2011 at 10:37 AM,  <oshima@chromium.org> wrote:
> On 2011/01/05 00:34:06, sky wrote:
>>
>> If you do this, what happens if you invoke RequestFocus on textfield?
>
> Good point. It used to work via ::Focus method, but it will stop working
> with
> this change. How would you recommend to fix this?
> Here are alternatives I can think of:
>
> 1) Delegate RequestFocus to wrapper.
> 2) Keep the focus on Textfield and forward all events from Textfield to
> wrapper.
> 3) Allow a view to return the view that events should be forwarded to when
> it
> has a focus. (like Views::GetEventForwardView()).
> 4) Make Textfield class abstract and have a factory method that returns
> NativeTextfield or TextfieldViews implementation.
>
> any other? (May be using focus group or FocusTraversal?)

I tend to like 1, but I suspect it'll have the same problem you're
trying to fix now. In fact Textfield::Focus delegates SetFocus to the
wrapper now.
My next thought was 4, but that means you'll have to change all code
that creates a Textfield. Which is ick. It also means you can't create
them by value, which I'm sure we do in some places. Ben wasn't too
keen on it either.
That leaves 2 or 3. Of those, I tend to like explicitly forwarding
events to the wrapper class. It's less invasive and you can fine tune
exactly the set you want to send.

I've cc'd Jay in case he has any thoughts on this.

  -Scott

[oshima, 2011-01-05 19:46:46]

On Wed, Jan 5, 2011 at 11:13 AM, Scott Violet <sky@chromium.org> wrote:

> On Wed, Jan 5, 2011 at 10:37 AM,  <oshima@chromium.org> wrote:
> > On 2011/01/05 00:34:06, sky wrote:
> >>
> >> If you do this, what happens if you invoke RequestFocus on textfield?
> >
> > Good point. It used to work via ::Focus method, but it will stop working
> > with
> > this change. How would you recommend to fix this?
> > Here are alternatives I can think of:
> >
> > 1) Delegate RequestFocus to wrapper.
> > 2) Keep the focus on Textfield and forward all events from Textfield to
> > wrapper.
> > 3) Allow a view to return the view that events should be forwarded to
> when
> > it
> > has a focus. (like Views::GetEventForwardView()).
> > 4) Make Textfield class abstract and have a factory method that returns
> > NativeTextfield or TextfieldViews implementation.
> >
> > any other? (May be using focus group or FocusTraversal?)
>
> I tend to like 1, but I suspect it'll have the same problem you're
> trying to fix now. In fact Textfield::Focus delegates SetFocus to the
> wrapper now.
>

The issue of RequestFoucs is that default impl does not work if the view is
not focusable.
If I delegate this to wrapper, the views wrapper can handle it
correctly. The down side of this
approach is that there may be other edge cases that does not work and could
be fragile.


> My next thought was 4, but that means you'll have to change all code
> that creates a Textfield. Which is ick. It also means you can't create
> them by value, which I'm sure we do in some places. Ben wasn't too
> keen on it either.
> That leaves 2 or 3. Of those, I tend to like explicitly forwarding
> events to the wrapper class. It's less invasive and you can fine tune
> exactly the set you want to send.
>

With 2 , the wrapper interface will be contaminated by event related methods
that
are only used for views, which I don't like. In any case, I don't have
strong opinion
on which way is better as none of them sounds absolutely better than others.
Jay, what is your thought on this?


> I've cc'd Jay in case he has any thoughts on this.
>
>  -Scott
>

[oshima, 2011-01-05 22:23:56]

I uploaded the patch that uses 2nd approach. Scott, please take a look and let
me know what you think. try-bot's redness is due to but issue but will test it
again when it's back.

On 2011/01/05 19:46:46, oshima wrote:
> On Wed, Jan 5, 2011 at 11:13 AM, Scott Violet <mailto:sky@chromium.org> wrote:
> 
> > On Wed, Jan 5, 2011 at 10:37 AM,  <mailto:oshima@chromium.org> wrote:
> > > On 2011/01/05 00:34:06, sky wrote:
> > >>
> > >> If you do this, what happens if you invoke RequestFocus on textfield?
> > >
> > > Good point. It used to work via ::Focus method, but it will stop working
> > > with
> > > this change. How would you recommend to fix this?
> > > Here are alternatives I can think of:
> > >
> > > 1) Delegate RequestFocus to wrapper.
> > > 2) Keep the focus on Textfield and forward all events from Textfield to
> > > wrapper.
> > > 3) Allow a view to return the view that events should be forwarded to
> > when
> > > it
> > > has a focus. (like Views::GetEventForwardView()).
> > > 4) Make Textfield class abstract and have a factory method that returns
> > > NativeTextfield or TextfieldViews implementation.
> > >
> > > any other? (May be using focus group or FocusTraversal?)
> >
> > I tend to like 1, but I suspect it'll have the same problem you're
> > trying to fix now. In fact Textfield::Focus delegates SetFocus to the
> > wrapper now.
> >
> 
> The issue of RequestFoucs is that default impl does not work if the view is
> not focusable.
> If I delegate this to wrapper, the views wrapper can handle it
> correctly. The down side of this
> approach is that there may be other edge cases that does not work and could
> be fragile.
> 
> 
> > My next thought was 4, but that means you'll have to change all code
> > that creates a Textfield. Which is ick. It also means you can't create
> > them by value, which I'm sure we do in some places. Ben wasn't too
> > keen on it either.
> > That leaves 2 or 3. Of those, I tend to like explicitly forwarding
> > events to the wrapper class. It's less invasive and you can fine tune
> > exactly the set you want to send.
> >
> 
> With 2 , the wrapper interface will be contaminated by event related methods
> that
> are only used for views, which I don't like. In any case, I don't have
> strong opinion
> on which way is better as none of them sounds absolutely better than others.
> Jay, what is your thought on this?
> 
> 
> > I've cc'd Jay in case he has any thoughts on this.
> >
> >  -Scott
> >

[sky, 2011-01-05 22:57:35]

This approach seems fine to me.

  -Scott

http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_gtk.h (right):

http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
views/controls/textfield/native_textfield_gtk.h:51: virtual bool
OnKeyPressed(const views::KeyEvent& e) { return false; }
Don't inline virtual methods.

http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_win.h (right):

http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
views/controls/textfield/native_textfield_win.h:69: virtual bool
OnKeyPressed(const views::KeyEvent& e) { return false; }
Don't inline virtual methods.

http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_wrapper.h (right):

http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
views/controls/textfield/native_textfield_wrapper.h:81: // Sets the focus to the
text field. Returns fale if the wrapper
fale -> false

http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
views/controls/textfield/native_textfield_wrapper.h:97: virtual bool
OnKeyPressed(const views::KeyEvent& e) = 0;
I'm a bit worried about naming these exactly the same as the views methods.
Doesn't that lead to a weird inheritance problems?

[oshima, 2011-01-05 23:43:05]

On 2011/01/05 22:57:35, sky wrote:
> This approach seems fine to me.
> 
>   -Scott
> 
>
http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
> File views/controls/textfield/native_textfield_gtk.h (right):
> 
>
http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_gtk.h:51: virtual bool
> OnKeyPressed(const views::KeyEvent& e) { return false; }
> Don't inline virtual methods.
done
> 
>
http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
> File views/controls/textfield/native_textfield_win.h (right):
> 
>
http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_win.h:69: virtual bool
> OnKeyPressed(const views::KeyEvent& e) { return false; }
> Don't inline virtual methods.
done
> 
>
http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
> File views/controls/textfield/native_textfield_wrapper.h (right):
> 
>
http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_wrapper.h:81: // Sets the focus to
the
> text field. Returns fale if the wrapper
> fale -> false
done

>
http://codereview.chromium.org/5988010/diff/22001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_wrapper.h:97: virtual bool
> OnKeyPressed(const views::KeyEvent& e) = 0;
> I'm a bit worried about naming these exactly the same as the views methods.
> Doesn't that lead to a weird inheritance problems?

This is legal in C++ and works as expected. That's being said, it's better to
add assertion to view's method to make sure we don't set focus to this instance
by accident. Renamed them to HandleXX and changed view's implementation to fail
with NOTRACHED() in debug build.

[sky, 2011-01-06 00:01:56]

LGTM

[Jay Civelli, 2011-01-06 00:55:54]

Some minor comments.
I guess another fix would have been to have some sort of
View::ShouldSkipViewOnFocusTraversal(bool reverse) method.
The focus manager when advancing focus for focus traversal could have ignored
such views when they returned true. (like it ignores right now non focusable
ones)

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:100: // gain a focus.
Nit: gain a focus -> gain focus.

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:62: void
InitTextfield(Textfield::StyleFlags style, int count) {
May be it should be named: InitMultipleTextfields?

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:330: EXPECT_EQ(3,
GetFocusedView()->GetID());
May be you could advance focus one more time to make sure it cycles OK?
You could then to a RequestFocus on the 3rd textfield to try the reverse
traversal. (with the added benefits of ensuring requestfocus still works as
expected).

[oshima, 2011-01-06 01:31:22]

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views.cc (right):

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views.cc:100: // gain a focus.
On 2011/01/06 00:55:54, Jay Civelli wrote:
> Nit: gain a focus -> gain focus.

Done. fixed other places as well.

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
File views/controls/textfield/native_textfield_views_unittest.cc (right):

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:62: void
InitTextfield(Textfield::StyleFlags style, int count) {
On 2011/01/06 00:55:54, Jay Civelli wrote:
> May be it should be named: InitMultipleTextfields?

renamed to InitTextfiels (Multiple sounded a bit verbose)

http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
views/controls/textfield/native_textfield_views_unittest.cc:330: EXPECT_EQ(3,
GetFocusedView()->GetID());
On 2011/01/06 00:55:54, Jay Civelli wrote:
> May be you could advance focus one more time to make sure it cycles OK?
> You could then to a RequestFocus on the 3rd textfield to try the reverse
> traversal. (with the added benefits of ensuring requestfocus still works as
> expected).

Thank you for good suggestion. Added a few more cases.

[oshima, 2011-01-06 01:35:19]

On 2011/01/06 00:55:54, Jay Civelli wrote:
> Some minor comments.
> I guess another fix would have been to have some sort of
> View::ShouldSkipViewOnFocusTraversal(bool reverse) method.
> The focus manager when advancing focus for focus traversal could have ignored
> such views when they returned true. (like it ignores right now non focusable
> ones)

Scott, what do you think? My current feeling, after updating the change, it may
be better to keep the focus on Textfield because that's what views framework
might expect, but i can be convinced eitherway. 



> 
>
http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
> File views/controls/textfield/native_textfield_views.cc (right):
> 
>
http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_views.cc:100: // gain a focus.
> Nit: gain a focus -> gain focus.
> 
>
http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
> File views/controls/textfield/native_textfield_views_unittest.cc (right):
> 
>
http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_views_unittest.cc:62: void
> InitTextfield(Textfield::StyleFlags style, int count) {
> May be it should be named: InitMultipleTextfields?
> 
>
http://codereview.chromium.org/5988010/diff/37001/views/controls/textfield/na...
> views/controls/textfield/native_textfield_views_unittest.cc:330: EXPECT_EQ(3,
> GetFocusedView()->GetID());
> May be you could advance focus one more time to make sure it cycles OK?
> You could then to a RequestFocus on the 3rd textfield to try the reverse
> traversal. (with the added benefits of ensuring requestfocus still works as
> expected).

[Jay Civelli, 2011-01-06 01:52:04]

LGTM

[oshima, 2011-01-06 02:24:22]

On 2011/01/06 01:52:04, Jay Civelli wrote:
> LGTM


I'm going to commit it for now. Please let me know if I should try Jay's
proposal.

[sky, 2011-01-06 18:05:59]

I like Jay's proposal. It has the advantage that it would work in
other places that have a similar problem, and focus remains where we
want it. When you have time, it would be great to look into it more
deeply.

  -Scott

On Wed, Jan 5, 2011 at 6:24 PM,  <oshima@chromium.org> wrote:
> On 2011/01/06 01:52:04, Jay Civelli wrote:
>>
>> LGTM
>
>
> I'm going to commit it for now. Please let me know if I should try Jay's
> proposal.
>
> http://codereview.chromium.org/5988010/
>