Bug 44088

Created attachment 64548 [details] patch v1.

Comment on attachment 64548 [details] patch v1. WebKit/qt/Api/qwebsettings.cpp:1108 + specially useful for touch devices, once heuristics can be used internally by I think it is called especially. WebKit/qt/Api/qwebsettings.cpp:1103 + Specifies the fuzzy range in pixels for accepting mouse events for a given what is the page is scaled? WebKit/qt/Api/qwebsettings.cpp:1132 + void QWebSettings::setHitTestPadding(Qt::Orientation orientation, int padding) how can a hit test have padding? setPaddingForHitTesting?

> WebKit/qt/Api/qwebsettings.cpp:1103 > + Specifies the fuzzy range in pixels for accepting mouse events for a given > what is the page is scaled? The padding value itself does not need to care about scaling: if patch if zoomed in, for example, you finger (i.e. the tapping area) would not change. > WebKit/qt/Api/qwebsettings.cpp:1132 > + void QWebSettings::setHitTestPadding(Qt::Orientation orientation, int padding) > how can a hit test have padding? setPaddingForHitTesting? I like setPaddingForHitTesting, but maybe the term 'padding' is non intuitive here, anyways. Maybe something more descriptive like setFuzzyRangeForAcceptingEvents or setExpansionAreaForClicking (just thinking lound here)?

> > what is the page is scaled? > > The padding value itself does not need to care about scaling: if patch if zoomed in, for example, you finger (i.e. the tapping area) would not change. err, in English now: The padding value itself does not need to care about scaling: if page is zoomed in, for example, your finger (i.e. the tapping area) would not change.

(In reply to comment #2) > (From update of attachment 64548 [details]) > WebKit/qt/Api/qwebsettings.cpp:1108 > + specially useful for touch devices, once heuristics can be used internally by > I think it is called especially. accordinging to http://englishplus.com/grammar/00000287.htm 'especially' is also apropriated here. Do you agree?

Simon you are the API guy :-) We need your input!

I suggest to have padding top/left/right/bottom instead of same padding everywhere. The reason is that the rect touching the screen is not exactly the rect the user expect to interact with. When you click on something, you look at your finger from the top, and expect to click on the area under your nail. If you look sideway on a finger pressing a button, the area touching the screen if offset down from the nail position. For this reason, the rect created from a touch point need to be offset to the top by a few pixels. The exact offset depends on the screen technology. If you don't understand what I am talking about, I suggest you to write a app where you have to click on points and display the rect your pressed. You'll see the rect is down from where you expected to click.

Comment on attachment 64548 [details] patch v1. WebKit/qt/Api/qwebsettings.h:142 + void setHitTestPadding(Qt::Orientation, int padding); Hmm, this is a member function of QWebSettings, but it actually doesn't change any members but reads/writes directly to the global settings. I think it should either be a static function or the setting should propagate correct from global settings to page specific settings.

Antonio, can you explain where in a Qt/Meegotouch application these padding values would come from? I like how this keeps things actually simple, it doesn't require changing all the APIs. I'm just curious where the values would come from and if they come from a system library like libmeegotouch, can we perhaps provide defaults? What about pure touch events? Should WebCore/page/EventHandler.cpp: EventHandler::handleTouchEvent() use your new internal functions for better accuracy?

