Summary: | Introduce an internal feature for a fixed placeholder | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||
Component: | Forms | Assignee: | Kent Tamura <tkent> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | haraken, morrita, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 83872 | ||||||||||||
Attachments: |
|
Description
Kent Tamura
2012-04-15 23:29:49 PDT
Created attachment 137282 [details]
Patch
Comment on attachment 137282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137282&action=review > Source/WebCore/html/InputType.h:243 > + // If supportsPlaceholder() && defaultPlaceholder().isEmpty(), it means a > + // type supports the 'placeholder' attribute. > + // If supportsPlaceholder() && !defaultPlaceholder().isEmpty(), it means a > + // type doesn't support the 'placeholder' attribute, but shows > + // defaultPlaceholder() string as a placeholder. Nit: It might be better to put this comment above supportsPlaceholder(). > Source/WebCore/html/TextFieldInputType.cpp:397 > + if (placeholderText.isEmpty()) Shouldn't this be 'if (!placeholderText.isEmpty())'? Comment on attachment 137282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137282&action=review >> Source/WebCore/html/InputType.h:243 >> + // defaultPlaceholder() string as a placeholder. > > Nit: It might be better to put this comment above supportsPlaceholder(). It's reasonable. I'll do. >> Source/WebCore/html/TextFieldInputType.cpp:397 >> + if (placeholderText.isEmpty()) > > Shouldn't this be 'if (!placeholderText.isEmpty())'? No. If defaultPlaceholder() is empty, we'll use the 'placeholder' attribute value. Created attachment 137302 [details]
Patch 2
Fix a comment
Created attachment 137303 [details]
Patch 3
Comment on attachment 137282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137282&action=review >>> Source/WebCore/html/TextFieldInputType.cpp:397 >>> + if (placeholderText.isEmpty()) >> >> Shouldn't this be 'if (!placeholderText.isEmpty())'? > > No. If defaultPlaceholder() is empty, we'll use the 'placeholder' attribute value. Ah, got it. I'd been confused. > Source/WebCore/html/TextFieldInputType.cpp:398 > + placeholderText = element()->strippedPlaceholder(); Nit: You can move this statement inside the below if statement. (In reply to comment #6) > > Source/WebCore/html/TextFieldInputType.cpp:398 > > + placeholderText = element()->strippedPlaceholder(); > > Nit: You can move this statement inside the below if statement. Sorry, ignore this comment. You code is right. Created attachment 137486 [details]
Patch 4
Add InputType::usesFixedPlaceholder()
Comment on attachment 137486 [details] Patch 4 Clearing flags on attachment: 137486 Committed r114360: <http://trac.webkit.org/changeset/114360> All reviewed patches have been landed. Closing bug. |