Bug 63927
| Summary: | [META] REGRESSION(r72052): Multiple placeholder regressions | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Kent Tamura <tkent> |
| Component: | Forms | Assignee: | 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: | |||
Kent Tamura
[META] REGRESSION(r72052): Multiple placeholder regressions
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Kent Tamura
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.
Kent Tamura
D)
Revting r72052 won't work well for Bug 53740.
It would be hard to support editing context menu.
Dimitri Glazkov (Google)
(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?
Kent Tamura
(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.
Kent Tamura
Ok, I confirmed B resolved all of issues and did not break existing tests.
I'll post a patch to Bug 64253.
Dimitri Glazkov (Google)
(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.
Kent Tamura
All dependent bugs have been fixed.