Bug 67408 - REGRESSION(r90971): Placeholder text of input control is rendered over drop down menu on hrblock.com
: REGRESSION(r90971): Placeholder text of input control is rendered over drop d...
Status: RESOLVED FIXED
: WebKit
Forms
: 528+ (Nightly build)
: All All
: P1 Normal
Assigned To:
:
: InRadar, Regression
: 64253 71779
: 72352
  Show dependency treegraph
 
Reported: 2011-09-01 08:08 PST by
Modified: 2012-09-19 14:03 PST (History)


Attachments
Reduction (182 bytes, text/html)
2011-09-01 21:37 PST, Kent Tamura
no flags Details
An idea to fix (1.22 KB, patch)
2011-09-01 23:46 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Another idea to fix (20.44 KB, patch)
2011-10-03 23:09 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch (sorting hack) (6.46 KB, patch)
2011-10-26 22:05 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch (just remove position:relative) (24.79 KB, patch)
2011-11-30 00:32 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff
Patch (rebase) (24.83 KB, patch)
2011-12-01 18:18 PST, Kent Tamura
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-09-01 08:08:24 PST
This can be reproduced in this website: http://www.hrblock.com/

 1. Move the mouse to TAX CALCULATIONS & TIPS 
 2. And now to 'Select your fill status' combo box
 3. A dropdown menu appears and the placeholder text (W-2 Box 1) of the text control behind is rendered over the menu

Not setting the shadow pseudo id -webkit-input-placeholder fixes the issue, but of course the placeholder text css attrs are not applied either.
------- Comment #1 From 2011-09-01 15:08:14 PST -------
I am not seeing this on Safari 5.1 or on Chrome 13. What browser were you seeing the bug on?
------- Comment #2 From 2011-09-01 15:09:56 PST -------
It's a regression from 5.1, so I'm seeing this on ToT.
------- Comment #3 From 2011-09-01 18:04:37 PST -------
I guess this was caused by http://trac.webkit.org/changeset/90971
I'll take a look.
------- Comment #4 From 2011-09-01 21:37:32 PST -------
Created an attachment (id=106090) [details]
Reduction
------- Comment #5 From 2011-09-01 22:00:36 PST -------
What's happening:
* The placeholder text is represented as a node with position:relative.
* The dropdown menu is represented as a node with position:absolute.
* The dropdown menu node is placed before the input node with the placeholder.
* The placeholder node and the dropdown node have no z-index, so WebKit paints them in the document order?
  If the order of the dropdown node and the input element with the placeholder was reversed, this bug would not happen.

