Bug 67408

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

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

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

Created attachment 106090 [details] Reduction

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.

Created attachment 106099 [details] An idea to fix

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.

(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

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

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

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

Created attachment 109582 [details] Another idea to fix Introduce another RenderView list

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

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

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?

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

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.

<rdar://problem/10231218>

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

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

Created attachment 112639 [details] Patch (sorting hack)

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.

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

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?

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

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.

(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

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?

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

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

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

Created attachment 117538 [details] Patch (rebase)

Comment on attachment 117538 [details] Patch (rebase) Clearing flags on attachment: 117538 Committed r101742: <http://trac.webkit.org/changeset/101742>

All reviewed patches have been landed. Closing bug.

Placeholder text still does not render immediately: bug 97133