Summary: | Text selection code shouldn't be invoked when -webkit-user-select is none | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Noam Rosenthal <noam> | ||||||||
Component: | HTML Editing | Assignee: | 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
Noam Rosenthal
2010-12-27 13:25:13 PST
Created attachment 77517 [details]
Patch
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. > 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...
Created attachment 77519 [details]
Patch
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? (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. Created attachment 77578 [details]
Patch
Comment on attachment 77578 [details]
Patch
LGTM. Thanks.
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 Looks like this needs an updated patch. (In reply to comment #10) > Looks like this needs an updated patch. I agree but I'm not sure how to fix it :) adding keyword "InTSW" for tracking by QtWebKit EM Is this bug fixed or we haven't landed the patch yet because of the test failure? The test failure actually pointed to a problem that makes the patch invalid. Need to find a different way to do this. === 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. |