Bug 82692 - -webkit-user-select: none makes text fields not editable
Summary: -webkit-user-select: none makes text fields not editable
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rakesh
URL:
Keywords: HasReduction
Depends on: 88514
Blocks: 208677
  Show dependency treegraph
 
Reported: 2012-03-29 22:55 PDT by Ryosuke Niwa
Modified: 2020-03-11 21:56 PDT (History)
26 users (show)

See Also:


Attachments
patch 1 (4.15 KB, patch)
2012-06-07 06:52 PDT, Rakesh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (765.34 KB, application/zip)
2012-06-07 12:11 PDT, WebKit Review Bot
no flags Details
Patch (20.33 KB, patch)
2020-03-11 06:49 PDT, Carlos Alberto Lopez Perez
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2012-03-29 22:55:42 PDT
You can't edit text inside the input element in the following example:

<style type="text/css">
.filter *{
-webkit-user-select: none;
-khtml-user-select: none;
-moz-user-select: none;
-o-user-select: none;
user-select: none;
}
</style>
<div class="filter">
<input type="text" placeholder="foobar" />
</div>

We should probably be limiting the effect of -webkit-user-select at shadow DOM boundary.
Comment 1 Ryosuke Niwa 2012-03-29 22:55:58 PDT
http://crbug.com/105876
Comment 2 Rakesh 2012-06-07 06:52:04 PDT
Created attachment 146279 [details]
patch 1

Modified to check the user select none condition on shadow root.
Comment 3 WebKit Review Bot 2012-06-07 12:11:28 PDT
Comment on attachment 146279 [details]
patch 1

Attachment 146279 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12910386

New failing tests:
editing/selection/select-all-user-select-none.html
fast/forms/input-select-webkit-user-select-none.html
editing/selection/5333725.html
fast/events/mouse-double-triple-click-should-not-select-next-node-for-user-select-none.html
editing/selection/5779984-1.html
Comment 4 WebKit Review Bot 2012-06-07 12:11:32 PDT
Created attachment 146356 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 5 Ryosuke Niwa 2012-06-07 13:53:55 PDT
Comment on attachment 146279 [details]
patch 1

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

> Source/WebCore/dom/Position.cpp:852
> -    return node && node->renderer() && node->renderer()->style()->userSelect() == SELECT_NONE;
> +    return node && node->renderer() && node->renderer()->style()->userSelect() == SELECT_NONE && node->isShadowRoot();

