Bug 67408 - REGRESSION(r90971): Placeholder text of input control is rendered over drop down menu on hrblock.com
Summary: REGRESSION(r90971): Placeholder text of input control is rendered over drop d...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Kent Tamura
URL:
Keywords: InRadar, Regression
Depends on: 64253 71779
Blocks: 72352
  Show dependency treegraph
 
Reported: 2011-09-01 08:08 PDT by Carlos Garcia Campos
Modified: 2012-09-19 14:03 PDT (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-09-01 08:08:24 PDT
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 Jon Lee 2011-09-01 15:08:14 PDT
I am not seeing this on Safari 5.1 or on Chrome 13. What browser were you seeing the bug on?
Comment 2 Alexey Proskuryakov 2011-09-01 15:09:56 PDT
It's a regression from 5.1, so I'm seeing this on ToT.
Comment 3 Kent Tamura 2011-09-01 18:04:37 PDT
I guess this was caused by http://trac.webkit.org/changeset/90971
I'll take a look.
Comment 4 Kent Tamura 2011-09-01 21:37:32 PDT
Created attachment 106090 [details]
Reduction
Comment 5 Kent Tamura 2011-09-01 22:00:36 PDT
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 Kent Tamura 2011-09-01 23:46:49 PDT
Created attachment 106099 [details]
An idea to fix
Comment 7 Dimitri Glazkov (Google) 2011-09-02 07:11:40 PDT
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.
Comment 8 Dimitri Glazkov (Google) 2011-09-02 07:14:13 PDT
(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 Kent Tamura 2011-09-02 07:27:20 PDT
(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 Simon Fraser (smfr) 2011-09-02 08:32:49 PDT
I don't like the fix. Can't we ensure that the shadow DOM renderers make a stacking context?
Comment 11 Kent Tamura 2011-09-05 02:24:46 PDT
(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 Kent Tamura 2011-10-03 23:09:23 PDT
Created attachment 109582 [details]
Another idea to fix

Introduce another RenderView list
Comment 13 Dave Hyatt 2011-10-03 23:47:26 PDT
(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.
Comment 14 Dave Hyatt 2011-10-03 23:50:25 PDT
You really should not be creating position:relative objects inside input controls.
Comment 15 Kent Tamura 2011-10-04 00:04:54 PDT
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 Dave Hyatt 2011-10-04 09:32:20 PDT
(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 Dave Hyatt 2011-10-04 10:00:09 PDT
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 Radar WebKit Bug Importer 2011-10-04 10:11:31 PDT
<rdar://problem/10231218>
Comment 19 Kent Tamura 2011-10-05 01:09:52 PDT
(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 Kent Tamura 2011-10-26 21:47:27 PDT
(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 Kent Tamura 2011-10-26 22:05:57 PDT
Created attachment 112639 [details]
Patch (sorting hack)
Comment 22 Dave Hyatt 2011-10-27 00:53:18 PDT
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.
Comment 23 Dave Hyatt 2011-10-27 00:55:44 PDT
I'd suggest disabling placeholders until they can be implemented correctly.
Comment 24 Dave Hyatt 2011-10-27 01:01:25 PDT
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 Kent Tamura 2011-10-27 01:06:33 PDT
(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 Dave Hyatt 2011-10-27 01:13:08 PDT
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 Kent Tamura 2011-10-27 01:25:48 PDT
(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 Dave Hyatt 2011-10-27 08:43:18 PDT
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 Kent Tamura 2011-11-04 01:52:02 PDT
(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 Kent Tamura 2011-11-30 00:32:20 PST
Created attachment 117137 [details]
Patch (just remove position:relative)
Comment 31 WebKit Review Bot 2011-11-30 07:46:35 PST
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
Comment 32 Kent Tamura 2011-12-01 18:18:56 PST
Created attachment 117538 [details]
Patch (rebase)
Comment 33 WebKit Review Bot 2011-12-01 22:23:23 PST
Comment on attachment 117538 [details]
Patch (rebase)

Clearing flags on attachment: 117538

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