Do not reset mouse focus flag on keydown at anchor elements

Do not reset mouse focus flag on keydown at anchor elements

BUG=412251
R=pdr@chromium.org
TEST=./Tools/Scripts/run-webkit-tests fast/events/click-*

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181844

[pdr., 2014-09-10 18:28:45]

On 2014/09/10 13:25:59, robwu wrote:

This looks like a nice cleanup to me but won't there be a small change in
behavior? If so, we should add a test.

[robwu, 2014-09-10 21:58:54]

I did not add a test because the feature cannot accidentally be added back.
In the initial version, I included the show-focus-ring-on-keydown behavior
without much thought, but in retrospect, not doing that (i.e. not resetting
the wasFocusedByMouse flag on keydown) makes more sense.

Do you still want a LayoutTest that tests whether the ring is not shown on
keydown?
On Sep 10, 2014 8:28 PM, <pdr@chromium.org> wrote:

> On 2014/09/10 13:25:59, robwu wrote:
>
> This looks like a nice cleanup to me but won't there be a small change in
> behavior? If so, we should add a test.
>
> https://codereview.chromium.org/562653002/
>

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

[pdr., 2014-09-11 04:15:49]

On 2014/09/10 21:58:54, robwu wrote:
> I did not add a test because the feature cannot accidentally be added back.
> In the initial version, I included the show-focus-ring-on-keydown behavior
> without much thought, but in retrospect, not doing that (i.e. not resetting
> the wasFocusedByMouse flag on keydown) makes more sense.
> 
> Do you still want a LayoutTest that tests whether the ring is not shown on
> keydown?

Please do add a test (I didn't realize the behavior change in the last patch). I
don't mean for this to be busy work, it'll just ensure that the next person in
this code has more to go on than we did.

[robwu, 2014-09-11 10:25:42]

On 2014/09/11 04:15:49, pdr wrote:
> On 2014/09/10 21:58:54, robwu wrote:
> > I did not add a test because the feature cannot accidentally be added back.
> > In the initial version, I included the show-focus-ring-on-keydown behavior
> > without much thought, but in retrospect, not doing that (i.e. not resetting
> > the wasFocusedByMouse flag on keydown) makes more sense.
> > 
> > Do you still want a LayoutTest that tests whether the ring is not shown on
> > keydown?
> 
> Please do add a test (I didn't realize the behavior change in the last patch).
I
> don't mean for this to be busy work, it'll just ensure that the next person in
> this code has more to go on than we did.

Added test. Locally, I've verified that the new test fails before this patch,
and passes after applying the patch.

[pdr., 2014-09-11 17:46:26]

On 2014/09/11 10:25:42, robwu wrote:
> On 2014/09/11 04:15:49, pdr wrote:
> > On 2014/09/10 21:58:54, robwu wrote:
> > > I did not add a test because the feature cannot accidentally be added
back.
> > > In the initial version, I included the show-focus-ring-on-keydown behavior
> > > without much thought, but in retrospect, not doing that (i.e. not
resetting
> > > the wasFocusedByMouse flag on keydown) makes more sense.
> > > 
> > > Do you still want a LayoutTest that tests whether the ring is not shown on
> > > keydown?
> > 
> > Please do add a test (I didn't realize the behavior change in the last
patch).
> I
> > don't mean for this to be busy work, it'll just ensure that the next person
in
> > this code has more to go on than we did.
> 
> Added test. Locally, I've verified that the new test fails before this patch,
> and passes after applying the patch.

Thank you. LGTM

[pdr., 2014-09-11 17:46:29]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:47:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/562653002/20001

[commit-bot: I haz the power, 2014-09-11 19:53:30]

Committed patchset #2 (id:20001) as 181844