This is not the right fix. We need to update the style resolution code instead.
Otherwise, -webkit-user-select: none would never work inside shadow DOM.
Comment 6 Rakesh 2012-06-08 07:51:22 PDT
(In reply to comment #5)
> This is not the right fix. We need to update the style resolution code instead.
> Otherwise, -webkit-user-select: none would never work inside shadow DOM.

You are right, will check in style resolution code for better fix.
Comment 7 Ryosuke Niwa 2012-06-08 09:31:28 PDT
See the bug 88514. We're implementing the right solution for -webkit-user-modify there.
Comment 8 Dean Johnson 2015-11-17 16:06:29 PST
Still running into this on trunk WebKit -- was there any resolution on the correct behavior?
Comment 9 Ben Frain 2016-02-02 02:47:04 PST
Here is a reduction of this issue:

http://codepen.io/benfrain/pen/PZBOjJ

Attempt to select the text and nothing is selected - correct
Type in the input and nothing appears - incorrect

This works fine in Chrome & Firefox as per specifications: http://www.w3.org/TR/2000/WD-css3-userint-20000216#user-select
Comment 10 Ben Frain 2016-02-02 02:58:28 PST
Note for authors looking for a workaround, you can do this:

* {
	user-select: none;
}

input[type] {
	user-select: text;
}

in your CSS and inputs won't get the lanky user-select experience in Safari/iOS
Comment 11 evan 2019-11-25 07:34:17 PST
Just wanted to mention that I ran into this bug recently—we had accidentally applied user-select: none to a couple of our search fields, but since it worked fine on Chrome & Firefox we didn't notice for a long time that search was broken on Safari. We fixed it by removing the user-select: none style, but definitely a web-compat issue waiting to happen!
Comment 12 Carlos Alberto Lopez Perez 2020-03-11 06:49:45 PDT
Created attachment 393234 [details]
Patch
Comment 13 Darin Adler 2020-03-11 09:18:18 PDT
Comment on attachment 393234 [details]
Patch

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

Bug title makes the patch sound like this affects only text fields. Do we have extensive enough tests to see all the cases this affects?

> Source/WebCore/dom/Position.cpp:930
>  bool Position::nodeIsUserSelectNone(Node* node)
>  {
> -    return node && node->renderer() && node->renderer()->style().userSelect() == UserSelect::None;
> +    return node && node->renderer() && node->renderer()->style().userSelect() == UserSelect::None && node->renderer()->style().userModify() == UserModify::ReadOnly;
>  }

Before this patch, the function name here literally described its function. How the function does a "derived operation" that checks more than just "user select none", so I suggest renaming it. Maybe something like "unselectable" or "selectable" since that’s the term you coined inside RenderElement.cpp, or maybe "editable".

Also, the expression is now long enough that we might want to switch to the vertical && style we use for long expressions.

    return node && node->renderer()
        && node->renderer()-> ...
        && ...;

Finally, I suggest sharing code between this and the code inside RenderElement. Perhaps we should make this a public function member on RenderElement? Or maybe it should stay somewhere in editing code. Maybe it’s OK to leave it a member of Position, but we should offer a version that takes a const RenderStyle& alongside a different version that takes a Node*, which calls the RenderStyle one.

We should *not* change this function’s meaning without changing its name.
Comment 14 Ryosuke Niwa 2020-03-11 09:41:44 PDT
Comment on attachment 393234 [details]
Patch

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

> Source/WebCore/dom/Position.cpp:929
> +    return node && node->renderer() && node->renderer()->style().userSelect() == UserSelect::None && node->renderer()->style().userModify() == UserModify::ReadOnly;

This is clearly not right. We have an important client for -webkit-user-select: none that uses it in contenteditable.
Comment 15 Ryosuke Niwa 2020-03-11 09:42:45 PDT
Comment on attachment 393234 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        This patch makes CSS property -webkit-user-select:none not to affect editability as FireFox and Chromium.

This description is inaccurate. What you're doing is disabling -webkit-user-select: none inside any editable region. This is not okay.
Comment 16 Carlos Alberto Lopez Perez 2020-03-11 10:06:23 PDT
(In reply to Darin Adler from comment #13)
> Comment on attachment 393234 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393234&action=review
> 
> Bug title makes the patch sound like this affects only text fields. Do we
> have extensive enough tests to see all the cases this affects?
> 

I hope so. I ran all the layout tests and did a bunch of manual tests.
Do you have any suggestions for testing this other than running all the WebKit layout tests?

> > Source/WebCore/dom/Position.cpp:930
> >  bool Position::nodeIsUserSelectNone(Node* node)
> >  {
> > -    return node && node->renderer() && node->renderer()->style().userSelect() == UserSelect::None;
> > +    return node && node->renderer() && node->renderer()->style().userSelect() == UserSelect::None && node->renderer()->style().userModify() == UserModify::ReadOnly;
> >  }
> 
> Before this patch, the function name here literally described its function.
> How the function does a "derived operation" that checks more than just "user
> select none", so I suggest renaming it. Maybe something like "unselectable"
> or "selectable" since that’s the term you coined inside RenderElement.cpp,
> or maybe "editable".
> 
> Also, the expression is now long enough that we might want to switch to the
> vertical && style we use for long expressions.
> 
>     return node && node->renderer()
>         && node->renderer()-> ...
>         && ...;
> 
> Finally, I suggest sharing code between this and the code inside
> RenderElement. Perhaps we should make this a public function member on
> RenderElement? Or maybe it should stay somewhere in editing code. Maybe it’s
> OK to leave it a member of Position, but we should offer a version that
> takes a const RenderStyle& alongside a different version that takes a Node*,
> which calls the RenderStyle one.
> 
> We should *not* change this function’s meaning without changing its name.

Here I backported a patch from Chromium that fixed this bug pretty much as this patch, I simply renamed the function shouldUseSelectionColor() to isUnselectable() and inverted the order (as it looked more readable to me)  See: https://chromium.googlesource.com/chromium/src/+/f66b7d356cf75be230690332b1ac34951e8b018c%5E%21/


I will try to take your suggestions (and Ryosuke ones) into account for a next version of the patch. Thanks
Comment 17 Carlos Alberto Lopez Perez 2020-03-11 11:11:36 PDT
(In reply to Ryosuke Niwa from comment #15)
> Comment on attachment 393234 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393234&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        This patch makes CSS property -webkit-user-select:none not to affect editability as FireFox and Chromium.
> 
> This description is inaccurate. What you're doing is disabling
> -webkit-user-select: none inside any editable region. This is not okay.

I agree that the description can be improved, but ..

do you mean its not ok to disable -webkit-user-select: none inside any editable region ? why?

I have created this simple test:

https://people.igalia.com/clopez/wkbug/82692/test-select-none-input-div-contenteditable.html


As far as I can see, this happens:

 * firefox allows to edit on any input element (test1) or any editable element (test3), but it doesn't allow to select the text with the mouse cursor.
 * chrome allows both to edit and to select the text with the mouse cursor on any input element (test1) and editable element (test3)
 * webkit doesn't allow to edit, neither to select the text (test1 and test3).


What should be the desired behaviour? My goal was to align this behaviour with what Chrome does to reduce the risk of web-compat before unprefixing this property in bug 208677
Comment 18 Ryosuke Niwa 2020-03-11 11:29:37 PDT
(In reply to Carlos Alberto Lopez Perez from comment #17)
> (In reply to Ryosuke Niwa from comment #15)
> > Comment on attachment 393234 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=393234&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        This patch makes CSS property -webkit-user-select:none not to affect editability as FireFox and Chromium.
> > 
> > This description is inaccurate. What you're doing is disabling
> > -webkit-user-select: none inside any editable region. This is not okay.
> 
> I agree that the description can be improved, but ..
> 
> do you mean its not ok to disable -webkit-user-select: none inside any
> editable region ? why?

Definitely not.

> I have created this simple test:
> 
> https://people.igalia.com/clopez/wkbug/82692/test-select-none-input-div-
> contenteditable.html
> 
> 
> As far as I can see, this happens:
> 
>  * firefox allows to edit on any input element (test1) or any editable
> element (test3), but it doesn't allow to select the text with the mouse
> cursor.
>  * chrome allows both to edit and to select the text with the mouse cursor
> on any input element (test1) and editable element (test3)
>  * webkit doesn't allow to edit, neither to select the text (test1 and
> test3).

That's the expected behavior. We don't want to allow editing of with -webkit-user-select: none. Content outside of what's being marked as -webkit-user-select: none should be editable, but not the content marked as -webkit-user-select: none.

> What should be the desired behaviour? My goal was to align this behaviour
> with what Chrome does to reduce the risk of web-compat before unprefixing
> this property in bug 208677

I don't think that's something we want to do.
Comment 19 evan 2020-03-11 11:36:37 PDT
(In reply to Carlos Alberto Lopez Perez from comment #17)
>  * firefox allows to edit on any input element (test1) or any editable
> element (test3), but it doesn't allow to select the text with the mouse
> cursor.

Just as a clarification, in my browser (Firefox 73 on macOS), I am able to select text and edit freely inside the input element. Screen recording: https://www.dropbox.com/s/nomf5jkdybx42iq/user%20select%20none.mp4?dl=0

I have no real opinion on contenteditable behavior, it definitely seems like more of an edge case.
Comment 20 Carlos Alberto Lopez Perez 2020-03-11 11:43:11 PDT
(In reply to Ryosuke Niwa from comment #18)
> > What should be the desired behaviour? My goal was to align this behaviour
> > with what Chrome does to reduce the risk of web-compat before unprefixing
> > this property in bug 208677
> 
> I don't think that's something we want to do.

Ok, so if I understood you right, we only want to allow editing text inside <input> elements. That's right?

And other than editing, what about selecting text with the mouse inside an input element that has -webkit-user-select:none? Do we want to allow that?
Comment 21 Ryosuke Niwa 2020-03-11 12:14:14 PDT
(In reply to Carlos Alberto Lopez Perez from comment #20)
> (In reply to Ryosuke Niwa from comment #18)
> > > What should be the desired behaviour? My goal was to align this behaviour
> > > with what Chrome does to reduce the risk of web-compat before unprefixing
> > > this property in bug 208677
> > 
> > I don't think that's something we want to do.
> 
> Ok, so if I understood you right, we only want to allow editing text inside
> <input> elements. That's right?

Right.

> And other than editing, what about selecting text with the mouse inside an
> input element that has -webkit-user-select:none? Do we want to allow that?

I guess we can match other browsers there. I don't think -webkit-user-select: none on input element is a scenario we deeply care about although any backwards incompatible change is risky so such a code change should be very carefully studied.
Comment 22 Carlos Alberto Lopez Perez 2020-03-11 14:57:48 PDT
(In reply to Ryosuke Niwa from comment #21)
> (In reply to Carlos Alberto Lopez Perez from comment #20)
> > (In reply to Ryosuke Niwa from comment #18)
> > > > What should be the desired behaviour? My goal was to align this behaviour
> > > > with what Chrome does to reduce the risk of web-compat before unprefixing
> > > > this property in bug 208677
> > > 
> > > I don't think that's something we want to do.
> > 
> > Ok, so if I understood you right, we only want to allow editing text inside
> > <input> elements. That's right?
> 
> Right.
> 

What about <textarea> ? Should that be editable if it has -webkit-user-select:no ?
Comment 23 Ryosuke Niwa 2020-03-11 21:56:28 PDT
(In reply to Carlos Alberto Lopez Perez from comment #22)
> (In reply to Ryosuke Niwa from comment #21)
> > (In reply to Carlos Alberto Lopez Perez from comment #20)
> > > (In reply to Ryosuke Niwa from comment #18)
> > > > > What should be the desired behaviour? My goal was to align this behaviour
> > > > > with what Chrome does to reduce the risk of web-compat before unprefixing
> > > > > this property in bug 208677
> > > > 
> > > > I don't think that's something we want to do.
> > > 
> > > Ok, so if I understood you right, we only want to allow editing text inside
> > > <input> elements. That's right?
> > 
> > Right.
> > 
> 
> What about <textarea> ? Should that be editable if it has
> -webkit-user-select:no ?

Same as input. We can probably match what other browsers are doing.