Bug 51653 – Text selection code shouldn't be invoked when -webkit-user-select is none

• Status: RESOLVED
• Resolution: INVALID
• Component: HTML Editing

[Noam Rosenthal]

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.

[Noam Rosenthal]

Created attachment 77517 [details]
Patch

[Kenneth Rohde Christiansen]

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]

> 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]

Created attachment 77519 [details]
Patch

[Eric Seidel (no email)]

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]

(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]

Created attachment 77578 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77578 [details]
Patch

LGTM.  Thanks.

[WebKit Commit Bot]

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)]

Looks like this needs an updated patch.

[Noam Rosenthal]

(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]

adding keyword "InTSW" for tracking by QtWebKit EM

[Ryosuke Niwa]

Is this bug fixed or we haven't landed the patch yet because of the test failure?

[Noam Rosenthal]

The test failure actually pointed to a problem that makes the patch invalid. Need to find a different way to do this.

[Jocelyn Turcotte]

=== 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.