Bug 67408 – REGRESSION(r90971): Placeholder text of input control is rendered over drop down menu on hrblock.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Carlos Garcia Campos]

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.

[Jon Lee]

I am not seeing this on Safari 5.1 or on Chrome 13. What browser were you seeing the bug on?

[Alexey Proskuryakov]

It's a regression from 5.1, so I'm seeing this on ToT.

[Kent Tamura]

I guess this was caused by http://trac.webkit.org/changeset/90971
I'll take a look.

[Kent Tamura]

Created attachment 106090 [details]
Reduction

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 106099 [details]
An idea to fix

[Dimitri Glazkov (Google)]

Comment on attachment 106099 [details]
An idea to fix

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.

[Dimitri Glazkov (Google)]

(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

[Kent Tamura]

(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.

[Simon Fraser (smfr)]

I don't like the fix. Can't we ensure that the shadow DOM renderers make a stacking context?

[Kent Tamura]

(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?

[Kent Tamura]

Created attachment 109582 [details]
Another idea to fix

Introduce another RenderView list

[Dave Hyatt]

(In reply to comment #12)
> Created an attachment (id=109582) [details]
> Another idea to fix
> 
> Introduce another RenderView list

Yeah, ugh, no. You should not need to introduce a whole new RenderLayer child list.

[Dave Hyatt]

You really should not be creating position:relative objects inside input controls.

[Kent Tamura]

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?

[Dave Hyatt]

(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.

[Dave Hyatt]

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.

[Radar WebKit Bug Importer]

<rdar://problem/10231218>

[Kent Tamura]

(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.

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 112639 [details]
Patch (sorting hack)

[Dave Hyatt]

Comment on attachment 112639 [details]
Patch (sorting hack)

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.

[Dave Hyatt]

I'd suggest disabling placeholders until they can be implemented correctly.

[Dave Hyatt]

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?

[Kent Tamura]

(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.

[Dave Hyatt]

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.

[Kent Tamura]

(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

[Dave Hyatt]

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?

[Kent Tamura]

(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.

[Kent Tamura]

Created attachment 117137 [details]
Patch (just remove position:relative)

[WebKit Review Bot]

Comment on attachment 117137 [details]
Patch (just remove position:relative)

Attachment 117137 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10704172

[Kent Tamura]

Created attachment 117538 [details]
Patch (rebase)

[WebKit Review Bot]

Comment on attachment 117538 [details]
Patch (rebase)

Clearing flags on attachment: 117538

Committed r101742: <http://trac.webkit.org/changeset/101742>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Placeholder text still does not render immediately: bug 97133