Show the Translate bubble when the content web view is not focused

Show the Translate bubble when the content web view is not focused

This CL changes the logic to determine whether the Translate bubble is
to be shown. Before this CL, it is not considered the case when the
contents web view is NOT focused. This CL changes this logic to show the
bubble when the content webview and an input element is focused and the
user is NOT trying to show the bubble explicitly. The latter condition
is checked by GetLocationBarView()->IsMouseHovered() because just after
clicking the Translate icon on the Omnibox, the content web view might
still be focused.

BUG=372755
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271966

[hajimehoshi, 2014-05-15 11:08:44]

PTAL

[sky, 2014-05-15 16:34:56]

https://codereview.chromium.org/289983002/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/289983002/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1163:
!GetLocationBarView()->IsMouseHovered()) {
Why the hovered check? What about users that are using the keyboard?

[hajimehoshi, 2014-05-16 03:49:50]

Thank you!

https://codereview.chromium.org/289983002/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/289983002/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1163:
!GetLocationBarView()->IsMouseHovered()) {
On 2014/05/15 16:34:56, sky wrote:
> Why the hovered check?

It's because when an input element is focused and the user is clicking the
Translate icon, the Translate bubble should be shown but the content web view
still has the focus even after clicking the icon.

Ideally, checking hovered is not needed but the reality is that the Translate
icon doesn't take focus and clicking the icon doesn't move the focus. As an
experiment, I changed the Translate icon to be able to take the focus, but the
situation didn't change.

> What about users that are using the keyboard?

Hmm, we need to consider the case when the keyboard shortcut or operation for
translation can be performed eventually...

AFAIK, the Translate feature doesn't have any keyboard shortcuts. Additionally,
the Translate icon doesn't take the focus, so it is impossible to launch the
Translate bubble from keyboard for now.

[sky, 2014-05-16 17:00:53]

https://codereview.chromium.org/289983002/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/289983002/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1163:
!GetLocationBarView()->IsMouseHovered()) {
On 2014/05/16 03:49:50, hajimehoshi wrote:
> On 2014/05/15 16:34:56, sky wrote:
> > Why the hovered check?
> 
> It's because when an input element is focused and the user is clicking the
> Translate icon, the Translate bubble should be shown but the content web view
> still has the focus even after clicking the icon.
> 
> Ideally, checking hovered is not needed but the reality is that the Translate
> icon doesn't take focus and clicking the icon doesn't move the focus. As an
> experiment, I changed the Translate icon to be able to take the focus, but the
> situation didn't change.
> 
> > What about users that are using the keyboard?
> 
> Hmm, we need to consider the case when the keyboard shortcut or operation for
> translation can be performed eventually...
> 
> AFAIK, the Translate feature doesn't have any keyboard shortcuts.
Additionally,
> the Translate icon doesn't take the focus, so it is impossible to launch the
> Translate bubble from keyboard for now.
> 

Sorry, I still don't get it. Could you outline in more detail how we end up here
and why we care about focus? (Sorry)

[hajimehoshi, 2014-05-19 03:57:51]

Sure, let me explain:

* The background

Originally there existed a bug (crbug/313100) that the Translate bubble was
shown even though the user was editing something at an <input>. When the
Translate bubble was shown, the bubble stole the focus from the user
immediately, which was very annoying to the users. The patch r269295 fixed the
problem to suppress Translate bubble when the focus was on an editable element
like <input> or <textarea>. However, the patch didn't consider whether the
content web view was focused or not, so the user can't open the Translate bubble
by clicking the icon when he/she is editing something. I thought that clicking
the icon would steal the focus and checking the content web view would work well
then.


* What this patch is doing

This patch moved the if clause (whether an editable element is focused) from
TranslateTabHelper to BrowserView because it is needed to check whether the
content web view is focused or not. The if clause is changed to check whether
(1) the content view is focused && (2) an editable element is focused.

The problem is that (1) doesn't work well. The Translate icon doesn't take
focus. Just after clicking the icon, the content web view still has focus, which
means that the Translate bubble can't be shown. To avoid this, I added a little
hacky code you pointed out. Checking the mouse cursor is on Omnibox seems to be
working well now, but this patch doesn't consider about keyboard. I have no idea
about that.

I wonder if you could give me some advice.

[sky, 2014-05-19 16:05:05]

How about making the bubble not take focus when shown? Shouldn't that fix all
issues and mean you don't have to worry about where focus is if the user clicks
on it? See BubbleDelegateView::set_use_focusless.

[sky, 2014-05-19 16:05:31]

And thanks for the great explanation!

[hajimehoshi, 2014-05-19 17:08:20]

Thanks! Your suggestion could be a great solution, but I need to ask the UI team
about that.

[hajimehoshi, 2014-05-20 03:22:11]

On 2014/05/19 17:08:20, hajimehoshi wrote:
> Thanks! Your suggestion could be a great solution, but I need to ask the UI
team
> about that.

Kenjibaheux and I discussed about that. We concluded we don't want to change the
bubble behavior because:

1. The bubble might hide the input element where the user is editing something.
2. Changing the behavior can produce other problems and we think it is not the
time. We'd like to retain our idea for now. Of cause, we may change the behavior
later.

It'd be the best if I could fix the current icon problem. If I can't, I need to
revert r269295...

[sky, 2014-05-20 13:21:01]

On Mon, May 19, 2014 at 8:22 PM,  <hajimehoshi@chromium.org> wrote:
> On 2014/05/19 17:08:20, hajimehoshi wrote:
>>
>> Thanks! Your suggestion could be a great solution, but I need to ask the
>> UI
>
> team
>>
>> about that.
>
>
> Kenjibaheux and I discussed about that. We concluded we don't want to change
> the
> bubble behavior because:
>
> 1. The bubble might hide the input element where the user is editing
> something.

The bubble is pretty small and shown over a portion of the page that
seems rather unlikely to contain input.

> 2. Changing the behavior can produce other problems and we think it is not
> the
> time. We'd like to retain our idea for now. Of cause, we may change the
> behavior
> later.

Can you elaborate on what those problems are?

  -Scott
>
> It'd be the best if I could fix the current icon problem. If I can't, I need
> to
> revert r269295...
>
>
> https://codereview.chromium.org/289983002/

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

[sky, 2014-05-20 17:25:29]

On 2014/05/20 13:21:01, sky wrote:
> On Mon, May 19, 2014 at 8:22 PM,  <mailto:hajimehoshi@chromium.org> wrote:
> > On 2014/05/19 17:08:20, hajimehoshi wrote:
> >>
> >> Thanks! Your suggestion could be a great solution, but I need to ask the
> >> UI
> >
> > team
> >>
> >> about that.
> >
> >
> > Kenjibaheux and I discussed about that. We concluded we don't want to change
> > the
> > bubble behavior because:
> >
> > 1. The bubble might hide the input element where the user is editing
> > something.
> 
> The bubble is pretty small and shown over a portion of the page that
> seems rather unlikely to contain input.
> 
> > 2. Changing the behavior can produce other problems and we think it is not
> > the
> > time. We'd like to retain our idea for now. Of cause, we may change the
> > behavior
> > later.
> 
> Can you elaborate on what those problems are?
> 
>   -Scott
> >
> > It'd be the best if I could fix the current icon problem. If I can't, I need
> > to
> > revert r269295...
> >
> >
> > https://codereview.chromium.org/289983002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

One other thing to consider. Some pages grab focus when shown and place it on an
editable field, such as google.com. If we determine the translate bubble should
be shown but don't show because focus happens to be in an editable field that
seems a bit wrong. Maybe a better sign would be if focus isn't in the default
field, or if the user has modified the content.

[kenjibaheux, 2014-05-21 00:31:38]

On 2014/05/20 17:25:29, sky wrote:
> On 2014/05/20 13:21:01, sky wrote:
> > On Mon, May 19, 2014 at 8:22 PM,  <mailto:hajimehoshi@chromium.org> wrote:
> > > On 2014/05/19 17:08:20, hajimehoshi wrote:
> > >>
> > >> Thanks! Your suggestion could be a great solution, but I need to ask the
> > >> UI
> > >
> > > team
> > >>
> > >> about that.
> > >
> > >
> > > Kenjibaheux and I discussed about that. We concluded we don't want to
change
> > > the
> > > bubble behavior because:
> > >
> > > 1. The bubble might hide the input element where the user is editing
> > > something.
> > 
> > The bubble is pretty small and shown over a portion of the page that
> > seems rather unlikely to contain input.

> > 
> > > 2. Changing the behavior can produce other problems and we think it is not
> > > the
> > > time. We'd like to retain our idea for now. Of cause, we may change the
> > > behavior
> > > later.
> > 
> > Can you elaborate on what those problems are?


I agree that this should be a minor issue but we did hear some folks complain
about it. That being said, I believe that the root cause is that they actually
can't decide to use one of the never options because they would use the
translate feature on some pages if they have difficulties with some
words/sentences. We plan to just stop showing the bubble after hitting some
threshold (most likely via a finch experiment to check the impact on metrics).

I've cc-ed Rebecca from the UX team to help us make the right decision.

> > 
> >   -Scott
> > >
> > > It'd be the best if I could fix the current icon problem. If I can't, I
need
> > > to
> > > revert r269295...
> > >
> > >
> > > https://codereview.chromium.org/289983002/
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> One other thing to consider. Some pages grab focus when shown and place it on
an
> editable field, such as http://google.com. If we determine the translate
bubble should
> be shown but don't show because focus happens to be in an editable field that
> seems a bit wrong. Maybe a better sign would be if focus isn't in the default
> field, or if the user has modified the content.

[kenjibaheux, 2014-05-21 03:26:24]

Ideally, we would have like to make the initial change in time for M36.
Could we finish off this patch as planned and continue the discussion for M37+
(as explained there are a couple of extra adjustments we want to make).

Thanks!


On 2014/05/21 00:31:38, kenjibaheux wrote:
> On 2014/05/20 17:25:29, sky wrote:
> > On 2014/05/20 13:21:01, sky wrote:
> > > On Mon, May 19, 2014 at 8:22 PM,  <mailto:hajimehoshi@chromium.org> wrote:
> > > > On 2014/05/19 17:08:20, hajimehoshi wrote:
> > > >>
> > > >> Thanks! Your suggestion could be a great solution, but I need to ask
the
> > > >> UI
> > > >
> > > > team
> > > >>
> > > >> about that.
> > > >
> > > >
> > > > Kenjibaheux and I discussed about that. We concluded we don't want to
> change
> > > > the
> > > > bubble behavior because:
> > > >
> > > > 1. The bubble might hide the input element where the user is editing
> > > > something.
> > > 
> > > The bubble is pretty small and shown over a portion of the page that
> > > seems rather unlikely to contain input.
> 
> > > 
> > > > 2. Changing the behavior can produce other problems and we think it is
not
> > > > the
> > > > time. We'd like to retain our idea for now. Of cause, we may change the
> > > > behavior
> > > > later.
> > > 
> > > Can you elaborate on what those problems are?
> 
> 
> I agree that this should be a minor issue but we did hear some folks complain
> about it. That being said, I believe that the root cause is that they actually
> can't decide to use one of the never options because they would use the
> translate feature on some pages if they have difficulties with some
> words/sentences. We plan to just stop showing the bubble after hitting some
> threshold (most likely via a finch experiment to check the impact on metrics).
> 
> I've cc-ed Rebecca from the UX team to help us make the right decision.
> 
> > > 
> > >   -Scott
> > > >
> > > > It'd be the best if I could fix the current icon problem. If I can't, I
> need
> > > > to
> > > > revert r269295...
> > > >
> > > >
> > > > https://codereview.chromium.org/289983002/
> > > 
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > 
> > One other thing to consider. Some pages grab focus when shown and place it
on
> an
> > editable field, such as http://google.com. If we determine the translate
> bubble should
> > be shown but don't show because focus happens to be in an editable field
that
> > seems a bit wrong. Maybe a better sign would be if focus isn't in the
default
> > field, or if the user has modified the content.

[sky, 2014-05-21 04:02:43]

LGTM

[hajimehoshi, 2014-05-21 04:30:03]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-05-21 04:30:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/289983002/1

[commit-bot: I haz the power, 2014-05-21 20:38:32]

Change committed as 271966