Bug 51653

Summary: Text selection code shouldn't be invoked when -webkit-user-select is none
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: commit-queue, eric, joel.parks, menard, rniwa
Priority: P2 Keywords: Performance, Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Noam Rosenthal
Reported 2010-12-27 13:25:13 PST
This has found to have a major performance impact on some use cases, mainly when using finger-touch or mouse to scroll. It seems like even when user-select is turned off, the logic to break the text into words/characters, which is part of the selection logic, is unnecessarily invoked. The text-breaking code in some cases can be performance intensive. This can be rectified by making sure that the user-select code is on before starting the text-breaking logic.
Attachments
Patch (1.84 KB, patch)
2010-12-27 13:35 PST, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (1.84 KB, patch)
2010-12-27 14:26 PST, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Patch (1.96 KB, patch)
2010-12-28 14:19 PST, Noam Rosenthal 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Noam Rosenthal
Comment 1 2010-12-27 13:35:05 PST
Created attachment 77517 [details] Patch
Kenneth Rohde Christiansen
Comment 2 2010-12-27 14:15:47 PST
Comment on attachment 77517 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77517&action=review > WebCore/page/EventHandler.cpp:611 > + const RenderStyle* style = targetRenderer->style(false); It would be nice to know what the false stood for. Without knowing that this code is quite unreadable.
Noam Rosenthal
Comment 3 2010-12-27 14:22:12 PST
> It would be nice to know what the false stood for. Without knowing that this code is quite unreadable. Yep, good ole boolean trap. It stands for "firstLine" :) In this case we want the regular style. I'd make that more readable after I get some more comments about the content...
Noam Rosenthal
Comment 4 2010-12-27 14:26:20 PST
Created attachment 77519 [details] Patch
Eric Seidel (no email)
Comment 5 2010-12-28 13:58:47 PST
Comment on attachment 77519 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77519&action=review Good idea, we should just clean this up slightly. > WebCore/page/EventHandler.cpp:617 > + // Don't modify the selection for nodes with -webkit-user-select: none. > + const RenderStyle* style = targetRenderer->style(); > + const EUserSelect userSelect = style->userSelect(); > + if (userSelect == SELECT_NONE) { > + const RenderStyle* styleForFirstLine = targetRenderer->firstLineStyle(); > + if (styleForFirstLine == style || styleForFirstLine->userSelect() == SELECT_NONE) > + return; > + } This feels like a separate (private or static) function. if (!selectionEnabled()) return; or something like that, no?
Noam Rosenthal
Comment 6 2010-12-28 14:01:59 PST
(In reply to comment #5) > (From update of attachment 77519 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77519&action=review > > This feels like a separate (private or static) function. > > if (!selectionEnabled()) > return; > > or something like that, no? Sure thing. Will repost promptly.
Noam Rosenthal
Comment 7 2010-12-28 14:19:16 PST
Created attachment 77578 [details] Patch
Eric Seidel (no email)
Comment 8 2010-12-28 14:20:56 PST
Comment on attachment 77578 [details] Patch LGTM. Thanks.
WebKit Commit Bot
Comment 9 2010-12-28 17:27:17 PST
Comment on attachment 77578 [details] Patch Rejecting attachment 77578 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2 Last 500 characters of output: .......................................................................................................................................................................................................... editing/selection .................................................... editing/selection/5333725.html -> failed Exiting early after 1 failures. 5182 tests run. 105.66s total testing time 5181 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7290229
Eric Seidel (no email)
Comment 10 2011-01-11 03:12:04 PST
Looks like this needs an updated patch.
Noam Rosenthal
Comment 11 2011-01-11 05:19:49 PST
(In reply to comment #10) > Looks like this needs an updated patch. I agree but I'm not sure how to fix it :)
Joel Parks
Comment 12 2011-03-22 15:30:52 PDT
adding keyword "InTSW" for tracking by QtWebKit EM
Ryosuke Niwa
Comment 13 2011-04-03 00:52:02 PDT
Is this bug fixed or we haven't landed the patch yet because of the test failure?
Noam Rosenthal
Comment 14 2011-04-03 08:24:18 PDT
The test failure actually pointed to a problem that makes the patch invalid. Need to find a different way to do this.
Jocelyn Turcotte
Comment 15 2014-02-03 03:50:51 PST
=== Bulk closing of Qt bugs === If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it. If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.
Note You need to log in before you can comment on or make changes to this bug.