Bug 49057 – Move whatever make sense from "touch adjuster" implementation to WebCore

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Antonio Gomes]

As suggested in bug 44089 (comment https://bugs.webkit.org/show_bug.cgi?id=44089#c25) we should consider moving part of touch adjuster (added by bug 44089) code to WebCore so it could be shared between webkit and webkit2 (at least for Qt).

(In reply to comment #25)
> isClickableElement and indCandidatePointForTouch seems like good candidates... not indCandidatePointForTouch - that method can easily live in WebKit and be duplicated

[Andre Pedralho]

Would it be ok for you guys if I add the TouchAdjuster to WebCore/rendering/HitTestResult.{h/cpp}?

[Antonio Gomes]

(In reply to comment #1)
> Would it be ok for you guys if I add the TouchAdjuster to WebCore/rendering/HitTestResult.{h/cpp}?

I do not think this code should go to HitTestResult. Instead it would be a class making use of it.

Maybe webcore/page/EventHandler or its own class, but I am also not sure.

There is also something to be done first I think: properly support touch events.

[Antonio Gomes]

The man issue I was discussing with Andre here is: If TouchAdjuster goes to WebCore, what would be the call sites?

1) Should we still call it from webkit before actually submitting the event down to WebCore?
2) Should we submit the event to webcore unchanged, and call TouchAdjuster from EventHandler?
3) If it is to be called from EventHandler, where should it get the paddings from? The webplugin thing is pretty qt-specfic. There should be a way to make platforms other than Qt make use of it.

Lets discuss the whole thing here before any coding... :)

[Benjamin Poulain]

I am looking at the issue to get

[Andreas Kling]

(In reply to comment #4)
> I am looking at the issue to get

You should probably look at this thing first:

[Benjamin Poulain]

Sorry for the previous comment, I hit enter by accident.

I am looking at the issue to get touch handling in WebKit 2. Since I'd prefer not to copy code I am moving that to EventHandler.

My current idea is:

1) if the touch event reports a rect:
-if javascript does not use the event
 -use TouchAdjuster to find the best click position
 -send a fake mouse event to that position
-ignore the associated mouse event

2) if the touch event does not report a rect:
 -create a fake rect for the touch event in the graphicswebview and send the touch event

3) if the platform is dumb and does not report touch event
 -create fake touch event in graphicswebview based on the platform plugin and back to (1).

Why do I want to deal with the rect in graphicswebview? To avoid the platform plugin in EventHandler, and take into acount the item transformation to adjust the rects of the touch points.

Any input welcome.

[Andre Pedralho]

My initial idea was just to move the TouchAdjuster to the WebCore/page/MouseEventWithHitTestResults as a kind of helper class and just use it if we have padding values set somewhere.

bool EventHandler::handleMousePressEvent(const PlatformMouseEvent& mouseEvent)

1) it creates a MouseEventWithHitTestResult:
MouseEventWithHitTestResults mev = m_frame->document()->prepareMouseEvent(request, documentPoint, mouseEvent);

2) the event is handled afterwards:
swallowEvent = handleMousePressEvent(mev);

My idea is to create the MouseEventWithHitTestResults in 1 with the TouchAdjuster already applied.

My doubt: from where to get the padding values?

[Antonio Gomes]

I generally think it is a good idea. Some comments:

> My current idea is:
> 
> 1) if the touch event reports a rect:

Some devices like the used to report the a rect, but the wrong rect. Using can be dangerous in cases like this, but it can not be prevented maybe :(

> -if javascript does not use the event
>  -use TouchAdjuster to find the best click position
>  -send a fake mouse event to that position
> -ignore the associated mouse event

This sounds reasonable. But remember we sould just do that if it is a single touch event.

> 2) if the touch event does not report a rect:
>  -create a fake rect for the touch event in the graphicswebview and send the touch event

That would use the platform plugin values, right?

> 3) if the platform is dumb and does not report touch event
>  -create fake touch event in graphicswebview based on the platform plugin and back to (1).

Hum ... /me thinking about this. Remember: mouse events are not the same as touch event, and this could break web compatibility in many sites.

[Kenneth Rohde Christiansen]

Benjamin how does this align with your webkit2 work?

[Benjamin Poulain]

(In reply to comment #9)
> Benjamin how does this align with your webkit2 work?

Currently I move everything in EventHandler. Antonio wants the touch adjustment to be port specific. At first I thought about using ChromeClient but that is a bad idea of WebKit 2 so I will have to introduce something to abstract that by platform.

I am now uber convince the rect should be part of the touch event.

My plan is to wait until I have a solution for gesture, and upstream what I did part by part.

[Luiz Agostini]

Is this bug really P1? Should it block QtWebKit 2.2?

[Kenneth Rohde Christiansen]

This needs input from Benjamin. A lot of the touch work changed a lot, so maybe this is not needed at all anymore. On the other hand it might be good with good touch support in qtwebkit 2.2 given that maybe probably will use it on touch enabled devices

[Benjamin Poulain]

To summarize my point of view :)
I think having complete touch support is a huge priority, given how many touch devices are using QtWebKit.

I would like to use as much as I can from the WebKit 2 implementation in WebKit 1. At first I was hoping to use the Chrome implementation but I am not too confident that would work for mobile.

But it is way too late for 2.2 imho. I hope to do it after the release of the next device we are working on.

So maybe this particular task is not a P1, but we should have a P1 about having proper touch support instead of a half-baked feature as we have now.

[Simon Hausmann]

Allan is working actively on this now as part of bug #78801

*** This bug has been marked as a duplicate of bug 78801 ***