Bug 61111

Summary:

RenderText with empty text is not created inside ShadowContentElement

Product:

WebKit

Reporter:

Hajime Morrita <morrita>

Component:

DOM

Assignee:

Hajime Morrita <morrita>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, tkent, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

61114

Bug Blocks:

56973

Attachments:

Description	Flags
A repro	none
Patch	none
Archive of layout-test-results from ec2-cr-linux-01	none
Patch	dglazkov: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-01	none

Hajime Morrita

Reported 2011-05-19 00:00:59 PDT

Created attachment 94045 [details] A repro This is because Text::renererIsNeeded() referes parentNode() and previousRenderer(), which should be based on content render tree, but is not at this time. Note that this missing RenderText problem is happens only after dynamics change of content element children.

Attachments
A repro (638 bytes, text/html) 2011-05-19 00:00 PDT, Hajime Morrita	no flags	Details
Patch (37.22 KB, patch) 2011-05-24 04:53 PDT, Hajime Morrita	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.26 MB, application/zip) 2011-05-24 06:07 PDT, WebKit Review Bot	no flags	Details
Patch (36.46 KB, patch) 2011-05-24 20:50 PDT, Hajime Morrita	dglazkov: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.28 MB, application/zip) 2011-05-24 22:42 PDT, WebKit Review Bot	no flags	Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Hajime Morrita

Comment 1 2011-05-24 04:53:37 PDT

Created attachment 94595 [details] Patch

WebKit Review Bot

Comment 2 2011-05-24 06:07:03 PDT

Comment on attachment 94595 [details] Patch Attachment 94595 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8730453 New failing tests: fast/forms/select-change-listbox-to-popup.html fast/forms/ValidityState-001.html fast/forms/input-file-re-render.html dom/html/level2/html/AppletsCollection.html svg/custom/getsvgdocument.html fast/forms/element-order.html fast/forms/preserveFormDuringResidualStyle.html fast/forms/input-type-change.html fast/forms/checkValidity-001.html fast/forms/input-type-change2.html tables/mozilla/bugs/bug113235-1.html fast/forms/select-change-listbox-size.html fast/forms/select-change-popup-to-listbox.html fast/forms/form-in-malformed-markup.html fast/forms/input-align-image.html fast/forms/input-number-change-type-on-focus.html fast/forms/input-align.html tables/mozilla/bugs/bug113235-2.html fast/forms/file-input-disabled.html fast/forms/formmove3.html

WebKit Review Bot

Comment 3 2011-05-24 06:07:08 PDT

Created attachment 94597 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Dimitri Glazkov (Google)

Comment 4 2011-05-24 08:13:06 PDT

Comment on attachment 94595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94595&action=review cr-linux is angry plus comments: > Source/WebCore/dom/NodeRenderingContext.cpp:56 > + // Doesn't resolve() yet. You can do it later by calling ensureResolved(). This comment is unnecessary. > Source/WebCore/dom/NodeRenderingContext.cpp:59 > +void NodeRenderingContext::resolve() determineLocation? > Source/WebCore/dom/NodeRenderingContext.h:66 > + LocationUnresolved, LocationUndetermined? > Source/WebCore/dom/NodeRenderingContext.h:102 > +inline void NodeRenderingContext::ensureResolved() usually this things have IfNeeded suffix, like resolveIfNeeded. > Source/WebCore/dom/Text.cpp:202 > + const_cast<NodeRenderingContext&>(context).ensureResolved(); This looks bad. Can we make the m_location mutable?

Hajime Morrita

Comment 5 2011-05-24 20:50:48 PDT

Created attachment 94738 [details] Patch

Hajime Morrita

Comment 6 2011-05-24 21:13:39 PDT

Hi Dimitri, thank you for reviewing! I updated the patch. After some digging, I found that we don't need resolve() phase. So I removed it and added an alternative way for updateRenderer() The point is that we can assume that the text node already has an renderer for it.

WebKit Review Bot

Comment 7 2011-05-24 22:42:11 PDT

Comment on attachment 94738 [details] Patch Attachment 94738 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8731621 New failing tests: fast/html/details-replace-text.html fast/html/details-replace-summary-child.html

WebKit Review Bot

Comment 8 2011-05-24 22:42:17 PDT

Created attachment 94745 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Dimitri Glazkov (Google)

Comment 9 2011-05-25 08:33:41 PDT

Comment on attachment 94738 [details] Patch Ok, but please look at the EWS test failures.

Hajime Morrita

Comment 10 2011-05-25 18:04:53 PDT

> (From update of attachment 94738 [details]) > Ok, but please look at the EWS test failures. Ops. It needs expectations. I mark it before landing. Thanks for the review!

Hajime Morrita

Comment 11 2011-05-25 19:32:07 PDT

Committed r87351: <http://trac.webkit.org/changeset/87351>

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