Don't make :hover sticky on tap on sites that set a mobile viewport set

Don't make :hover sticky on tap on sites that set a mobile viewport set

This adds a new flag to hittest request type and forces hover to be cleared on Tap on pages considered optimized for mobile.

BUG=370155

[USE s.singapati at gmail.com, 2014-11-13 08:11:31]

s.singapati@samsung.com changed reviewers:
+ igsolla@chromium.org, meh@chromium.org, rbyers@chromium.org

[USE s.singapati at gmail.com, 2014-11-13 08:11:31]

:active & :hover style implementation is mixed based on HitTestRequestType
"active/release". Handling Tap case is easy but LongPress requirement makes
different handling for :active & :hover. 
In Current implementation:
- :active style is cleared after Tap / releasing LongPress / ALSO when "Touch
Moves after LongPress". I assume this behavior should be retained
- When :hover is sticky on an element, doing "Touch Down & Move quickly" outside
that element does not clear :hover. (same with patch)

Event dispatch sequence for Tap & LongTap is same for baseline and with patch.

About meta viewport, based on viewport setting in WebSettingsImpl, touch ACK
timeout is enabled/disabled in TouchEventQueue. So, this check is used in this
patch. any other ideas/comments please?

[USE s.singapati at gmail.com, 2014-11-13 10:30:57]

Patchset 2: Retained "Release" flag to not break any other functionality that
uses release flag.

https://codereview.chromium.org/724673003/diff/20001/Source/core/page/EventHa...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/724673003/diff/20001/Source/core/page/EventHa...
Source/core/page/EventHandler.cpp:2667: return hitType | HitTestRequest::Release
| HitTestRequest::Hover;
Retained "Release" flag to not break any other that uses release flag.

[USE s.singapati at gmail.com, 2014-11-13 12:59:49]

Another question is, sequence of events dispatched in current implementation and
with this patch are same.
Tap: "touchstart, touchend, mouseout (if other element was tapped before),
mouseover, mousemove, mousedown, mouseup, click"

LongTap: "touchstart, touchend"

How about mouse events for long tap. Need some clarification from spec/proposal.

[USE s.singapati at gmail.com, 2014-11-14 13:45:19]

s.singapati@samsung.com changed reviewers:
+ tdanderson@chromium.org, zeeshanq@chromium.org

[USE s.singapati at gmail.com, 2014-11-14 13:45:19]

updated reviewers list.

[Rick Byers, 2014-11-17 21:17:10]

> About meta viewport, based on viewport setting in WebSettingsImpl, touch ACK
> timeout is enabled/disabled in TouchEventQueue. So, this check is used in this
> patch. any other ideas/comments please?

It is, where?  The only thing I see you checking is whether the viewport tag is
enabled (which should always be true on Android) - nothing about the state of
the current page.

I think we really need to expose the logic in
WebViewImpl::shouldDisableDesktopWorkarounds to blink core somehow.  One option
is to effectively move that code down into Page (and have WebViewImpl invoke it
there).  Another option is possibly to have blink call back out through
ChromeClient to ask WebViewImpl.

I think we better send a PSA to blink-dev about this policy (it will be
contentious).  I'll do that now and cc you (then come back to providing feedback
on the rest of your patch).

[Rick Byers, 2014-11-17 22:00:09]

rbyers@chromium.org changed reviewers:
- igsolla@chromium.org, meh@chromium.org, tdanderson@chromium.org

[Rick Byers, 2014-11-17 22:00:10]

Sorry for the delay.  This is going to be a non-trivial change to get right, I'm
a little concerned that you might be better off starting with some less risky /
complex changes in this code.

That said, here's my thoughts on the patch.  You also need to update / add tests
(surely this change is causing some things to now fail in
LayoutTests/fast/touch/gesture, right?)

https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:3429: m_hoverNode.clear();
why is this change necessary?  Doesn't it already behave like this?  Eg. try
moving the mouse out of the window - that should call setHoverNode(nullptr)
already today I think.

