Magnifier needs to follow the focus of the textfield.

Magnifier needs to follow the focus and caret change of the textfield.

BUG=426232
Committed: https://crrev.com/91b6ed0c33013c73e91b5c8c8be57f340a76f7a6
Cr-Commit-Position: refs/heads/master@{#300955}

[dmazzoni, 2014-10-21 05:44:19]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2014-10-21 05:44:19]

Great!

Here's a quick outline of how I think you could also track when other web links
and controls get focus:

In RenderViewImpl::focusedNodeChanged, cast the WebNode to a WebElement and call
boundsInViewportSpace to get its bounds, then pass that through in the IPC.

The IPC is handled in RenderViewHostImpl::OnFocusedNodeChanged, where you could
retrieve the rectangle and add it as an additional argument to the
NOTIFICATION_FOCUS_CHANGED_IN_PAGE notification.

Then you just need to handle that notification and move focus. Note that that
notification already has a bool telling you whether it's editable text or not.
Presumably you could ignore any focus on an editable text node since you're
already tracking that with IME code.

Hope that's helpful!

[jennyz, 2014-10-21 17:18:40]

Hi Dominic,

Thanks so much for your suggestion, it looks like a promising path to me
for tracking the focus change on other web link and controls. I will head
down for further investigation on that path. As we talked yesterday, we
will file a separate bug for this one, and I will make another cl for it.

Thanks a lot!

Jenny

On Mon, Oct 20, 2014 at 10:44 PM, <dmazzoni@chromium.org> wrote:

> Great!
>
> Here's a quick outline of how I think you could also track when other web
> links
> and controls get focus:
>
> In RenderViewImpl::focusedNodeChanged, cast the WebNode to a WebElement
> and call
> boundsInViewportSpace to get its bounds, then pass that through in the IPC.
>
> The IPC is handled in RenderViewHostImpl::OnFocusedNodeChanged, where you
> could
> retrieve the rectangle and add it as an additional argument to the
> NOTIFICATION_FOCUS_CHANGED_IN_PAGE notification.
>
> Then you just need to handle that notification and move focus. Note that
> that
> notification already has a bool telling you whether it's editable text or
> not.
> Presumably you could ignore any focus on an editable text node since you're
> already tracking that with IME code.
>
> Hope that's helpful!
>
>
> https://codereview.chromium.org/665903003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jennyz, 2014-10-22 21:48:12]

Patchset #2 (id:20001) has been deleted

[jennyz, 2014-10-22 21:54:26]

jennyz@chromium.org changed reviewers:
+ oshima@chromium.org

[jennyz, 2014-10-22 21:54:44]

jennyz@chromium.org changed reviewers:
+ mukai@chromium.org

[jennyz, 2014-10-22 22:13:44]

I will add a browser test for testing the text input on web content later, for
catching up the M40 feature check in deadline this Friday, I think we can review
this cl first.

[Jun Mukai, 2014-10-22 22:22:53]

mukai@chromium.org changed reviewers:
+ shuchen@chromium.org

[Jun Mukai, 2014-10-22 22:22:54]

+shuchen

https://codereview.chromium.org/665903003/diff/40001/ui/base/ime/input_method...
File ui/base/ime/input_method_chromeos.h (right):

https://codereview.chromium.org/665903003/diff/40001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.h:117: bool IsInputFieldFocused();
It is really weird to have IsInputFieldFocused and IsTextInputFieldFocused at
the same time... :-/

Can we rename this method?  An idea: IsNonPasswordInputFieldFocused().  shuchen
may have some other opinions though.

[jennyz, 2014-10-22 22:29:00]

https://codereview.chromium.org/665903003/diff/40001/ui/base/ime/input_method...
File ui/base/ime/input_method_chromeos.h (right):

https://codereview.chromium.org/665903003/diff/40001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.h:117: bool IsInputFieldFocused();
On 2014/10/22 22:22:54, Jun Mukai wrote:
> It is really weird to have IsInputFieldFocused and IsTextInputFieldFocused at
> the same time... :-/
> 
> Can we rename this method?  An idea: IsNonPasswordInputFieldFocused(). 
shuchen
> may have some other opinions though.
Yes, it does sound weird. The only reason I didn't rename it is because it is
used in many places, I am trying to keep it as is. If no one is annoyed for
renaming it, I will go ahead rename it as Is
IsNonPasswordInputFieldFocus().

[Shu Chen, 2014-10-23 08:03:57]

https://codereview.chromium.org/665903003/diff/40001/ui/base/ime/input_method...
File ui/base/ime/input_method_chromeos.h (right):

https://codereview.chromium.org/665903003/diff/40001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.h:117: bool IsInputFieldFocused();
On 2014/10/22 22:29:00, jennyz wrote:
> On 2014/10/22 22:22:54, Jun Mukai wrote:
> > It is really weird to have IsInputFieldFocused and IsTextInputFieldFocused
at
> > the same time... :-/
> > 
> > Can we rename this method?  An idea: IsNonPasswordInputFieldFocused(). 
> shuchen
> > may have some other opinions though.
> Yes, it does sound weird. The only reason I didn't rename it is because it is
> used in many places, I am trying to keep it as is. If no one is annoyed for
> renaming it, I will go ahead rename it as Is
> IsNonPasswordInputFieldFocus().

Looks like it is only used in ui/base/ime/input_method_chromeos.cc, which you
already included in this cl.
So please rename it for better readability. Thanks!

