Allow platforms to specify if the placeholder should be visible when text controls are focused

[Adele Peterson]

Allow platforms to specify if the placeholder should be visible when text controls are focused

[Adele Peterson]

Created attachment 81151 [details]
Patch

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.

[Adele Peterson]

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.

[Adele Peterson]

Created attachment 81161 [details]
Patch

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?

[Adele Peterson]

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.

[Adele Peterson]

Committed revision 77639.

[Dimitri Glazkov (Google)]

Maybe we should do this too for Chromium. Kent-san, WDYT?

[Kent Tamura]

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

[Masataka Yakura]

(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