Hi Benjamin. (In reply to comment #7) > I suggest to have padding top/left/right/bottom instead of same padding everywhere. > > When you click on something, you look at your finger from the top, and expect to click on the area under your nail. If you look sideway on a finger pressing a button, the area touching the screen if offset down from the nail position. > > For this reason, the rect created from a touch point need to be offset to the top by a few pixels. The exact offset depends on the screen technology. Yes, I totally understand your suggestion. In fact, the rect-based hit testing idea was inspired of Mozilla's SmartTap, which on its turns, exposes a more flexible API, just like you said: nsIDOMNodeList nodesFromRect(in long aX, in long aY, in long aTopSize, in long aRightSize, in long aBottomSize, in long aLeftSize, in boolean aIgnoreRootScrollFrame, in boolean aFlushLayout); Mark Finkle, from the mozilla mobile team, explains that nicely in a blog post: http://starkravingfinkle.org/blog/2010/05/smart-tapping-in-mobile-firefox/ , including why their values also benefit the TOP direction. "Firefox Mobile uses a region that is offset more above the touch point. This has the affect of favoring elements above the touch point – which is based on our observations that people tend to “tap” below elements." Making that change, would be very logic to me, yes, but would need changes in the underlying WebCore HitTestResult and Hit Testing system implementations.

@Grace: We are trying to define a nice API for setting the padding values for QtWebKit. But it turns out that horizontal and vertical paddings *might* not be the best way to specify the paddings. We might end up needing separeted "up, right, down and left" paddings properties (comment #10). Could you please explain how android is making use of the rect-based hit testing we added? Is that through a public API or hardcoded values? Also, would changing from the rect-based hit testing padding from "horizontal + vertical paddings" to "up + left + down + left paddings" interest/impact for you? Please comment.

(In reply to comment #11) > @Grace: We are trying to define a nice API for setting the padding values for QtWebKit. But it turns out that horizontal and vertical paddings *might* not be the best way to specify the paddings. We might end up needing separeted "up, right, down and left" paddings properties (comment #10). > > Could you please explain how android is making use of the rect-based hit testing we added? Is that through a public API or hardcoded values? > > Also, would changing from the rect-based hit testing padding from "horizontal + vertical paddings" to "up + left + down + left paddings" interest/impact for you? > > Please comment. Android currently is experimenting a simple padding. We are ok with the fine tuned api if it is needed for the other port.

(In reply to comment #8) > (From update of attachment 64548 [details]) > WebKit/qt/Api/qwebsettings.h:142 > + void setHitTestPadding(Qt::Orientation, int padding); > Hmm, this is a member function of QWebSettings, but it actually doesn't change any members but reads/writes directly to the global settings. I think it should either be a static function or the setting should propagate correct from global settings to page specific settings. Good point, simon. I can fix that. (In reply to comment #9) > Antonio, can you explain where in a Qt/Meegotouch application these padding values would come from? > > I like how this keeps things actually simple, it doesn't require changing all the APIs. I'm just curious where the values would come from and if they come from a system library like libmeegotouch, can we perhaps provide defaults? Applications should be the setters here, I think simon. About the defaults, we could either set defaults *or* document good values. I particularly prefer the later. I will check when mozilla values came from, and what interfer to them (resolution, dpi, etc). > What about pure touch events? Should WebCore/page/EventHandler.cpp: EventHandler::handleTouchEvent() use your new internal functions for better accuracy? I think for touch events, things could work similarly to mouse events: before submitting a touch event (click) to WebCore::EventHandler::handleTouchEvent, we would use similar heuristic to the one proposed in bug 44089 to "find" the clickable target user more likely aimed to tap. Then we'd adjust the QTouchEvent::TouchPoint::setPos(x,y) accordingly, and finally submit this adjusted position to webcore. If hardware provided an accurate rect through the QTouchEvent::TouchPoint::rect() we could just use it as the rect input. However, currently Qt is only passing the right touchpoint::rect for Windows-based devices. On linux (e.g. maemo6) it is wrong. As a fallback, we would build up the rect from the paddings. What to do?

> Android currently is experimenting a simple padding. We are ok with the fine tuned api if it is needed for the other port. @Grace, just to be sure: what would be a simple padding? Same padding value for both horizontal and vertical orientations?

(In reply to comment #14) > > Android currently is experimenting a simple padding. We are ok with the fine tuned api if it is needed for the other port. > > @Grace, just to be sure: what would be a simple padding? Same padding value for both horizontal and vertical orientations? Yes. We currently use the same value for horizontal and vertical. I understand Ben's example of tapping on a link. But users have adjusted to it. And some of them are tapping sideways. So arbitrary adjust the size may not help. Of course, this needs a lot of user testing.

Created attachment 66414 [details] patch v2 (In reply to comment #8) > (From update of attachment 64548 [details]) > WebKit/qt/Api/qwebsettings.h:142 > + void setHitTestPadding(Qt::Orientation, int padding); > > Hmm, this is a member function of QWebSettings, but it actually doesn't change any members but reads/writes directly to the global settings. I think it should either be a static function or the setting should propagate correct from global settings to page specific settings. Addressed with Simon's first suggestion here: made methods static functions. @Simon: at the moment I did not have a maemo6 device to test it, but n900 (maemo5). It would be great if it could get tested together with patch in bug 44089. Touch interaction result should be really better. @Benjamin: I understand your concern that we could be favoring elements above the touch point, as mozilla makes it possible to set with their API, but would the gain be a difference. API would be simpler as is, and it'd be great with we could keep it as is. Again, real testing would answer these questions.

> Android currently is experimenting a simple padding. We are ok with the fine tuned api if it is needed for the other port. @Grace, are you guys getting good TAP interaction improvements with using same padding value for UP and DOWN paddings?

(In reply to comment #17) > > Android currently is experimenting a simple padding. We are ok with the fine tuned api if it is needed for the other port. > > @Grace, are you guys getting good TAP interaction improvements with using same padding value for UP and DOWN paddings? This is very subjective. I think for a right hand person, tap touch point is normally at the bottom right relative to the finger tip. But people have been adjusted to this gesture when they tap. So we currently increase the touch area evenly.

Benjamin, please share result you got from testing on meego handset, if you got that far. Thank you in advance.

(In reply to comment #19) > Benjamin, please share result you got from testing on meego handset, if you got that far. I had been distracted by another problem on Friday, I have not tested your branch yet.

Created attachment 68060 [details] Usability test on netbook with touch screen This show the result of the patch on a MeeGo netbook with touchscreen. This video is with the 10x15 pad area.

Created attachment 68061 [details] Usability test on a netbook with touch screen This show the result of the patch on a MeeGo netbook with touchscreen. This video is with the 10x10 pad area.

Created attachment 68084 [details] Problem when the rect intersect the link on top of the touch point

(In reply to comment #23) > Created an attachment (id=68084) [details] > Problem when the rect intersect the link on top of the touch point Benjamin, I just found we work perfectly with the default zoom factor. Once I zoom in it stops working. I will fix that. In your experiments (specially in video _006), do you zoom in/out the page?

(In reply to comment #24) > In your experiments (specially in video _006), do you zoom in/out the page? Yep, it was zoomed out.

(In reply to comment #25) > (In reply to comment #24) > > In your experiments (specially in video _006), do you zoom in/out the page? > > Yep, it was zoomed out. Ah ha! we got our bug, then. I will fix it in bug 44089. Thanks much again, Ben.

Created attachment 68373 [details] Another quick test on netbook This video show another quick test of the feature with the last update of Antonio. This shows the usability is now very good. The only issue I had while testing is from https://bugs.webkit.org/show_bug.cgi?id=44089 : the linked are not clicked when the cursor is not hitting the same RenderBlock as the link.

This might interest here: https://lists.webkit.org/pipermail/webkit-dev/2010-September/014457.html We should opine if we would prefer to keep the API as simple as possible, i.e. keep set the padding per orientation (vertical and horizontal) as proposed by attachment 66414 [details], or if we are going to wait the webcore related change in bug 46336, and set the paddings per direction (top, right, bottom, left)? In summary: void QWebSettings::setHitTestPadding(Qt::Orientation orientation, int padding) X void QWebSettings::setHitTestPaddings(int top, int right, int bottom, int left) Thoughts?

(In reply to comment #27) > The only issue I had while testing is from https://bugs.webkit.org/show_bug.cgi?id=44089 : the linked are not clicked when the cursor is not hitting the same RenderBlock as the link. That is a great catch! Fix should go in WebCore. Will file a bug a paste here for the record.

We discussed this API today. We could not find use cases where application developers want to change that. We would prefer the platform to provide reasonable values. So what do you think about extending the platform plugin APIs to provide the numbers?

Sounds good to me!

(In reply to comment #30) > We discussed this API today. > > We could not find use cases where application developers want to change that. We would prefer the platform to provide reasonable values. > > So what do you think about extending the platform plugin APIs to provide the numbers? I am not very familiar with that, but can try.

(In reply to comment #30) > We discussed this API today. > > We could not find use cases where application developers want to change that. We would prefer the platform to provide reasonable values. > > So what do you think about extending the platform plugin APIs to provide the numbers? Sounds good to me too

Alright. So I am going to close this one as INVALID, and open a fresh new one :) Thanks for all feedback and testing.

(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > In your experiments (specially in video _006), do you zoom in/out the page? > > > > Yep, it was zoomed out. > > Ah ha! we got our bug, then. I will fix it in bug 44089. Thanks much again, Ben. Bug 46696 filed about it.

(In reply to comment #33) > (In reply to comment #30) > > We discussed this API today. > > > > We could not find use cases where application developers want to change that. We would prefer the platform to provide reasonable values. > > > > So what do you think about extending the platform plugin APIs to provide the numbers? > > Sounds good to me too Benjamin, yael, kenneth: should it go with the platform plugin approach? (kenneth and yael said YES, but benjamin pointed out something negative about it on IRC).

(In reply to comment #36) > Benjamin, yael, kenneth: should it go with the platform plugin approach? (kenneth and yael said YES, but benjamin pointed out something negative about it on IRC). I have been thinking about this while running :) The problem we have with the rect are the zoom factors: -scaled QGraphicsWebView -QGraphicsView with arbitrary transforms. The web frame are not aware of painter transformations above WebKit so the informations must be provided somehow. We could do 2 kind of APIs: 1) use the platform plugin to define a rect. Modify this rect on the fly by looking at the transforms of the item and the view from which the event comes (I guess we could use QGraphicsSceneEvent::widget()::parent(), this is unfortunately hackish). 2) let the use provide the scale factor per event. (2) is the complete solution since it allow arbitrary manipulations (like manually scaled painters on a QWebView). (1) will give a simpler API, and will be less error prone for developers using QtWebKit. I am in favor (1) since I think it is a more Qt-ish API (simple yet powerful :)). Kenneth, Yael, any suggestion? Do you agree what I describe above?

(In reply to comment #37) > (In reply to comment #36) > > Benjamin, yael, kenneth: should it go with the platform plugin approach? (kenneth and yael said YES, but benjamin pointed out something negative about it on IRC). > > I have been thinking about this while running :) > > The problem we have with the rect are the zoom factors: > -scaled QGraphicsWebView > -QGraphicsView with arbitrary transforms. > > The web frame are not aware of painter transformations above WebKit so the informations must be provided somehow. > > We could do 2 kind of APIs: > 1) use the platform plugin to define a rect. Modify this rect on the fly by looking at the transforms of the item and the view from which the event comes (I guess we could use QGraphicsSceneEvent::widget()::parent(), this is unfortunately hackish). > 2) let the use provide the scale factor per event. > > (2) is the complete solution since it allow arbitrary manipulations (like manually scaled painters on a QWebView). > (1) will give a simpler API, and will be less error prone for developers using QtWebKit. I am in favor (1) since I think it is a more Qt-ish API (simple yet powerful :)). > > Kenneth, Yael, any suggestion? Do you agree what I describe above? I'm OK with number 1. It should just work out of the box if possible.

invalid ==> remove block from release and API meta-bugs

I had a brief talk with Antonio and figured out that we have two different open bugs here: 1) Consider the eventual transformations in the QGraphicsWebView before looking for the click candidate nodes in the HitTestResult process which is bug #46696. 2) And the former one: to provide the padding values to be used in the HitTestResult hit area. As far as I could understand it should use *constant* values provided by the Platform Plugin. I mean, each platform should specify its preferred padding values and this value will not change even if a transformation in QGraphicsWebView occurs. Am I correct? May I reopen it and solve the item 2?

> 2) And the former one: to provide the padding values to be used in the HitTestResult hit area. As far as I could understand it should use *constant* values provided by the Platform Plugin. I mean, each platform should specify its preferred padding values and this value will not change even if a transformation in QGraphicsWebView occurs. > > May I reopen it and solve the item 2? Lets make a fresh new one for addressing this. You can refer to this one in the description.

(In reply to comment #41) > > 2) And the former one: to provide the padding values to be used in the HitTestResult hit area. As far as I could understand it should use *constant* values provided by the Platform Plugin. I mean, each platform should specify its preferred padding values and this value will not change even if a transformation in QGraphicsWebView occurs. > > > > May I reopen it and solve the item 2? > > Lets make a fresh new one for addressing this. You can refer to this one in the description. Pedralho filed it: bug 48450.

Comment on attachment 66414 [details] patch v2 Cleared review? from attachment 66414 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).