Bug 139447

Summary: REGRESSION(r164329): Input fields are not honoring the maxlength attribute
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: FormsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, commit-queue, darin, enrica, kling
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Fixes the bug
none
Addressed Darin's comments
none
Patch for landing none

Description Ryosuke Niwa 2014-12-09 08:17:29 PST
maxlength attribute is not honored when text is inserted in the middle of a text field.
Comment 1 Ryosuke Niwa 2014-12-09 08:21:39 PST
Created attachment 242923 [details]
Fixes the bug
Comment 2 Darin Adler 2014-12-09 09:01:21 PST
Comment on attachment 242923 [details]
Fixes the bug

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

> Source/WebCore/html/TextFieldInputType.cpp:389
> +        int selectedOffsets = selectionStart - element().selectionEnd();

This looks backwards. Computing start - end would give a negative number; not sure why this works. I also think that “offsets” is a confusing name for this. Maybe selectionCodeUnitCount?

> Source/WebCore/html/TextFieldInputType.cpp:390
> +        selectionLength = selectedOffsets ? numGraphemeClusters(innerText.substring(selectionStart, selectedOffsets)) : 0;

Would be nice, some day, to have this use StringView::substring instead of String::substring.
Comment 3 Ryosuke Niwa 2014-12-09 10:42:10 PST
Created attachment 242939 [details]
Addressed Darin's comments
Comment 4 Ryosuke Niwa 2014-12-09 10:42:49 PST
(In reply to comment #2)
> Comment on attachment 242923 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242923&action=review
> 
> > Source/WebCore/html/TextFieldInputType.cpp:389
> > +        int selectedOffsets = selectionStart - element().selectionEnd();
> 
> This looks backwards. Computing start - end would give a negative number;
> not sure why this works. I also think that “offsets” is a confusing name for
> this. Maybe selectionCodeUnitCount?

Oh oops, that's because end == start in my test case. Let me add another test case there...

> > Source/WebCore/html/TextFieldInputType.cpp:390
> > +        selectionLength = selectedOffsets ? numGraphemeClusters(innerText.substring(selectionStart, selectedOffsets)) : 0;
> 
> Would be nice, some day, to have this use StringView::substring instead of
> String::substring.

Indeed.
Comment 5 Benjamin Poulain 2014-12-10 12:41:39 PST
Comment on attachment 242939 [details]
Addressed Darin's comments

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

I like both tests.

> Source/WebCore/html/TextFieldInputType.cpp:389
> +        int selectionCodeUnitCount = element().selectionEnd() - selectionStart;

selectionCodeUnitCount is a weird name.
Comment 6 Ryosuke Niwa 2014-12-10 13:20:48 PST
Created attachment 243062 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2014-12-10 14:09:48 PST
Comment on attachment 243062 [details]
Patch for landing

Clearing flags on attachment: 243062

Committed r177098: <http://trac.webkit.org/changeset/177098>
Comment 8 WebKit Commit Bot 2014-12-10 14:09:56 PST
All reviewed patches have been landed.  Closing bug.