Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(625)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1280423002: CSS4: Implement :placeholder-shown pseudo class (Closed)

Created:
5 years, 4 months ago by ramya.v
Modified:
5 years, 3 months ago
CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-html_chromium.org, blink-reviews-rendering, caseq+blink_chromium.org, devtools-reviews_chromium.org, dglazkov+blink, eae+blinkwatch, jchaffraix+rendering, kozyatinskiy+blink_chromium.org, leviw+renderwatch, lushnikov+blink_chromium.org, pdr+renderingwatchlist_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, szager+layoutwatch_chromium.org, yurys+blink_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

CSS4: Implement :placeholder-shown pseudo class The changes include 1) Refactoring HTMLTextFormControlElement to have an explicit visible placeholder state. 2) Adding support for :placeholder-shown pseudo class Taken reference from webkit: trac.webkit.org/changeset/172826 BUG=451120 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201702

Patch Set 1 #

Patch Set 2 : Rebased #

Patch Set 3 : updated the case when placeholder element doesnot exist #

Patch Set 4 : Updated testcase #

Total comments: 34

Patch Set 5 : Updated as per review comments #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+727 lines, -25 lines) Patch
M LayoutTests/fast/css/css-selector-text.html View 1 chunk +2 lines, -0 lines 0 comments Download
M LayoutTests/fast/css/css-selector-text-expected.txt View 1 chunk +2 lines, -0 lines 0 comments Download
M LayoutTests/fast/css/css-set-selector-text.html View 1 chunk +2 lines, -0 lines 0 comments Download
M LayoutTests/fast/css/css-set-selector-text-expected.txt View 1 chunk +2 lines, -0 lines 0 comments Download
A LayoutTests/fast/css/placeholder-shown-basics.html View 1 2 3 4 1 chunk +107 lines, -0 lines 2 comments Download
A LayoutTests/fast/css/placeholder-shown-basics-expected.html View 1 2 3 4 1 chunk +106 lines, -0 lines 2 comments Download
A LayoutTests/fast/selectors/placeholder-shown-sibling-style-update.html View 1 2 3 4 1 chunk +46 lines, -0 lines 0 comments Download
A LayoutTests/fast/selectors/placeholder-shown-sibling-style-update-expected.txt View 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/fast/selectors/placeholder-shown-style-update.html View 1 2 3 4 1 chunk +90 lines, -0 lines 0 comments Download
A LayoutTests/fast/selectors/placeholder-shown-style-update-expected.txt View 1 2 3 1 chunk +64 lines, -0 lines 0 comments Download
A LayoutTests/fast/selectors/placeholder-shown-with-input-basics.html View 1 2 3 4 1 chunk +65 lines, -0 lines 0 comments Download
A LayoutTests/fast/selectors/placeholder-shown-with-input-basics-expected.txt View 1 chunk +39 lines, -0 lines 0 comments Download
A LayoutTests/fast/selectors/placeholder-shown-with-textarea-basics.html View 1 2 3 4 1 chunk +70 lines, -0 lines 0 comments Download
A LayoutTests/fast/selectors/placeholder-shown-with-textarea-basics-expected.txt View 1 chunk +42 lines, -0 lines 0 comments Download
M Source/core/css/CSSSelector.h View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/css/CSSSelector.cpp View 1 3 chunks +3 lines, -0 lines 0 comments Download
M Source/core/css/RuleFeature.cpp View 2 chunks +2 lines, -0 lines 0 comments Download
M Source/core/css/SelectorChecker.cpp View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M Source/core/css/resolver/SharedStyleFinder.cpp View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M Source/core/html/HTMLInputElement.h View 1 2 3 4 2 chunks +5 lines, -0 lines 0 comments Download
M Source/core/html/HTMLInputElement.cpp View 1 2 3 4 3 chunks +7 lines, -1 line 0 comments Download
M Source/core/html/HTMLTextAreaElement.h View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/html/HTMLTextAreaElement.cpp View 1 2 3 4 7 chunks +11 lines, -4 lines 0 comments Download
M Source/core/html/HTMLTextFormControlElement.h View 1 2 3 4 2 chunks +5 lines, -2 lines 0 comments Download
M Source/core/html/HTMLTextFormControlElement.cpp View 1 2 3 4 5 chunks +16 lines, -12 lines 0 comments Download
M Source/core/html/forms/TextFieldInputType.cpp View 4 chunks +4 lines, -3 lines 0 comments Download
M Source/core/inspector/InspectorTraceEvents.cpp View 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/layout/LayoutTextControl.cpp View 1 1 chunk +1 line, -1 line 0 comments Download
M Source/core/testing/Internals.cpp View 1 2 3 4 1 chunk +5 lines, -2 lines 0 comments Download

Messages

