Only allow editable textfields to consume backspace.

Only allow editable textfields to consume backspace.

Do not prevent accelerator handling by read only textfields.

BUG=374006
TEST=Backspace navigates back on when a popup omnibox or cookies dialog textfield is focused.
R=pkasting@chromium.org

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

[msw, 2014-05-16 20:54:26]

Hey Peter, please take a look; thanks!
As I note in the bug, the behavior is debatable.

[Peter Kasting, 2014-05-16 20:59:13]

What's debatable?  I saw your comment in the bug, but you didn't present the
rationale on why this change might not be good.

Technically, LGTM.

[msw, 2014-05-16 21:15:21]

On 2014/05/16 20:59:13, Peter Kasting wrote:
> What's debatable?  I saw your comment in the bug, but you didn't present the
> rationale on why this change might not be good.
> 
> Technically, LGTM.

I guess it might feel odd to navigate back if you think that by focusing a
textfield (regardless of its editable/read-only state) backspace should apply
only to the textfield... Maybe in the same way that having an empty omnibox and
hitting backspace just does nothing, it doesn't navigate back (unchanged here).
Thoughts?

[Peter Kasting, 2014-05-16 21:39:29]

On 2014/05/16 21:15:21, msw wrote:
> On 2014/05/16 20:59:13, Peter Kasting wrote:
> > What's debatable?  I saw your comment in the bug, but you didn't present the
> > rationale on why this change might not be good.
> > 
> > Technically, LGTM.
> 
> I guess it might feel odd to navigate back if you think that by focusing a
> textfield (regardless of its editable/read-only state) backspace should apply
> only to the textfield... Maybe in the same way that having an empty omnibox
and
> hitting backspace just does nothing, it doesn't navigate back (unchanged
here).
> Thoughts?

Maybe the issue here is that the collected cookies dialog you refer to uses
textfields to display information when it should use something else?  I notice
that when you open the cookie dialog in the Chrome content settings,
highlighting text in a specific cookie and then hitting backspace navigates
back.  This is just text on the webpage, which users can select but not edit.

Maybe we should make labels in Chrome selectable, so you can copy the text in
them, and then switch from readonly textfields for this case to labels.

Or maybe we don't want to make labels selectable in general.  In that case, I
might suggest tweaking the behavior of readonly textfields to allow you to
select text, but not allow you to "place the cursor" and have a blinking cursor
within the textfield.  I think the biggest weirdness here is when you have a
blinking cursor and hit backspace, and instead of doing nothing, that would
navigate back.  If you just have a selection in a readonly textfield, and you
hit backspace, it somehow seems more reasonable that backspace is treated as an
accelerator.

The only problem with this suggestion is that it might decrease keyboard
accessibility of these readonly textfields.  I suppose one could still treat the
cursor as if it logically exists, and simply leave it invisible.  So clicking
and then using shift-arrows could still change the selection.

I tested (non-textfield) web content regarding this last idea, and it's a bit
inconsistent.  I filed http://crbug.com/374401 about this inconsistency.  If we
fixed this, this might be a compelling way for readonly textfields to behave. 
Note that in Firefox, the "invisible cursor" cannot be moved with the arrow keys
-- you can only adjust the selection, not shrink to nothing, move the cursor,
and then start a selection from a new location.

[msw, 2014-05-16 22:51:30]

On 2014/05/16 21:39:29, Peter Kasting wrote:
> On 2014/05/16 21:15:21, msw wrote:
> > On 2014/05/16 20:59:13, Peter Kasting wrote:
> > > What's debatable?  I saw your comment in the bug, but you didn't present
the
> > > rationale on why this change might not be good.
> > > 
> > > Technically, LGTM.
> > 
> > I guess it might feel odd to navigate back if you think that by focusing a
> > textfield (regardless of its editable/read-only state) backspace should
apply
> > only to the textfield... Maybe in the same way that having an empty omnibox
> and
> > hitting backspace just does nothing, it doesn't navigate back (unchanged
> here).
> > Thoughts?
> 
> Maybe the issue here is that the collected cookies dialog you refer to uses
> textfields to display information when it should use something else?  I notice
> that when you open the cookie dialog in the Chrome content settings,
> highlighting text in a specific cookie and then hitting backspace navigates
> back.  This is just text on the webpage, which users can select but not edit.
> 
> Maybe we should make labels in Chrome selectable, so you can copy the text in
> them, and then switch from readonly textfields for this case to labels.
> 
> Or maybe we don't want to make labels selectable in general.  In that case, I
> might suggest tweaking the behavior of readonly textfields to allow you to
> select text, but not allow you to "place the cursor" and have a blinking
cursor
> within the textfield.  I think the biggest weirdness here is when you have a
> blinking cursor and hit backspace, and instead of doing nothing, that would
> navigate back.  If you just have a selection in a readonly textfield, and you
> hit backspace, it somehow seems more reasonable that backspace is treated as
an
> accelerator.
> 
> The only problem with this suggestion is that it might decrease keyboard
> accessibility of these readonly textfields.  I suppose one could still treat
the
> cursor as if it logically exists, and simply leave it invisible.  So clicking
> and then using shift-arrows could still change the selection.
> 
> I tested (non-textfield) web content regarding this last idea, and it's a bit
> inconsistent.  I filed http://crbug.com/374401 about this inconsistency.  If
we
> fixed this, this might be a compelling way for readonly textfields to behave. 
> Note that in Firefox, the "invisible cursor" cannot be moved with the arrow
keys
> -- you can only adjust the selection, not shrink to nothing, move the cursor,
> and then start a selection from a new location.

Interesting, I wonder what others will think about the bug you filed. I do plan
on making Views::Labels optionally selectable when I re-implement Label using
persistent RenderText instances. Anyway, letting backspace navigate on read-only
textfields seems okay, at least for now, and it's easy to revert if we change
our minds.

[msw, 2014-05-16 22:51:33]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-05-16 23:33:47]

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

[commit-bot: I haz the power, 2014-05-17 11:42:08]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-17 17:03:22]

Change committed as 271232