NOT FOR COMMIT: Make altgr+left and altgr+right navigate back/forward on views.

NOT FOR COMMIT: Make altgr+left and altgr+right navigate back/forward on views.

This makes them work like alt+left and alt+right so that users
on European keyboard have a one-handed keyboard shortcut.
A followup patch will need to make this work on Mac and ChromeOS.

[ojan, 2016-05-27 00:53:54]

Peter, Rachel, here's the patch to make views work. Realistically, I won't be
able to get to finishing the one bit I comment below and adding tests until late
July. Also, of course, I won't be able to do Mac/ChromeOS before then either.
Would really appreciate if you could help find someone to take this over.

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

https://codereview.chromium.org/2013293002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:851: bool isAltGrLeftBackNavigation =
true;
This is the bit of code that needs to be finished to make this work on Linux.

[Peter Kasting, 2016-05-27 01:11:00]

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

[Peter Kasting, 2016-05-27 01:11:01]

Did you select the places to add IsAltGrDown() calls by auditing all the
existing IsAltDown() calls?  Or would someone need to audit the rest?  Are there
existing places where we want IsAltDown() to report false for AltGr (since if
not, we could combine the two)?

https://codereview.chromium.org/2013293002/diff/1/ui/base/accelerators/accele...
File ui/base/accelerators/accelerator.cc (right):

https://codereview.chromium.org/2013293002/diff/1/ui/base/accelerators/accele...
ui/base/accelerators/accelerator.cc:31: ui::EF_ALT_DOWN | ui::EF_ALTGR_DOWN |
ui::EF_COMMAND_DOWN;
Nit: 80 columns

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

