touchstart event defined null instead of undefined when touch is not available

• Component: UI Events
• Status: RESOLVED WONTFIX

[Titta Heikkala]

Overview:
When opening an xhtml file with a chart one should be able to select an area from the chart. Following page http://www.highcharts.com/demo/line-time-series has a zoomable time series chart where the selected area should be zoomed in. With QtWebKit (in Qt 4.7.2) the mouse release is not recognized thus selecting an area is not possible.

Steps to Reproduce:
With the following (simple) code the issue can be reproduced:
int main(int argc, char *argv[])
{
    QApplication a(argc, argv);
    QWebView w;
    w.load(QUrl("http://www.highcharts.com/demo/line-time-series"));
    w.show();
}

Actual Results:
The selection for zooming area works ok, but the mouse release is not working hence the zoom in is not happening.

Additional Information:
With Qt 4.6.2 it was possible to select an area for zooming in.

[Benjamin Poulain]

Please follow this when reporting bugs for QtWebKit: http://trac.webkit.org/wiki/QtWebKitBugs

[Benjamin Poulain]

I can reproduce on trunk with Linux.

[Janne Koskinen]

*** Bug 68236 has been marked as a duplicate of this bug. ***

[Jani Honkonen]

Created attachment 116212 [details]
Patch

[Adam Barth]

Comment on attachment 116212 [details]
Patch

Test?  (Also, your ChangeLog diff looks corrupt.)

[Adam Barth]

I'm not sure whether this change is correct.  Can you test the behavior in other browsers to make sure we're improving interoperability with this change?

[Adam Barth]

This bug affects more than just Qt.

[Jani Honkonen]

The exact problem with the highcharts is this check:

hasTouch = doc.documentElement.ontouchstart !== undefined

When touch is not available this currently returns incorrectly true because ontouchstart is null instead of undefined. 

At least on chorme and firefox this is working properly plus the draft specification for TouchEvents defines it to be undefined. http://www.w3.org/TR/2011/WD-touch-events-20111027/

As for the fix proposal the CodeGeneratorJS.pm is the place where this is conrolled. I'm aware that this fix will affect more than just the touchstart but I don't see any way to control the return value of a single eventlistener.

The changelog is corrupt probably because I do not know how to use vim :) Also I'm having hard time running webkit scripts in cygwin. I'm not able to run layout tests using the scripts but I can run them individually. So some help in getting a proper fix for this would be nice :)

[Janne Koskinen]

(In reply to comment #7)
> This bug affects more than just Qt.

Correct, the bug is in webkit if it is a bug i.e. how should the specification read when it says default action is undefined.

Why it only shows up in QtWebkit is that QtWebkit defaults to TOUCH_ENABLED.

[Jani Honkonen]

Created attachment 116914 [details]
Patch

[Jani Honkonen]

(In reply to comment #10)
> Created an attachment (id=116914) [details]
> Patch

Added a new patch. Hopefully better than the previous.

Switched to Ubuntu where I can run all the layout tests succesfully. Did not find anything suscpicious with this patch.

[WebKit Review Bot]

Comment on attachment 116914 [details]
Patch

Attachment 116914 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10683107

New failing tests:
fast/events/touch/basic-single-touch-events.html

[Simon Hausmann]

Comment on attachment 116914 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116914&action=review

I'm saying r- because of the missing v8 part. I'm not 100% sure neither if this is generally the correct change. It's probably going to have to be somebody else to give an r+ there.

Once thing I would strongly suggest is to _not_ test this behaviour embedded in the touch test but instead create a dedicated layout test, as this isn't really specific to the touch event handlers, right?

I admit I'm very surprised that no other existing layout test is covering this particular aspect of the DOM bindings.

> Source/WebCore/ChangeLog:3
> +        Mouse button release not recognized with QWebView

I suggest to change the title away from making it sound Qt specific towards something that indicates that this is about changing WebKit's behavior with regards to event listeners.

> Source/WebCore/ChangeLog:13
> +        * bindings/scripts/CodeGeneratorJS.pm:
> +        (GenerateImplementation): Changed EventListener return value from jsNull() to jsUndefined().

You're going to have to do a similar change in the V8 generator to fix the Chromium test failure (Chromium is using V9).

[Janne Koskinen]

Changed title to reflect the bug.

[Adam Barth]

I'm not sure this approach is correct.  This is not something that should be defined by the Touch Events spec.  This is something that needs to be defined by DOM Core.

[Jani Honkonen]

Created attachment 117158 [details]
Patch

[Jani Honkonen]

(In reply to comment #16)
> Created an attachment (id=117158) [details]
> Patch

I was trying to fix the chromium test failure with this latest patch but it seems the ews has failed to apply the patch with a HTTP Error 500. Do I need to make a new patch or can someone make ews do a re-run?

[Simon Hausmann]

(In reply to comment #17)
> (In reply to comment #16)
> > Created an attachment (id=117158) [details] [details]
> > Patch
> 
> I was trying to fix the chromium test failure with this latest patch but it seems the ews has failed to apply the patch with a HTTP Error 500. Do I need to make a new patch or can someone make ews do a re-run?

I just checked in #webkit and yeah, seems re-uploading is the easiest way :)

[Jani Honkonen]

Created attachment 117374 [details]
Patch

[Adam Barth]

Comment on attachment 117374 [details]
Patch

You haven't responded to my earlier comments.

[Jani Honkonen]

(In reply to comment #20)
> (From update of attachment 117374 [details])
> You haven't responded to my earlier comments.

I don't know what else to add to my comment #8. I do not have enough competence in this area to suggests another approach for fixing this.

My intrest in this is from Qt Commercial perspective. We have a workaround fix for this in our (upcoming) release but naturally it would be better if this would be fixed properly.

[Benjamin Poulain]

The fix is not correct.

For Qt, the touch handler are defined even if your platform does not have or support touch events. So the event handler is effectively defined, and the value should be null, not undefined.

I suspect this change could break many feature detection scripts (try Modernizr). I suggest you to recompile QtWebKit without ENABLE_TOUCH_EVENTS.

[Jani Honkonen]

(In reply to comment #22)
> The fix is not correct.
> 
> For Qt, the touch handler are defined even if your platform does not have or support touch events. So the event handler is effectively defined, and the value should be null, not undefined.
> 
> I suspect this change could break many feature detection scripts (try Modernizr). I suggest you to recompile QtWebKit without ENABLE_TOUCH_EVENTS.

Yes, for Qt disabling the ENABLE_TOUCH_EVENTS flag will fix this issue but we don't want to do that because we still want to support touch events. From my point of view it's just another workaround.

Could you explain in more detail why do you think the value should be null instead of undefined?

[Benjamin Poulain]

> Could you explain in more detail why do you think the value should be null instead of undefined?

From the spec....
"An event handler can either have the value null or be set to a Function object. Initially, event handlers must be set to null."

[Jani Honkonen]

(In reply to comment #24)
> > Could you explain in more detail why do you think the value should be null instead of undefined?
> 
> From the spec....
> "An event handler can either have the value null or be set to a Function object. Initially, event handlers must be set to null."

This is the spec you are referring to?
http://www.w3.org/TR/html5/webappapis.html#events

In this light seems quite clear now that the fault is in highcharts not webkit.

Although the touch event spec seems to contradict this but I choose to believe html5 spec :)

[Benjamin Poulain]

 > In this light seems quite clear now that the fault is in highcharts not webkit.

That's a point of view I guess.