Bug 237525 – [selection] Set correct selection range for TEXTAREA when updating default value

• Component: CSS
• Status: RESOLVED FIXED

https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#textFieldSelection:concept-textarea/input-relevant-value

Created attachment 453971 [details]
Patch

Created attachment 453975 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/90243598>

Created attachment 458582 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 458582 [details]
Patch

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

> Source/WebCore/html/HTMLTextAreaElement.cpp:408
> +    unsigned selectionStartValue = isClamp ? computeSelectionStart() : 0;
> +    unsigned selectionEndValue = isClamp ? computeSelectionEnd() : 0;

auto

> Source/WebCore/html/HTMLTextFormControlElement.h:86
> +    WEBCORE_EXPORT unsigned computeSelectionStart() const;
> +    WEBCORE_EXPORT unsigned computeSelectionEnd() const;

This doesn't need WEBCORE_EXPORT, you're not using outside of the webcore directory.

[Chris Dumez]

Comment on attachment 458582 [details]
Patch

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

r=me with comments. (also agree with all of Tim's)

> Source/WebCore/html/HTMLTextAreaElement.cpp:139
> +        setNonDirtyValue(defaultValue(),  TextControlSetValueSelection::Clamp);

Too many spaces after the comma.

> Source/WebCore/html/HTMLTextAreaElement.cpp:406
> +    bool isClamp = selection == TextControlSetValueSelection::Clamp;

I think this indicates whether we want to clamp or not. If so, I think "shouldClamp" would be a better variable name.

>> Source/WebCore/html/HTMLTextFormControlElement.h:86
>> +    WEBCORE_EXPORT unsigned computeSelectionEnd() const;
> 
> This doesn't need WEBCORE_EXPORT, you're not using outside of the webcore directory.

True but also, why did you make them public. Seems like they could be protected?

[Tim Nguyen (:ntim)]

Comment on attachment 458582 [details]
Patch

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

> Source/WebCore/html/HTMLTextAreaElement.cpp:426
>      if (document().focusedElement() == this) {
>          unsigned endOfString = m_value.length();
>          setSelectionRange(endOfString, endOfString);
> -    }  else if (selection == TextControlSetValueSelection::SetSelectionToEnd) {
> +    } else if (selection == TextControlSetValueSelection::SetSelectionToEnd) {
>          // We don't change text selection here but need to update caret to
>          // the end of the text value except for initialize.
>          unsigned endOfString = m_value.length();
>          cacheSelection(endOfString, endOfString, SelectionHasNoDirection);
> +    } else if (isClamp) {
> +        unsigned endOfString = m_value.length();

Since all 3 branches use `endOfString`, can we factor it out of the if/elses?

Also `unsigned` -> `auto`

Created attachment 458666 [details]
Patch

[Tim Nguyen (:ntim)]

Comment on attachment 458666 [details]
Patch

I assume this is ready to cq+

[EWS]

Committed r293673 (250175@main): <https://commits.webkit.org/250175@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458666 [details].