Total messages: 18 (5 generated)
ramya.v
PTAL! Thanks
5 years, 4 months ago (2015-08-13 07:47:17 UTC) #2
ramya.v
On 2015/08/13 07:47:17, ramya.v wrote: > PTAL! > > Thanks Friendly ping.. PTAL! Thanks
5 years, 4 months ago (2015-08-17 05:22:41 UTC) #3
ramya.v
On 2015/08/17 05:22:41, ramya.v wrote: > On 2015/08/13 07:47:17, ramya.v wrote: > > PTAL! > ...
5 years, 4 months ago (2015-08-20 03:55:12 UTC) #5
rwlbuis
On 2015/08/20 03:55:12, ramya.v wrote: > On 2015/08/17 05:22:41, ramya.v wrote: > > On 2015/08/13 ...
5 years, 4 months ago (2015-08-20 15:12:39 UTC) #6
alancutter (OOO until 2018)
Not familiar enough with selectors to review the code I'm afraid. https://codereview.chromium.org/1280423002/diff/60001/LayoutTests/fast/css/placeholder-shown-basics.html File LayoutTests/fast/css/placeholder-shown-basics.html (right): ...
5 years, 4 months ago (2015-08-20 23:36:46 UTC) #7
esprehn
This is basically copy pasta of Benjamin Poulain's patch which isn't quite right for blink, ...
5 years, 4 months ago (2015-08-21 10:18:11 UTC) #9
ramya.v
Thank all for reviewing the patch. Updated the patch and comments. PTAL! Thanks https://codereview.chromium.org/1280423002/diff/60001/LayoutTests/fast/css/placeholder-shown-basics-expected.html File ...
5 years, 3 months ago (2015-08-25 10:24:45 UTC) #10
ramya.v
On 2015/08/25 10:24:45, ramya.v wrote: > Thank all for reviewing the patch. > Updated the ...
5 years, 3 months ago (2015-09-03 03:11:42 UTC) #11
esprehn
lgtm
5 years, 3 months ago (2015-09-03 04:36:18 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1280423002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1280423002/80001
5 years, 3 months ago (2015-09-03 04:44:13 UTC) #14
commit-bot: I haz the power
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201702
5 years, 3 months ago (2015-09-03 05:54:21 UTC) #15
tkent
https://codereview.chromium.org/1280423002/diff/80001/LayoutTests/fast/css/placeholder-shown-basics-expected.html File LayoutTests/fast/css/placeholder-shown-basics-expected.html (right): https://codereview.chromium.org/1280423002/diff/80001/LayoutTests/fast/css/placeholder-shown-basics-expected.html#newcode66 LayoutTests/fast/css/placeholder-shown-basics-expected.html:66: <input type="datetime"> Ditto https://codereview.chromium.org/1280423002/diff/80001/LayoutTests/fast/css/placeholder-shown-basics.html File LayoutTests/fast/css/placeholder-shown-basics.html (right): https://codereview.chromium.org/1280423002/diff/80001/LayoutTests/fast/css/placeholder-shown-basics.html#newcode66 LayoutTests/fast/css/placeholder-shown-basics.html:66: ...
5 years, 3 months ago (2015-09-08 03:12:05 UTC) #17
ramya.v
5 years, 3 months ago (2015-09-08 03:58:29 UTC) #18
Message was sent while issue was closed. 
Since the datetime not implemented it is considered as type text and showing
placeholder. Modified the testcase to type date which will ignore placeholder
attribute.
Added changes in https://codereview.chromium.org/1332543002/
Please review.

@tkent thanks for correcting.
Will take care next time to take the most relevant changes to blink while taking
patches from Webkit.

https://codereview.chromium.org/1280423002/diff/80001/LayoutTests/fast/css/pl...
File LayoutTests/fast/css/placeholder-shown-basics-expected.html (right):

https://codereview.chromium.org/1280423002/diff/80001/LayoutTests/fast/css/pl...
LayoutTests/fast/css/placeholder-shown-basics-expected.html:66: <input
type="datetime">
On 2015/09/08 03:12:04, tkent wrote:
> Ditto

Incorporated the changes in https://codereview.chromium.org/1332543002/

https://codereview.chromium.org/1280423002/diff/80001/LayoutTests/fast/css/pl...
File LayoutTests/fast/css/placeholder-shown-basics.html (right):

https://codereview.chromium.org/1280423002/diff/80001/LayoutTests/fast/css/pl...
LayoutTests/fast/css/placeholder-shown-basics.html:66: <input type="datetime">
On 2015/09/08 03:12:04, tkent wrote:
> It's very strange that type=datetime, which Blink has no implementation, is
> tested, and other supported types such as type=date is not tested.

Incorporated the changes in https://codereview.chromium.org/1332543002/

Powered by Google App Engine
This is Rietveld 408576698