Bug 53740

Summary: Allow platforms to specify if the placeholder should be visible when text controls are focused
Product: WebKit Reporter: Adele Peterson <adele>
Component: FormsAssignee: Adele Peterson <adele>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, darin, dglazkov, myakura.web, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 63927    
Attachments:
Description Flags
Patch
none
Patch mitz: review+

Description Adele Peterson 2011-02-03 17:07:36 PST
Allow platforms to specify if the placeholder should be visible when text controls are focused
Comment 1 Adele Peterson 2011-02-03 17:11:38 PST
Created attachment 81151 [details]
Patch
Comment 2 mitz 2011-02-03 17:19:11 PST
Comment on attachment 81151 [details]
Patch

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

> Source/WebCore/html/HTMLFormControlElement.cpp:586
>      return supportsPlaceholder()
>          && isEmptyValue()
> -        && document()->focusedNode() != this
> -        && !isPlaceholderEmpty();
> +        && !isPlaceholderEmpty()
> +        && document()->focusedNode() != this || (renderer() && renderer()->theme()->shouldShowPlaceholderWhenFocused());

I was really confused by this but finally determined that you meant for this last line to be one term in the chain of && conditions. However, right now it isn’t. I think you need parentheses around the last line.
Comment 3 Adele Peterson 2011-02-03 17:25:59 PST
Doh!  I thought I had worked it out right, and at the last minute removed those parenthesis.  I will re-add them…

(In reply to comment #2)
> (From update of attachment 81151 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81151&action=review
> 
> > Source/WebCore/html/HTMLFormControlElement.cpp:586
> >      return supportsPlaceholder()
> >          && isEmptyValue()
> > -        && document()->focusedNode() != this
> > -        && !isPlaceholderEmpty();
> > +        && !isPlaceholderEmpty()
> > +        && document()->focusedNode() != this || (renderer() && renderer()->theme()->shouldShowPlaceholderWhenFocused());
> 
> I was really confused by this but finally determined that you meant for this last line to be one term in the chain of && conditions. However, right now it isn’t. I think you need parentheses around the last line.
Comment 4 Adele Peterson 2011-02-03 17:54:33 PST
Created attachment 81161 [details]
Patch
Comment 5 mitz 2011-02-03 17:59:24 PST
Comment on attachment 81161 [details]
Patch

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

You are missing a LayoutTests/ChangeLog.

> Source/WebCore/html/HTMLTextAreaElement.cpp:280
> +    const_cast<HTMLTextAreaElement*>(this)->updatePlaceholderVisibility(false);

Is this not needed for text fields too?
Comment 6 Adele Peterson 2011-02-03 18:14:34 PST
Comment on attachment 81161 [details]
Patch

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

Thanks!

>> Source/WebCore/html/HTMLTextAreaElement.cpp:280
>> +    const_cast<HTMLTextAreaElement*>(this)->updatePlaceholderVisibility(false);
> 
> Is this not needed for text fields too?

Text fields update the value at a slightly different time, and the placeholder already gets updated at that time.
Comment 7 Adele Peterson 2011-02-04 10:19:45 PST
Committed revision 77639.
Comment 8 Dimitri Glazkov (Google) 2011-02-04 11:29:13 PST
Maybe we should do this too for Chromium. Kent-san, WDYT?
Comment 9 Kent Tamura 2011-02-05 01:48:11 PST
So, will Lion show placeholders for focused text fields?

I have received some requests for this new behavior.  But the HTML5 clearly specifies the current behavior. [1]
Should we ask to update the HTML5 specification so that the behavior depends on platforms?



[1] http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#the-placeholder-attribute
Comment 10 Masataka Yakura 2011-10-29 02:32:29 PDT
(In reply to comment #9)
> So, will Lion show placeholders for focused text fields?

LIon removes placeholder when the element got an input, not on focus.

> I have received some requests for this new behavior.  But the HTML5 clearly specifies the current behavior. [1]
> Should we ask to update the HTML5 specification so that the behavior depends on platforms?
> 
> 
> 
> [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#the-placeholder-attribute

I filed a bug asking for that and the spec changed today. So now you can answer to the requests without breaking spec conformance :-)
http://html5.org/r/6782