RESOLVED FIXED Bug 237525
[selection] Set correct selection range for TEXTAREA when updating default value
https://bugs.webkit.org/show_bug.cgi?id=237525
Summary [selection] Set correct selection range for TEXTAREA when updating default value
Attachments
Patch (39.34 KB, patch)
2022-03-07 06:21 PST, zsun
ews-feeder: commit-queue-
Patch (39.33 KB, patch)
2022-03-07 06:52 PST, zsun
no flags
Patch (7.74 KB, patch)
2022-04-29 05:32 PDT, zsun
no flags
Patch (7.67 KB, patch)
2022-05-02 00:59 PDT, zsun
no flags
zsun
Comment 1 2022-03-07 06:21:05 PST
zsun
Comment 2 2022-03-07 06:52:18 PST
Radar WebKit Bug Importer
Comment 3 2022-03-14 06:39:16 PDT
zsun
Comment 4 2022-04-29 05:32:26 PDT
Tim Nguyen (:ntim)
Comment 5 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.
Chris Dumez
Comment 6 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?
Tim Nguyen (:ntim)
Comment 7 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`
zsun
Comment 8 2022-05-02 00:59:13 PDT
Tim Nguyen (:ntim)
Comment 9 2022-05-02 11:34:02 PDT
Comment on attachment 458666 [details] Patch I assume this is ready to cq+
EWS
Comment 10 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].
Note You need to log in before you can comment on or make changes to this bug.