Bug 57289 – "user-select none" causes selection to incorrectly escape from clicked container

• Component: HTML Editing
• Status: RESOLVED FIXED

[Benjamin (Ben) Kalman]

See the test case.

It seems like putting user-select:none on containers causes the selection to escape from them.  More concretely,
 - double clicking on a user-select:none div causes the word after the div to be selected
 - triple clicking causes the the paragraph after the div

In fact, a simpler test case is just using the selection API:
 - setPosition(child1, 0) then querying the selection base returns "Foo" when presumably it... shouldn't.

[Benjamin (Ben) Kalman]

Created attachment 87244 [details]
Test case

[Rakesh]

Created attachment 119013 [details]
Prposed patch

If the style for the target node is user-select:none, we do not dispatch a selection start for nearest word from mouse event.

[Ryosuke Niwa]

Comment on attachment 119013 [details]
Prposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119013&action=review

> Source/WebCore/page/EventHandler.cpp:301
> +    if (targetNode)
> +        if (RenderObject* renderer = targetNode->renderer())
> +            if (renderer->style() && renderer->style()->userSelect() == SELECT_NONE)
> +                return false;

We need { } for if (target) and if (RenderObject* ~).

We should do this check before we dispatch select start.

[Rakesh]

Created attachment 119076 [details]
Updated Patch

Moved the change before dispatch select start and added proper braces.

[Ryosuke Niwa]

Comment on attachment 119076 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119076&action=review

> LayoutTests/fast/events/mouse-double-triple-click-should-not-select-next-node-for-user-select-none-expected.txt:7
> +1
> +2
> +3
> +4
> +Foo

Can we hide these?

> LayoutTests/fast/events/mouse-double-triple-click-should-not-select-next-node-for-user-select-none.html:36
> +    debug("Check double click");
> +    eventSender.mouseDown();
> +    eventSender.mouseUp();
> +    eventSender.mouseDown();
> +    eventSender.mouseUp();
> +    shouldBeTrue("document.getSelection() !=\'Foo\'");

Instead of using debug to print "Check double click", you can just create a function like doubleClick() and do:
shouldBeTrue("doubleClick(); document.getSelection() !=\'Foo\'");

By the way, could you add new function to js-test-pre.js so that you can do:
shouldBeNotEqual("doubleClick(); document.getSelection()", "'Foo'");
instead?

> LayoutTests/fast/events/mouse-double-triple-click-should-not-select-next-node-for-user-select-none.html:40
> +    debug("Clearing selection if any");
> +    document.getSelection().removeAllRanges();
> +    shouldBeTrue("document.getSelection() !=\'Foo\'");

Similarly, shouldBeTrue("document.getSelection().removeAllRanges(); document.getSelection() !=\'Foo\'");

> Source/WebCore/page/EventHandler.cpp:300
> +    if (targetNode) {
> +        if (RenderObject* renderer = targetNode->renderer()) {
> +            if (renderer->style() && renderer->style()->userSelect() == SELECT_NONE)
> +                return false;
> +        }
> +    }

You should call Position::nodeIsUserSelectNone here.

[Rakesh]

Created attachment 119397 [details]
Updated patch

Changes as suggested.

[WebKit Review Bot]

Comment on attachment 119397 [details]
Updated patch

Clearing flags on attachment: 119397

Committed r102918: <http://trac.webkit.org/changeset/102918>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

http://crbug.com/106180