[jennyz, 2014-10-23 18:10:11]

https://codereview.chromium.org/665903003/diff/40001/ui/base/ime/input_method...
File ui/base/ime/input_method_chromeos.h (right):

https://codereview.chromium.org/665903003/diff/40001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.h:117: bool IsInputFieldFocused();
On 2014/10/23 08:03:57, Shu Chen wrote:
> On 2014/10/22 22:29:00, jennyz wrote:
> > On 2014/10/22 22:22:54, Jun Mukai wrote:
> > > It is really weird to have IsInputFieldFocused and IsTextInputFieldFocused
> at
> > > the same time... :-/
> > > 
> > > Can we rename this method?  An idea: IsNonPasswordInputFieldFocused(). 
> > shuchen
> > > may have some other opinions though.
> > Yes, it does sound weird. The only reason I didn't rename it is because it
is
> > used in many places, I am trying to keep it as is. If no one is annoyed for
> > renaming it, I will go ahead rename it as Is
> > IsNonPasswordInputFieldFocus().
> 
> Looks like it is only used in ui/base/ime/input_method_chromeos.cc, which you
> already included in this cl.
> So please rename it for better readability. Thanks!

Done.

https://codereview.chromium.org/665903003/diff/40001/ui/base/ime/input_method...
ui/base/ime/input_method_chromeos.h:117: bool IsInputFieldFocused();
On 2014/10/22 22:22:54, Jun Mukai wrote:
> It is really weird to have IsInputFieldFocused and IsTextInputFieldFocused at
> the same time... :-/
> 
> Can we rename this method?  An idea: IsNonPasswordInputFieldFocused(). 
shuchen
> may have some other opinions though.

Done.

[Jun Mukai, 2014-10-23 18:22:42]

lgtm for ui/base/ime

[oshima, 2014-10-23 18:36:48]

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller.cc:169: // ui::InputMethodObserver
overrides:
// ui::InputMethodObserver:

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller.cc:206: ui::InputMethod* input_method_;
document ownership.

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller.cc:605: return
root_window_->GetProperty(aura::client::kRootWindowInputMethodKey);
optinoal: you may just want to inline this.

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
File ash/magnifier/magnification_controller_unittest.cc (right):

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller_unittest.cc:37: text_field_ = new
views::Textfield;
move this to initializer list.

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller_unittest.cc:144: TextInputView*
text_input_view_;
initialize this in ctor. (you may use nullptr)

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller_unittest.cc:148:
aura::client::kRootWindowInputMethodKey);
text_input_view_->GetWidget()->GetNativeWindow()->GetRootWindow()

would be more portable.

https://codereview.chromium.org/665903003/diff/60001/ui/base/ime/input_method...
File ui/base/ime/input_method_base.h (right):

https://codereview.chromium.org/665903003/diff/60001/ui/base/ime/input_method...
ui/base/ime/input_method_base.h:84: void NotifyTextInputCaretBoundsChanged(const
TextInputClient* client);
document this method (primarily for consistency with other methods)

[jennyz, 2014-10-23 19:34:56]

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
File ash/magnifier/magnification_controller.cc (right):

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller.cc:169: // ui::InputMethodObserver
overrides:
On 2014/10/23 18:36:49, oshima wrote:
> // ui::InputMethodObserver:

Done.

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller.cc:206: ui::InputMethod* input_method_;
On 2014/10/23 18:36:49, oshima wrote:
> document ownership.

Done.

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller.cc:605: return
root_window_->GetProperty(aura::client::kRootWindowInputMethodKey);
On 2014/10/23 18:36:49, oshima wrote:
> optinoal: you may just want to inline this.

Done.

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
File ash/magnifier/magnification_controller_unittest.cc (right):

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller_unittest.cc:37: text_field_ = new
views::Textfield;
On 2014/10/23 18:36:49, oshima wrote:
> move this to initializer list.

Done.

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller_unittest.cc:144: TextInputView*
text_input_view_;
On 2014/10/23 18:36:49, oshima wrote:
> initialize this in ctor. (you may use nullptr)

Done.

https://codereview.chromium.org/665903003/diff/60001/ash/magnifier/magnificat...
ash/magnifier/magnification_controller_unittest.cc:148:
aura::client::kRootWindowInputMethodKey);
On 2014/10/23 18:36:49, oshima wrote:
> text_input_view_->GetWidget()->GetNativeWindow()->GetRootWindow()
> 
> would be more portable.

Done.

https://codereview.chromium.org/665903003/diff/60001/ui/base/ime/input_method...
File ui/base/ime/input_method_base.h (right):

https://codereview.chromium.org/665903003/diff/60001/ui/base/ime/input_method...
ui/base/ime/input_method_base.h:84: void NotifyTextInputCaretBoundsChanged(const
TextInputClient* client);
On 2014/10/23 18:36:49, oshima wrote:
> document this method (primarily for consistency with other methods)

Done.

[oshima, 2014-10-23 20:22:36]

lgtm

[jennyz, 2014-10-23 20:30:24]

The CQ bit was checked by jennyz@chromium.org

[commit-bot: I haz the power, 2014-10-23 20:32:27]

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

[commit-bot: I haz the power, 2014-10-23 21:52:51]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2014-10-23 21:53:32]

Patchset 4 (id:??) landed as
https://crrev.com/91b6ed0c33013c73e91b5c8c8be57f340a76f7a6
Cr-Commit-Position: refs/heads/master@{#300955}