Disable gesture tap highlight on editable nodes

Disable gesture tap highlight on editable nodes

This patch fixes the gesture tap not to highlight editable nodes even
when they have "cursor: pointer" in their ancestor chains.

It was unlikely to occur until bug 399522 changed the tap highlighting
to use the composed tree traversal. With that fixed, input elements
with "cursor: pointer" in their ancestor nodes can hit this issue.

BUG=457552
TEST=compositing/gestures/gesture-tapHighlight-form-input-text.html

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

[kojii, 2015-02-17 09:21:58]

kojii@chromium.org changed reviewers:
+ hayato@chromium.org

[kojii, 2015-02-17 09:21:58]

hayato@, PTAL.

[hayato, 2015-02-17 10:34:56]

https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1279: // Editable nodes should not be highlighted
(e.g., <input>)
Not sure this is always true.
I'm afraid that this would disable highlights in other cases with false
positive.

I think you might want to add other reviewers who are familiar with how
highlight should be.

[kojii, 2015-02-17 13:08:01]

kojii@chromium.org changed reviewers:
+ esprehn@chromium.org, rbyers@chromium.org

[kojii, 2015-02-17 13:08:02]

esprehn@, rbyers@, could you PTAL?

https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1279: // Editable nodes should not be highlighted
(e.g., <input>)
On 2015/02/17 10:34:56, hayato wrote:
> Not sure this is always true.
> I'm afraid that this would disable highlights in other cases with false
> positive.
> 
> I think you might want to add other reviewers who are familiar with how
> highlight should be.

Highlights are for links, so I suppose it's quite unlikely to wrap editable
content with <a>.

I wondered the opposite case, where we may also want to disable other UA-shadow
controls, but thought focusing on reported case would be better.

I'll look around similar bugs and ask more reviewers.

[Rick Byers, 2015-02-20 20:37:14]

mustaq@ owns this highlighting heuristic, but we know it's not perfect.  I'm OK
with this tweak if mustaq is.

https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1272: // FIXME: This wants to walk composed tree with
NodeRenderingTraversal::parent().
nit: mind removing this FIXME while you're here?  It was fixed but somehow the
comment didn't get removed.

https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1279: // Editable nodes should not be highlighted
(e.g., <input>)
On 2015/02/17 13:08:02, koji wrote:
> On 2015/02/17 10:34:56, hayato wrote:
> > Not sure this is always true.
> > I'm afraid that this would disable highlights in other cases with false
> > positive.
> > 
> > I think you might want to add other reviewers who are familiar with how
> > highlight should be.
> 
> Highlights are for links, so I suppose it's quite unlikely to wrap editable
> content with <a>.
> 
> I wondered the opposite case, where we may also want to disable other
UA-shadow
> controls, but thought focusing on reported case would be better.
> 
> I'll look around similar bugs and ask more reviewers.

Highlights are for more than "links", eg. a <div> that's logically a button
should get highlighted.  But this is a heuristic, and I suspect this is a
net-improvement (although the google image search case does feel kind of like a
bug in the page - the pointer cursor is being used even when clicking will do
nothing instead of the typical affordance for something the sites is indicating
is clickable).

[mustaq, 2015-02-20 22:20:39]

On 2015/02/20 20:37:14, Rick Byers wrote:
> mustaq@ owns this highlighting heuristic, but we know it's not perfect.  I'm
OK
> with this tweak if mustaq is.
> 
> https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp#n...
> Source/web/WebViewImpl.cpp:1272: // FIXME: This wants to walk composed tree
with
> NodeRenderingTraversal::parent().
> nit: mind removing this FIXME while you're here?  It was fixed but somehow the
> comment didn't get removed.
> 
>
https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp#n...
> Source/web/WebViewImpl.cpp:1279: // Editable nodes should not be highlighted
> (e.g., <input>)
> On 2015/02/17 13:08:02, koji wrote:
> > On 2015/02/17 10:34:56, hayato wrote:
> > > Not sure this is always true.
> > > I'm afraid that this would disable highlights in other cases with false
> > > positive.
> > > 
> > > I think you might want to add other reviewers who are familiar with how
> > > highlight should be.
> > 
> > Highlights are for links, so I suppose it's quite unlikely to wrap editable
> > content with <a>.
> > 
> > I wondered the opposite case, where we may also want to disable other
> UA-shadow
> > controls, but thought focusing on reported case would be better.
> > 
> > I'll look around similar bugs and ask more reviewers.
> 
> Highlights are for more than "links", eg. a <div> that's logically a button
> should get highlighted.  But this is a heuristic, and I suspect this is a
> net-improvement (although the google image search case does feel kind of like
a
> bug in the page - the pointer cursor is being used even when clicking will do
> nothing instead of the typical affordance for something the sites is
indicating
> is clickable).

I agree, this is an improvement. So LGTM.

[kojii, 2015-02-23 01:32:18]

New patchsets have been uploaded after l-g-t-m from mustaq@chromium.org

[kojii, 2015-02-23 03:44:18]

https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/931753002/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1272: // FIXME: This wants to walk composed tree with
NodeRenderingTraversal::parent().
On 2015/02/20 20:37:14, Rick Byers (Out until 3-3) wrote:
> nit: mind removing this FIXME while you're here?  It was fixed but somehow the
> comment didn't get removed.

Done.

[kojii, 2015-02-23 04:36:04]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-02-23 04:36:19]

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

[commit-bot: I haz the power, 2015-02-23 04:36:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-23 04:36:21]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[kojii, 2015-02-23 04:43:32]

kojii@chromium.org changed reviewers:
+ tkent@chromium.org

[kojii, 2015-02-23 04:43:33]

tkent@, OWNER PTAL?

[hayato, 2015-02-23 04:46:38]

lgtm2
This still needs lgtm from Source/OWNERs.

[tkent, 2015-02-23 04:51:08]

lgtm

[kojii, 2015-02-23 04:51:40]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-02-23 04:52:29]

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

[commit-bot: I haz the power, 2015-02-23 04:55:56]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190635