Bug 237210

Summary: [Selection] Selection Range should be clamped by the current value length
Product: WebKit Reporter: zsun
Component: FormsAssignee: zsun
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, mifenton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Comment 1 zsun 2022-02-25 07:12:10 PST
Created attachment 453212 [details]
Patch
Comment 2 Darin Adler 2022-02-27 12:17:58 PST
Comment on attachment 453212 [details]
Patch

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

> Source/WebCore/html/HTMLTextFormControlElement.cpp:283
> +    const int innerTextValueLength = innerTextValue().length();
> +    end = std::min(std::max(end, 0), std::max(innerTextValueLength, 0));
>      start = std::min(std::max(start, 0), end);

The "const" on the first line is not WebKit coding style. Most local variables are constant, not reassigned, and we typically don’t put "const" in front of all of them, although we could. I suggest writing this instead:

    constexpr unsigned maxInt = std::numeric_limits<int>::max();
    int innerTextValueLength = std::min(innerTextValue().length(), maxInt);
    end = std::clamp(end, 0, innerTextValueLength);
    start = std::clamp(start, 0, end);
Comment 3 Darin Adler 2022-02-27 12:19:57 PST
Comment on attachment 453212 [details]
Patch

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

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:283
>>      start = std::min(std::max(start, 0), end);
> 
> The "const" on the first line is not WebKit coding style. Most local variables are constant, not reassigned, and we typically don’t put "const" in front of all of them, although we could. I suggest writing this instead:
> 
>     constexpr unsigned maxInt = std::numeric_limits<int>::max();
>     int innerTextValueLength = std::min(innerTextValue().length(), maxInt);
>     end = std::clamp(end, 0, innerTextValueLength);
>     start = std::clamp(start, 0, end);

Here’s a better idea:

    end = std::clamp(end, 0, clampTo<int>(innerTextValue().length()));
    start = std::clamp(start, 0, end);
Comment 4 zsun 2022-02-28 01:46:21 PST
Created attachment 453376 [details]
Patch
Comment 5 EWS 2022-03-01 01:02:20 PST
Committed r290635 (247908@main): <https://commits.webkit.org/247908@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453376 [details].
Comment 6 Radar WebKit Bug Importer 2022-03-01 01:03:17 PST
<rdar://problem/89607167>