Bug 237525 - [selection] Set correct selection range for TEXTAREA when updating default value
Summary: [selection] Set correct selection range for TEXTAREA when updating default value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zsun
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-07 05:38 PST by zsun
Modified: 2022-05-02 12:13 PDT (History)
8 users (show)

See Also:


Attachments
Patch (39.34 KB, patch)
2022-03-07 06:21 PST, zsun
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (39.33 KB, patch)
2022-03-07 06:52 PST, zsun
no flags Details | Formatted Diff | Diff
Patch (7.74 KB, patch)
2022-04-29 05:32 PDT, zsun
no flags Details | Formatted Diff | Diff
Patch (7.67 KB, patch)
2022-05-02 00:59 PDT, zsun
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 zsun 2022-03-07 06:21:05 PST
Created attachment 453971 [details]
Patch
Comment 2 zsun 2022-03-07 06:52:18 PST
Created attachment 453975 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2022-03-14 06:39:16 PDT
<rdar://problem/90243598>
Comment 4 zsun 2022-04-29 05:32:26 PDT
Created attachment 458582 [details]
Patch
Comment 5 Tim Nguyen (:ntim) 2022-04-29 07:25:11 PDT
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.
Comment 6 Chris Dumez 2022-04-29 07:28:48 PDT
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?
Comment 7 Tim Nguyen (:ntim) 2022-04-29 07:30:41 PDT
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`
Comment 8 zsun 2022-05-02 00:59:13 PDT
Created attachment 458666 [details]
Patch
Comment 9 Tim Nguyen (:ntim) 2022-05-02 11:34:02 PDT
Comment on attachment 458666 [details]
Patch

I assume this is ready to cq+
Comment 10 EWS 2022-05-02 12:13:17 PDT
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].