IMO web authors should specify a higher z-index to a topmost widget like the dropdown menu, but they don't expect input elements contains position:relative nodes.
I'd like to fix this issue, but I have no idea how to fix this for now.
------- Comment #6 From 2011-09-01 23:46:49 PST -------
Created an attachment (id=106099) [details]
An idea to fix
------- Comment #7 From 2011-09-02 07:11:40 PST -------
(From update of attachment 106099 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=106099&action=review

> Source/WebCore/rendering/RenderLayer.cpp:3832
> +    if (firstNode && firstNode->isInShadowTree())

You can just compare treeScope() if they aren't equal, you're in shadow tree.
------- Comment #8 From 2011-09-02 07:14:13 PST -------
(In reply to comment #5)
> What's happening:
> * The placeholder text is represented as a node with position:relative.
> * The dropdown menu is represented as a node with position:absolute.
> * The dropdown menu node is placed before the input node with the placeholder.
> * The placeholder node and the dropdown node have no z-index, so WebKit paints them in the document order?
>   If the order of the dropdown node and the input element with the placeholder was reversed, this bug would not happen.
> 
> IMO web authors should specify a higher z-index to a topmost widget like the dropdown menu, but they don't expect input elements contains position:relative nodes.
> I'd like to fix this issue, but I have no idea how to fix this for now.

This also might be something we should fix generally in shadow DOM subtrees
------- Comment #9 From 2011-09-02 07:27:20 PST -------
(In reply to comment #8)
> This also might be something we should fix generally in shadow DOM subtrees

Yeah, <input> is a kind of a component and component's relative/absolute shadow nodes should not disturb other components.

My "An idea to fix" is still wrong. If a component would like to show a dropdown menu, it should be topmost.
------- Comment #10 From 2011-09-02 08:32:49 PST -------
I don't like the fix. Can't we ensure that the shadow DOM renderers make a stacking context?
------- Comment #11 From 2011-09-05 02:24:46 PST -------
(In reply to comment #10)
> I don't like the fix. Can't we ensure that the shadow DOM renderers make a stacking context?

Do you mean ensuring a renderer for the <input> or ShadowRoot of the <input> has a stacking context layer?


I had an expriment today.
- Added "requiresLayer() const { return true; }" to RenderTextControl.
- Changed isStackingContext() and stackingContext() so that they assumed RenderTextControl was a stacking context.

This fixed the placeholder issue for <textarea>, but didn't fix for <input> because
 - <textarea> was isNormalFlowOnly() and it was appended to m_normalFlowList, and it was painted before m_posZorderList.
 - A layer for <input> was added to m_posZorderList, and <input> was painted after the position:absolute <div>.

Should we add a new list for placeholder layers into RenderLayer?
------- Comment #12 From 2011-10-03 23:09:23 PST -------
Created an attachment (id=109582) [details]
Another idea to fix

Introduce another RenderView list
------- Comment #13 From 2011-10-03 23:47:26 PST -------
(In reply to comment #12)
> Created an attachment (id=109582) [details] [details]
> Another idea to fix
> 
> Introduce another RenderView list

Yeah, ugh, no. You should not need to introduce a whole new RenderLayer child list.
------- Comment #14 From 2011-10-03 23:50:25 PST -------
You really should not be creating position:relative objects inside input controls.
------- Comment #15 From 2011-10-04 00:04:54 PST -------
Dave, thank you for the comments.

(In reply to comment #14)
> You really should not be creating position:relative objects inside input controls.

We decided to use position:relative for placeholder in order to implement the behavior of Bug 53740. We needed to show an editable text and a placeholder at the same time.

Do you have an idea to implement placeholder without position:relative?
------- Comment #16 From 2011-10-04 09:32:20 PST -------
(In reply to comment #15)
> Dave, thank you for the comments.
> 
> (In reply to comment #14)
> > You really should not be creating position:relative objects inside input controls.
> 
> We decided to use position:relative for placeholder in order to implement the behavior of Bug 53740. We needed to show an editable text and a placeholder at the same time.
> 
> Do you have an idea to implement placeholder without position:relative?

Why do you need position:relative for that? I don't know much about what you're trying to do, but maybe you could use generated content like ::before to make the placeholder text? Editing can't see generated content.
------- Comment #17 From 2011-10-04 10:00:09 PST -------
Just to be clear, there's nothing about shadow trees that are special here. You've created a rendering and stacking that is behaving exactly as it should. You're thinking about the problem the wrong way if you think you need some kind of hacked code to "violate" that standard.

You need to be thinking about how you can implement the feature in accordance with Web standards and get the behavior you want. It may involve special casing editing code to ignore the placeholder text, which is infinitely more preferable than hacking core rendering code.
------- Comment #18 From 2011-10-04 10:11:31 PST -------
<rdar://problem/10231218>
------- Comment #19 From 2011-10-05 01:09:52 PST -------
(In reply to comment #16)
> > Do you have an idea to implement placeholder without position:relative?
> 
> Why do you need position:relative for that? I don't know much about what you're trying to do, but maybe you could use generated content like ::before to make the placeholder text? Editing can't see generated content.

:before or :after is a nice idea though we will need to do something when Bug 7562 is fixed.
------- Comment #20 From 2011-10-26 21:47:27 PST -------
(In reply to comment #19)
> > Why do you need position:relative for that? I don't know much about what you're trying to do, but maybe you could use generated content like ::before to make the placeholder text? Editing can't see generated content.
> 
> :before or :after is a nice idea though we will need to do something when Bug 7562 is fixed.

I implemented this way these days, and gave it up.
On Lion Safari and Windows Safari,
  <input style="text-align: center" placeholder=placeholder>
shows the caret and the placeholder text if the element has focus, and they are overlapped. I have no good idea to realize this overlapping behavior without a positioning node.

I'd like to proceed the first hack https://bugs.webkit.org/attachment.cgi?id=106099, and let's discuss how to improve the placeholder implementation later.
------- Comment #21 From 2011-10-26 22:05:57 PST -------
Created an attachment (id=112639) [details]
Patch (sorting hack)
------- Comment #22 From 2011-10-27 00:53:18 PST -------
(From update of attachment 112639 [details])
This is not acceptable. You need to eliminate the use of relative positioning. I'm not letting you hack up RenderLayer just because something else was implemented incorrectly. Go fix the other code. Don't pile on more incorrect code.
------- Comment #23 From 2011-10-27 00:55:44 PST -------
I'd suggest disabling placeholders until they can be implemented correctly.
------- Comment #24 From 2011-10-27 01:01:25 PST -------
Why is this a regression by the way? Did this work before somehow? If so, what was done before to make it work, and why is it broken now?
------- Comment #25 From 2011-10-27 01:06:33 PST -------
(In reply to comment #24)
> Why is this a regression by the way? Did this work before somehow? If so, what was done before to make it work, and why is it broken now?

r72052 made many regressions, and I tried to fix them by r90971.
We can't go back to the way of r72051 or earlier because of the behavior of Bug 53740.

There would be many ways to implement placeholders without positioned node if we removed the feature introduced by Bug 53740.  The feature is not compliant to the HTML standard.
------- Comment #26 From 2011-10-27 01:13:08 PST -------
I think fundamentally it is a mistake to try to represent the placeholder text as a generic shadow element. It's also more heavyweight. I would go back to simply painting the text in the background phase of the RenderTextControl.

You say it caused many regressions. Can you point to those bugs? I think it's much more lightweight to just do some custom painting of the placeholder text in C++ code and not try to express the placeholder text using a shadow tree.

Basically I like the approach that was taken in r72052 and would like to know more about what was wrong with it that led to swapping in a more heavyweight shadow tree implementation for placeholders.
------- Comment #27 From 2011-10-27 01:25:48 PST -------
(In reply to comment #26)
> I think fundamentally it is a mistake to try to represent the placeholder text as a generic shadow element. It's also more heavyweight. I would go back to simply painting the text in the background phase of the RenderTextControl.
> 
> You say it caused many regressions. Can you point to those bugs? I think it's much more lightweight to just do some custom painting of the placeholder text in C++ code and not try to express the placeholder text using a shadow tree.
> 
> Basically I like the approach that was taken in r72052 and would like to know more about what was wrong with it that led to swapping in a more heavyweight shadow tree implementation for placeholders.

At least,
Bug 51290, Bug 54797, Bug 54814, Bug 63367
------- Comment #28 From 2011-10-27 08:43:18 PST -------
So what about keeping the shadow tree element for the placeholder text, but don't make it position:relative. Then just hack the paint method of RenderTextControl to paint the kids in the order you want?
------- Comment #29 From 2011-11-04 01:52:02 PST -------
(In reply to comment #28)
> So what about keeping the shadow tree element for the placeholder text, but don't make it position:relative. Then just hack the paint method of RenderTextControl to paint the kids in the order you want?

Thanks for the advice.  I tried to make an idea how to lay out a placeholder node.  Then, I got unsure about the requirement of position:relative in the current implementation. I removed position:relative from html.css, and the current implementation seemed to work well for this bug and existing tests.
------- Comment #30 From 2011-11-30 00:32:20 PST -------
Created an attachment (id=117137) [details]
Patch (just remove position:relative)
------- Comment #31 From 2011-11-30 07:46:35 PST -------
(From update of attachment 117137 [details])
Attachment 117137 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10704172
------- Comment #32 From 2011-12-01 18:18:56 PST -------
Created an attachment (id=117538) [details]
Patch (rebase)
------- Comment #33 From 2011-12-01 22:23:23 PST -------
(From update of attachment 117538 [details])
Clearing flags on attachment: 117538

Committed r101742: <http://trac.webkit.org/changeset/101742>
------- Comment #34 From 2011-12-01 22:23:31 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #35 From 2012-09-19 14:03:33 PST -------
Placeholder text still does not render immediately: bug 97133