[webkit-reviews] review denied: [Bug 82692] -webkit-user-select: none makes text fields not editable : [Attachment 393234] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Carlos Alberto Lopez Perez
<clopez at igalia.com>'s request for review:
Bug 82692: -webkit-user-select: none makes text fields not editable
https://bugs.webkit.org/show_bug.cgi?id=82692

Attachment 393234: Patch

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




--- Comment #13 from Darin Adler <darin at apple.com> ---
Comment on attachment 393234
  --> https://bugs.webkit.org/attachment.cgi?id=393234
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.