Bug 51653 - Text selection code shouldn't be invoked when -webkit-user-select is none
Summary: Text selection code shouldn't be invoked when -webkit-user-select is none
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords: Performance, Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-12-27 13:25 PST by Noam Rosenthal
Modified: 2014-02-03 03:50 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.84 KB, patch)
2010-12-27 13:35 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (1.84 KB, patch)
2010-12-27 14:26 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2010-12-28 14:19 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 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.
Comment 1 Noam Rosenthal 2010-12-27 13:35:05 PST
Created attachment 77517 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 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.
Comment 3 Noam Rosenthal 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...
Comment 4 Noam Rosenthal 2010-12-27 14:26:20 PST
Created attachment 77519 [details]
Patch
Comment 5 Eric Seidel (no email) 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?
Comment 6 Noam Rosenthal 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.
Comment 7 Noam Rosenthal 2010-12-28 14:19:16 PST
Created attachment 77578 [details]
Patch
Comment 8 Eric Seidel (no email) 2010-12-28 14:20:56 PST
Comment on attachment 77578 [details]
Patch

LGTM.  Thanks.
Comment 9 WebKit Commit Bot 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
Comment 10 Eric Seidel (no email) 2011-01-11 03:12:04 PST
Looks like this needs an updated patch.
Comment 11 Noam Rosenthal 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 :)
Comment 12 Joel Parks 2011-03-22 15:30:52 PDT
adding keyword "InTSW" for tracking by QtWebKit EM
Comment 13 Ryosuke Niwa 2011-04-03 00:52:02 PDT
Is this bug fixed or we haven't landed the patch yet because of the test failure?
Comment 14 Noam Rosenthal 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.
Comment 15 Jocelyn Turcotte 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.