Bug 115824 - [BlackBerry] Don't set active or hover styles on touch start
Summary: [BlackBerry] Don't set active or hover styles on touch start
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-08 14:18 PDT by gmak
Modified: 2022-02-28 03:55 PST (History)
4 users (show)

See Also:


Attachments
Patch with changelog (5.04 KB, patch)
2013-05-08 14:29 PDT, gmak
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch with changelog (5.08 KB, patch)
2013-05-09 15:07 PDT, gmak
no flags Details | Formatted Diff | Diff
Patch with changelog (9.78 KB, patch)
2013-05-15 13:21 PDT, gmak
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch with changelog (9.84 KB, patch)
2013-05-15 13:39 PDT, gmak
benjamin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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().