https://codereview.chromium.org/2013293002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:543: e.IsShiftDown() || e.IsControlDown()
|| e.IsAltDown() || e.IsAltGrDown()) {
Nit: 80 columns

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

https://codereview.chromium.org/2013293002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:851: bool isAltGrLeftBackNavigation =
true;
On 2016/05/27 00:53:54, ojan wrote:
> This is the bit of code that needs to be finished to make this work on Linux.

Let me see if I understand.  This code needs to return "true" for any keyevent
that might normally be interpreted as some kind of global shortcut, but when
this textfield is focused needs to be captured by the textfield instead, in
order to be handled as an editing-specific key event.  Specifically, this block
looks for keystrokes that have some kind of system custom editing keybinding
(e.g. vi keybindings), which should result in MatchEvent() filling the vector
it's given.

The issue is that, for some reason, MatchEvent() is always returning true for
AltGr+Left, even though it has no apparent meaning, resulting in it being
captured by the edit control, resulting in the shortcut not working when a
top-chrome textfield is focused.  Is that right?

If this is the case my question would be why MatchEvent() would return true in
this case; and how the proposed solution here would avoid the issue of the user
intentionally doing a custom keybinding for AltGr+Left and then having it not
work as expected in this edit control.

Or have I totally misunderstood what's happening?

https://codereview.chromium.org/2013293002/diff/1/ui/views/focus/focus_manage...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/2013293002/diff/1/ui/views/focus/focus_manage...
ui/views/focus/focus_manager.cc:533: if (event.IsShiftDown() ||
event.IsControlDown() || event.IsAltDown() || event.IsAltGrDown())
Nit: 80 columns

[ojan, 2016-05-27 01:16:03]

On 2016/05/27 at 01:11:01, pkasting wrote:
> Did you select the places to add IsAltGrDown() calls by auditing all the
existing IsAltDown() calls?  Or would someone need to audit the rest?  Are there
existing places where we want IsAltDown() to report false for AltGr (since if
not, we could combine the two)?

I did my best to audit. I think I did a thorough job, but you never know. There
are places where we want IsAltDown and not IsAltGrDown.

>
https://codereview.chromium.org/2013293002/diff/1/ui/views/controls/textfield...
> File ui/views/controls/textfield/textfield.cc (right):
> 
>
https://codereview.chromium.org/2013293002/diff/1/ui/views/controls/textfield...
> ui/views/controls/textfield/textfield.cc:851: bool isAltGrLeftBackNavigation =
true;
> On 2016/05/27 00:53:54, ojan wrote:
> > This is the bit of code that needs to be finished to make this work on
Linux.
> 
> Let me see if I understand.  This code needs to return "true" for any keyevent
that might normally be interpreted as some kind of global shortcut, but when
this textfield is focused needs to be captured by the textfield instead, in
order to be handled as an editing-specific key event.  Specifically, this block
looks for keystrokes that have some kind of system custom editing keybinding
(e.g. vi keybindings), which should result in MatchEvent() filling the vector
it's given.
> 
> The issue is that, for some reason, MatchEvent() is always returning true for
AltGr+Left, even though it has no apparent meaning, resulting in it being
captured by the edit control, resulting in the shortcut not working when a
top-chrome textfield is focused.  Is that right?

Correct.

> If this is the case my question would be why MatchEvent() would return true in
this case; and how the proposed solution here would avoid the issue of the user
intentionally doing a custom keybinding for AltGr+Left and then having it not
work as expected in this edit control.
> 
> Or have I totally misunderstood what's happening?

You've got it exactly right. This is the part of the patch I didn't have time to
finish and to figure out the right way to do. Maybe erg@ has good ideas here.

[ojan, 2016-05-27 01:16:13]

Description was changed from

==========
Make altgr+left and altgr+right navigate back/forward on views.

This makes them work like alt+left and alt+right so that users
on European keyboard have a one-handed keyboard shortcut.
A followup patch will need to make this work on Mac and ChromeOS.
==========

to

==========
NOT FOR COMMIT: Make altgr+left and altgr+right navigate back/forward on views.

This makes them work like alt+left and alt+right so that users
on European keyboard have a one-handed keyboard shortcut.
A followup patch will need to make this work on Mac and ChromeOS.
==========

[Elliot Glaysher, 2016-05-27 21:53:28]

On 2016/05/27 01:16:03, ojan wrote:
> On 2016/05/27 at 01:11:01, pkasting wrote:
> > Did you select the places to add IsAltGrDown() calls by auditing all the
> existing IsAltDown() calls?  Or would someone need to audit the rest?  Are
there
> existing places where we want IsAltDown() to report false for AltGr (since if
> not, we could combine the two)?
> 
> I did my best to audit. I think I did a thorough job, but you never know.
There
> are places where we want IsAltDown and not IsAltGrDown.
> 
> >
>
https://codereview.chromium.org/2013293002/diff/1/ui/views/controls/textfield...
> > File ui/views/controls/textfield/textfield.cc (right):
> > 
> >
>
https://codereview.chromium.org/2013293002/diff/1/ui/views/controls/textfield...
> > ui/views/controls/textfield/textfield.cc:851: bool isAltGrLeftBackNavigation
=
> true;
> > On 2016/05/27 00:53:54, ojan wrote:
> > > This is the bit of code that needs to be finished to make this work on
> Linux.
> > 
> > Let me see if I understand.  This code needs to return "true" for any
keyevent
> that might normally be interpreted as some kind of global shortcut, but when
> this textfield is focused needs to be captured by the textfield instead, in
> order to be handled as an editing-specific key event.  Specifically, this
block
> looks for keystrokes that have some kind of system custom editing keybinding
> (e.g. vi keybindings), which should result in MatchEvent() filling the vector
> it's given.
> > 
> > The issue is that, for some reason, MatchEvent() is always returning true
for
> AltGr+Left, even though it has no apparent meaning, resulting in it being
> captured by the edit control, resulting in the shortcut not working when a
> top-chrome textfield is focused.  Is that right?
> 
> Correct.
> 
> > If this is the case my question would be why MatchEvent() would return true
in
> this case; and how the proposed solution here would avoid the issue of the
user
> intentionally doing a custom keybinding for AltGr+Left and then having it not
> work as expected in this edit control.
> > 
> > Or have I totally misunderstood what's happening?
> 
> You've got it exactly right. This is the part of the patch I didn't have time
to
> finish and to figure out the right way to do. Maybe erg@ has good ideas here.

I didn't know that there was a separate altgr modifier until just now so take
all of this with a grain of salt.

One possible thing to look at is all the places where instead of saying
"event->IsAltDown()", we do "event->flags() & EF_ALT_DOWN". Also, is altgr
actually being recorded on the key events on x11? I don't even know what that
corresponds to in the underlying x protocol.

[Peter Kasting, 2016-08-20 02:43:51]

Description was changed from

==========
NOT FOR COMMIT: Make altgr+left and altgr+right navigate back/forward on views.

This makes them work like alt+left and alt+right so that users
on European keyboard have a one-handed keyboard shortcut.
A followup patch will need to make this work on Mac and ChromeOS.
==========

to

==========
NOT FOR COMMIT: Make altgr+left and altgr+right navigate back/forward on views.

This makes them work like alt+left and alt+right so that users
on European keyboard have a one-handed keyboard shortcut.
A followup patch will need to make this work on Mac and ChromeOS.
==========

[Peter Kasting, 2016-08-20 02:44:31]

On 2016/05/27 00:53:54, ojan wrote:
> Peter, Rachel, here's the patch to make views work. Realistically, I won't be
> able to get to finishing the one bit I comment below and adding tests until
late
> July. Also, of course, I won't be able to do Mac/ChromeOS before then either.

It's after late July and none of the rest of us got to this :).  Any chance you
can pick this back up?

[Peter Kasting, 2017-02-11 02:04:48]

On 2016/08/20 02:44:31, Peter Kasting wrote:
> On 2016/05/27 00:53:54, ojan wrote:
> > Peter, Rachel, here's the patch to make views work. Realistically, I won't
be
> > able to get to finishing the one bit I comment below and adding tests until
> late
> > July. Also, of course, I won't be able to do Mac/ChromeOS before then
either.
> 
> It's after late July and none of the rest of us got to this :).  Any chance
you
> can pick this back up?

I'm guessing from the lack of response the answer was "no"... but this is still
relevant :)

[ojan, 2017-02-25 00:58:49]

Yeah, realistically, I just can't make time for this. Sorry. :(

On Fri, Feb 10, 2017 at 6:04 PM <pkasting@chromium.org> wrote:

> On 2016/08/20 02:44:31, Peter Kasting wrote:
> > On 2016/05/27 00:53:54, ojan wrote:
> > > Peter, Rachel, here's the patch to make views work. Realistically, I
> won't
> be
> > > able to get to finishing the one bit I comment below and adding tests
> until
> > late
> > > July. Also, of course, I won't be able to do Mac/ChromeOS before then
> either.
> >
> > It's after late July and none of the rest of us got to this :). Any
> chance
> you
> > can pick this back up?
>
> I'm guessing from the lack of response the answer was "no"... but this is
> still
> relevant :)
>
> https://codereview.chromium.org/2013293002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.