Bug 63927 - [META] REGRESSION(r72052): Multiple placeholder regressions
Summary: [META] REGRESSION(r72052): Multiple placeholder regressions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on: 49452 51290 53740 54797 54814 63367
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-05 02:25 PDT by Kent Tamura
Modified: 2011-07-13 19:19 PDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
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.