Bug 111826

Summary:

RenderTextControlSingleLine should not assume that its text element has a renderer

Product:

WebKit

Reporter:

Dominic Cooney <dominicc>

Component:

Layout and Rendering

Assignee:

Dominic Cooney <dominicc>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, eric, esprehn+autocc, inferno, leviw, mifenton, ojan.autocc, ojan, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none

Description Dominic Cooney 2013-03-07 23:32:54 PST

RenderTextControlSingleLine dereferences innerTextElement()->renderBox() during layout, hit testing and scrolling. However that element may not have a renderer, for example if ::-webkit-textfield-decoration-controller is used to set it to display: none;.

Comment 1 Dominic Cooney 2013-03-07 23:35:24 PST

Created attachment 192161 [details]
Patch

Comment 2 Ojan Vafai 2013-03-08 10:09:36 PST

Comment on attachment 192161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192161&action=review

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:176
> +        ASSERT(innerTextRenderer);

Having a placeholderBox always means you'll also have a innerTextRenderer?

Comment 3 WebKit Review Bot 2013-03-08 10:14:46 PST

Comment on attachment 192161 [details]
Patch

Clearing flags on attachment: 192161

Committed r145239: <http://trac.webkit.org/changeset/145239>

Comment 4 WebKit Review Bot 2013-03-08 10:14:50 PST

All reviewed patches have been landed.  Closing bug.

Comment 5 Christian Biesinger 2013-03-13 14:26:29 PDT

Comment on attachment 192161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192161&action=review

> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:134
> +        if (innerTextRenderer)

this check is not necessary, because just above you added innerTextRenderer to the if...

Comment 6 Dominic Cooney 2013-03-14 22:53:35 PDT

(In reply to comment #2)
> Having a placeholderBox always means you'll also have a innerTextRenderer?

I thought so, but now digging into TextFieldInputType::createShadowDOM I am not so sure. I will investigate.

(In reply to comment #5)
> (From update of attachment 192161 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192161&action=review
> 
> > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:134
> > +        if (innerTextRenderer)
> 
> this check is not necessary, because just above you added innerTextRenderer to the if...

Oops. Code blindness. I filed bug 112406.

Comment 7 Dominic Cooney 2013-03-14 23:31:30 PDT

(In reply to comment #6)
> (In reply to comment #2)
> > Having a placeholderBox always means you'll also have a innerTextRenderer?
> 
> I thought so, but now digging into TextFieldInputType::createShadowDOM I am not so sure. I will investigate.

I have filed bug 112410 for this. I think HTMLTextFormControlElement::fixPlaceholderRenderer makes this assumption too, but the assumption is wrong.