Bug 115824

Summary: [BlackBerry] Don't set active or hover styles on touch start
Product: WebKit Reporter: gmak
Component: UI EventsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: andersca, buildbot, commit-queue, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch with changelog
buildbot: commit-queue-
Patch with changelog
none
Patch with changelog
buildbot: commit-queue-
Patch with changelog benjamin: review-

Description gmak 2013-05-08 14:18:18 PDT
Setting active or hover styles on links and other parts of the tree can require a relayout and a style recalc which on some pages such as links on http://en.wikipedia.org/wiki/Olympic_Games (desktop version) can take over 300 ms. 
We would like to be able to set elements active if they have a touch listener or are form controls.

Anecdotally, those doing a mouseover on those links on safari desktop results in approx 20ms of style recalc and 20ms of layout which seems high compared to other sites, but not being familiar with its stats I can't tell if its reasonable.
Comment 1 gmak 2013-05-08 14:29:26 PDT
Created attachment 201109 [details]
Patch with changelog
Comment 2 WebKit Commit Bot 2013-05-08 14:31:26 PDT
Attachment 201109 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/EventHandler.cpp']" exit_code: 1
Source/WebCore/page/EventHandler.cpp:466:  Do not add platform specific code in WebCore outside of platform.  [build/webcore_platform_layering_violation] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Build Bot 2013-05-08 14:51:56 PDT
Comment on attachment 201109 [details]
Patch with changelog

Attachment 201109 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/428067
Comment 4 Build Bot 2013-05-08 15:09:07 PDT
Comment on attachment 201109 [details]
Patch with changelog

Attachment 201109 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/281496
Comment 5 gmak 2013-05-09 15:07:39 PDT
Created attachment 201284 [details]
Patch with changelog
Comment 6 WebKit Commit Bot 2013-05-09 15:09:35 PDT
Attachment 201284 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/EventHandler.cpp']" exit_code: 1
Source/WebCore/page/EventHandler.cpp:466:  Do not add platform specific code in WebCore outside of platform.  [build/webcore_platform_layering_violation] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 gmak 2013-05-15 13:21:11 PDT
Created attachment 201873 [details]
Patch with changelog
Comment 8 Build Bot 2013-05-15 13:37:42 PDT
Comment on attachment 201873 [details]
Patch with changelog

Attachment 201873 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/473636
Comment 9 gmak 2013-05-15 13:39:06 PDT
Created attachment 201876 [details]
Patch with changelog
Comment 10 Benjamin Poulain 2013-05-15 14:26:49 PDT
Comment on attachment 201876 [details]
Patch with changelog

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

> Source/WebCore/ChangeLog:15
> +        Reviewed Internally by Mike Fenton and Mike Lattanzio.
> +        Setting active or hover styles on links can require a style recalculation and a relayout which on some pages such as
> +        http://en.wikipedia.org/wiki/Olympic_Games (desktop version) can take over 300 ms and severly impact our scrolling reaction time.
> +        This optimization still lets us use active styles if the element is a form control (so that we can get depressed
> +        button states) or if the element or its parent has a touch listener (this aligns us with ios and android behaviour.)
> +
> +        No new tests because it shouldn't change behaviour for anybody else.

I sounds more like you have a bad architecture. Why would layout influence the scrolling, I thought you had a multiprocess WebKit on blackberry?

> Source/WebCore/page/efl/EventHandlerEfl.cpp:131
> +    notImplemented();

This is not a notImplemented.
Comment 11 gmak 2013-05-15 14:50:06 PDT
(In reply to comment #10)
> (From update of attachment 201876 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201876&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Reviewed Internally by Mike Fenton and Mike Lattanzio.
> > +        Setting active or hover styles on links can require a style recalculation and a relayout which on some pages such as
> > +        http://en.wikipedia.org/wiki/Olympic_Games (desktop version) can take over 300 ms and severly impact our scrolling reaction time.
> > +        This optimization still lets us use active styles if the element is a form control (so that we can get depressed
> > +        button states) or if the element or its parent has a touch listener (this aligns us with ios and android behaviour.)
> > +
> > +        No new tests because it shouldn't change behaviour for anybody else.
> 
> I sounds more like you have a bad architecture. Why would layout influence the scrolling, I thought you had a multiprocess WebKit on blackberry?

Its because for us touch events are sync, and we want to set the element active/hovered in some cases such as form controls or if there's a touch event handler. So if setting something active such as links requires a big relayout and style recalc, then webkit grinds away doing that before it returns with whether the event has been consumed or not.

As far as I can tell ios safari simply doesn't send touch events to pages with no touch event handlers which prevents elements from being set active which in turn prevents this problem, but we want different behaviour.

> 
> > Source/WebCore/page/efl/EventHandlerEfl.cpp:131
> > +    notImplemented();
> 
> This is not a notImplemented.
Sure, I can remove the notImplemented().