Bug 63927

Summary: [META] REGRESSION(r72052): Multiple placeholder regressions
Product: WebKit Reporter: Kent Tamura <tkent>
Component: FormsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: adele, darin, dglazkov, playmobil
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 49452, 51290, 53740, 54797, 54814, 63367    
Bug Blocks:    

Description Kent Tamura 2011-07-05 02:25:16 PDT
[META] REGRESSION(r72052): Multiple placeholder regressions
Comment 1 Kent Tamura 2011-07-05 20:19:02 PDT
A)
I tried to represent a placeholder text as a shadow node, and replace the inner editable node with the placeholder node.  The code change for this approach was really simple.
However I had no good idea for implementing Bug 53740 behavior.
 - How to show the text caret with the placeholder text?
 - How to handle context menu on the placeholder node?

B)
I tried to implement another approach. Representing a placeholder text as a shadow node, it has display:none if the control has non-empty value, and show the placeholder node if the placeholder should be visible.
This approach requires to add a lot of custom layout code in RenderTextControlSingleLine.cpp. I don't want to make it more complex ;-(

C)
Fixing text-align and line-height issues in the current implementation is not hard. I guess fixing the wrapping issue, Bug 51290, would be hard and boring.
Comment 2 Kent Tamura 2011-07-05 22:12:12 PDT
D)
Revting r72052 won't work well for Bug 53740.
It would be hard to support editing context menu.
Comment 3 Dimitri Glazkov (Google) 2011-07-06 08:24:41 PDT
(In reply to comment #1)

I think A is the right approach.

> A)
> I tried to represent a placeholder text as a shadow node, and replace the inner editable node with the placeholder node.  The code change for this approach was really simple.
> However I had no good idea for implementing Bug 53740 behavior.
>  - How to show the text caret with the placeholder text?
>  - How to handle context menu on the placeholder node?

Can we position placeholder under the text input? Would this solve the issue?
Comment 4 Kent Tamura 2011-07-08 00:57:06 PDT
(In reply to comment #3)
> (In reply to comment #1)
> 
> I think A is the right approach.
> 
> > A)
> > I tried to represent a placeholder text as a shadow node, and replace the inner editable node with the placeholder node.  The code change for this approach was really simple.
> > However I had no good idea for implementing Bug 53740 behavior.
> >  - How to show the text caret with the placeholder text?
> >  - How to handle context menu on the placeholder node?
> 
> Can we position placeholder under the text input? Would this solve the issue?

In A, the inner editable text node is detached while the placeholder is visible.  So we can't do anything for editing.

I'm trying B again.  If we use position:absolute for the placeholder node, it's difficult to keep the correct position when the <input> is moved. If we use position:relative/static, <input> secures the intrinsic block for the placeholder node.  I found a way to resolve this issue and minimize custom layout code.
Comment 5 Kent Tamura 2011-07-11 00:28:10 PDT
Ok, I confirmed B resolved all of issues and did not break existing tests.
I'll post a patch to Bug 64253.
Comment 6 Dimitri Glazkov (Google) 2011-07-11 10:33:07 PDT
(In reply to comment #5)
> Ok, I confirmed B resolved all of issues and did not break existing tests.
> I'll post a patch to Bug 64253.

Great to hear. In retrospect, it does look like the sanest approach.
Comment 7 Kent Tamura 2011-07-13 19:19:17 PDT
All dependent bugs have been fixed.