https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5431: if ((settings() &&
settings()->viewportEnabled())
The logic to decide when to apply this policy belongs in EventHandler, not here
in Document.  The behavior here should be based just on what HitTestRequest
flags are present.  This will be important, for example, when we start caching
HitTestResult objects keyed by their HitTestRequest.

https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5434: for (RenderObject* curr = oldHoverObj; curr;
curr = curr->hoverAncestor()) {
please don't copy/paste code from above.  We should be able to refactor the code
to unify the "clear old hover" logic in one place.

https://codereview.chromium.org/724673003/diff/20001/Source/core/rendering/Hi...
File Source/core/rendering/HitTestRequest.h (right):

https://codereview.chromium.org/724673003/diff/20001/Source/core/rendering/Hi...
Source/core/rendering/HitTestRequest.h:41: Hover = 1 << 11,
It's a little confusing to explicitly opt-in to hover, when Active, Move and
Release already normally imply Hover.

Perhaps instead we should invert this by adding a new 'Leave' flag which (when
applied on conjunction with Release) forces hover to be cleared.

I know the existing code isn't well commented here, but please add a comment
describing the meaning of this new bit since it's rather subtle.

[Rick Byers, 2014-11-17 22:02:43]

On 2014/11/13 12:59:49, s.singapati wrote:
> Another question is, sequence of events dispatched in current implementation
and
> with this patch are same.
> Tap: "touchstart, touchend, mouseout (if other element was tapped before),
> mouseover, mousemove, mousedown, mouseup, click"
> 
> LongTap: "touchstart, touchend"
> 
> How about mouse events for long tap. Need some clarification from
spec/proposal.

I think we should keep that separate - let's not try to change it here.  We
should probably be sending at least mousemove (and so mouseover/mouseenter) on
long press.  But that's likely a pre-existing bug.  In fact, that might be a
much simpler/safer bug for you to start with if you're interested.

[USE s.singapati at gmail.com, 2014-11-20 16:46:54]

Thank you for comments. My comments below.

On 2014/11/17 22:00:10, Rick Byers wrote:
> That said, here's my thoughts on the patch.  You also need to update / add
tests
> (surely this change is causing some things to now fail in
> LayoutTests/fast/touch/gesture, right?)

YES, I see some of gesture layout tests are failed with Patch, also on baseline
(Tested on Android). But, If this patch is what is expected behavior, I will
continue work on updating/adding new test cases.

>
https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
> Source/core/dom/Document.cpp:3429: m_hoverNode.clear();
> why is this change necessary?  Doesn't it already behave like this?  Eg. try
> moving the mouse out of the window - that should call setHoverNode(nullptr)
> already today I think.

DONE: Double checked and this change is not needed.

>
https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
> Source/core/dom/Document.cpp:5431: if ((settings() &&
> settings()->viewportEnabled())
> The logic to decide when to apply this policy belongs in EventHandler, not
here
> in Document.  The behavior here should be based just on what HitTestRequest
> flags are present.  This will be important, for example, when we start caching
> HitTestResult objects keyed by their HitTestRequest.

DONE: Moved this logic to EventHandler.

>
https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
> Source/core/dom/Document.cpp:5434: for (RenderObject* curr = oldHoverObj;
curr;
> curr = curr->hoverAncestor()) {
> please don't copy/paste code from above.  We should be able to refactor the
code
> to unify the "clear old hover" logic in one place.

YES, I also thought of re-factoring this but It looked to take more changes,
slightly makes it complex (for this fix). Change is few lines and probably this
code will be not used from other places. (I agree that this method needs
re-factoring anyways at some point, considering issues like hovering when mouse
dragging, mustBeInActiveChain logic etc)

 
>
https://codereview.chromium.org/724673003/diff/20001/Source/core/rendering/Hi...
> Source/core/rendering/HitTestRequest.h:41: Hover = 1 << 11,
> It's a little confusing to explicitly opt-in to hover, when Active, Move and
> Release already normally imply Hover.
> 
> Perhaps instead we should invert this by adding a new 'Leave' flag which (when
> applied on conjunction with Release) forces hover to be cleared.

DONE: made it bit more clear with "leave" flag.

If this patch is close to what is expected behavior (probably with any other
minor things to fix in this patch & layout tests), may be we can continue work
and fix this issues. Otherwise, If we want to wait for SPEC for "Touch+Hover" to
be prepared, we can comeback to this patch later.

[USE s.singapati at gmail.com, 2014-11-20 20:13:31]

On 2014/11/17 22:02:43, Rick Byers wrote:
> On 2014/11/13 12:59:49, s.singapati wrote:
> > Another question is, sequence of events dispatched in current implementation
> and
> > with this patch are same.
> > Tap: "touchstart, touchend, mouseout (if other element was tapped before),
> > mouseover, mousemove, mousedown, mouseup, click"
> > 
> > LongTap: "touchstart, touchend"
> > 
> > How about mouse events for long tap. Need some clarification from
> spec/proposal.
> 
> I think we should keep that separate - let's not try to change it here.  We
> should probably be sending at least mousemove (and so mouseover/mouseenter) on
> long press.  But that's likely a pre-existing bug.  In fact, that might be a
> much simpler/safer bug for you to start with if you're interested.

Yes, dispatching mousemove, mouseover/mouseenter on LongPress would be good
solution to fix problems with hover menus. I will create a new issue.

[Rick Byers, 2014-11-21 15:08:39]

On 2014/11/20 16:46:54, s.singapati wrote:
> Thank you for comments. My comments below.
> 
> On 2014/11/17 22:00:10, Rick Byers wrote:
> > That said, here's my thoughts on the patch.  You also need to update / add
> tests
> > (surely this change is causing some things to now fail in
> > LayoutTests/fast/touch/gesture, right?)
> 
> YES, I see some of gesture layout tests are failed with Patch, also on
baseline
> (Tested on Android). But, If this patch is what is expected behavior, I will
> continue work on updating/adding new test cases.
> 
> >
>
https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
> > Source/core/dom/Document.cpp:3429: m_hoverNode.clear();
> > why is this change necessary?  Doesn't it already behave like this?  Eg. try
> > moving the mouse out of the window - that should call setHoverNode(nullptr)
> > already today I think.
> 
> DONE: Double checked and this change is not needed.
> 
> >
>
https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
> > Source/core/dom/Document.cpp:5431: if ((settings() &&
> > settings()->viewportEnabled())
> > The logic to decide when to apply this policy belongs in EventHandler, not
> here
> > in Document.  The behavior here should be based just on what HitTestRequest
> > flags are present.  This will be important, for example, when we start
caching
> > HitTestResult objects keyed by their HitTestRequest.
> 
> DONE: Moved this logic to EventHandler.
> 
> >
>
https://codereview.chromium.org/724673003/diff/20001/Source/core/dom/Document...
> > Source/core/dom/Document.cpp:5434: for (RenderObject* curr = oldHoverObj;
> curr;
> > curr = curr->hoverAncestor()) {
> > please don't copy/paste code from above.  We should be able to refactor the
> code
> > to unify the "clear old hover" logic in one place.
> 
> YES, I also thought of re-factoring this but It looked to take more changes,
> slightly makes it complex (for this fix). Change is few lines and probably
this
> code will be not used from other places. (I agree that this method needs
> re-factoring anyways at some point, considering issues like hovering when
mouse
> dragging, mustBeInActiveChain logic etc)
> 
>  
> >
>
https://codereview.chromium.org/724673003/diff/20001/Source/core/rendering/Hi...
> > Source/core/rendering/HitTestRequest.h:41: Hover = 1 << 11,
> > It's a little confusing to explicitly opt-in to hover, when Active, Move and
> > Release already normally imply Hover.
> > 
> > Perhaps instead we should invert this by adding a new 'Leave' flag which
(when
> > applied on conjunction with Release) forces hover to be cleared.
> 
> DONE: made it bit more clear with "leave" flag.
> 
> If this patch is close to what is expected behavior (probably with any other
> minor things to fix in this patch & layout tests), may be we can continue work
> and fix this issues. Otherwise, If we want to wait for SPEC for "Touch+Hover"
to
> be prepared, we can comeback to this patch later.

Yeah, I was content to just make some low-cost small tweaks now, but from the
blink-dev thread
(https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DllLcNXHfHw)
it sounds like the IE team is pretty motivated to try to get the behavior
spec'd.  If that's going to happen in the near/medium future, then I'd prefer to
hold off on making any changes here so that we only need to tell web developers
about changes once.  I'm sorry that I'm waffling here.

> Yes, dispatching mousemove, mouseover/mouseenter on LongPress would be good
> solution to fix problems with hover menus. I will create a new issue.

Thanks, let's discuss the options on the bug once you've filed it.  Eg. perhaps
we should be sending a mousemove event on GestureShowPress (which is the same
time we set the :active/:hover states), instead of sending one for GestureTap. 
That would be nice and consistent.  However there's possible web-compat impact
(eg. code that expects to see mousemove/mouseenter/etc. on tap after the
touchend instead of before).  So again, we should probably discuss the options
with the other vendors.  If there's something low-risk we can do now, then
great.  Otherwise we may want to lump this in with the spec work too.

[USE s.singapati at gmail.com, 2014-11-24 17:53:44]

> > Yes, dispatching mousemove, mouseover/mouseenter on LongPress would be good
> > solution to fix problems with hover menus. I will create a new issue.
> 
> Thanks, let's discuss the options on the bug once you've filed it.  Eg.
perhaps
> we should be sending a mousemove event on GestureShowPress (which is the same
> time we set the :active/:hover states), instead of sending one for GestureTap.

> That would be nice and consistent.  However there's possible web-compat impact
> (eg. code that expects to see mousemove/mouseenter/etc. on tap after the
> touchend instead of before).  So again, we should probably discuss the options
> with the other vendors.  If there's something low-risk we can do now, then
> great.  Otherwise we may want to lump this in with the spec work too.

New issue here to discuss:
http://code.google.com/p/chromium/issues/detail?id=436089

[USE s.singapati at gmail.com, 2015-03-18 09:24:37]

Rick Byers: This issue has been there for a while. Could we take some action?
closing it for now or what do you suggest?
Thank you.