Summary: | Crash while processing ill-formed <textPath> ouside of <text> | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Cosmin Truta <ctruta> | ||||||||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, mdelaney7, rwlbuis, zimmermann | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | All | ||||||||||||||||
Bug Depends on: | 50211 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Cosmin Truta
2010-10-15 17:53:35 PDT
We've discussed this with Dirk & Rob, and I think what I described is more a workaround than a real fix. The question is: shall we create "invalid" DOMs at all, aka. those that violate the SVG "content model"? If we only ever create valid DOMs, we wouldn't need to add rendererIsNeeded methods, that compare tag names. In the past we had checkDTD/checkAddChild methods on the HTML* Elements that did that job, they're gone now replaced by the HTML5 TreeBuilder (IIRC). I'm CC'ing Adam & Eric for their insight, on how to solve this problem in a clean fashion. Created attachment 74958 [details]
Fix and layout test
There is one thing that isn't exactly done as stated in the content model spec:
SVGTextPathElement::childShouldCreateRenderer allows <textPath> inside <textPath>. (SVGTextPathElement::rendererIsNeeded is fine, however.)
This is to accommodate <textPath><a><textpath>Text</textPath></a></textPath>, which is tested in svg/custom/text-linking.svg
(In reply to comment #2) > Created an attachment (id=74958) [details] > Fix and layout test > > There is one thing that isn't exactly done as stated in the content model spec: > SVGTextPathElement::childShouldCreateRenderer allows <textPath> inside <textPath>. (SVGTextPathElement::rendererIsNeeded is fine, however.) > This is to accommodate <textPath><a><textpath>Text</textPath></a></textPath>, which is tested in svg/custom/text-linking.svg I guess the testcase is wrong, can you compare to Opera? Comment on attachment 74958 [details]
Fix and layout test
Looks good to me in general, can you verify that text-linking.svg is wrong, first and completely adhere to the defined content model?
Created attachment 75045 [details] Opera screenshot (In reply to comment #3) > > This is to accommodate <textPath><a><textpath>Text</textPath></a></textPath>, which is tested in svg/custom/text-linking.svg > > I guess the testcase is wrong, can you compare to Opera? I was puzzled about this myself, I thought I was reading the SVG spec incorrectly. Opera (both 10 release and 11 beta) shows "textPath in link in textPath", but Inkscape doesn't. Inkscape doesn't show any of the two textPaths. An Opera screenshot is attached. A bunch of things seem to broken in Opera. Inkscape shows the following (just straight lines, no curved text paths): ** begin Inkscape output tspan in link tref in link Referenced character data AltGlyph content This should not be rendered tspan in link in tspan ** end Inkscape output (In reply to comment #5) > Created an attachment (id=75045) [details] > Opera screenshot > > (In reply to comment #3) > > > This is to accommodate <textPath><a><textpath>Text</textPath></a></textPath>, which is tested in svg/custom/text-linking.svg > > > > I guess the testcase is wrong, can you compare to Opera? > > I was puzzled about this myself, I thought I was reading the SVG spec incorrectly. > > Opera (both 10 release and 11 beta) shows "textPath in link in textPath", but Inkscape doesn't. Inkscape doesn't show any of the two textPaths. Hm, okay it's fine with me for now. Just go ahead and land the patch, we can still tweak the content model, if needed. Comment on attachment 74958 [details] Fix and layout test View in context: https://bugs.webkit.org/attachment.cgi?id=74958&action=review Can you modify the test, then I'll r+ it. > LayoutTests/svg/custom/invalid-text-content.svg:5 > + layoutTestController.dumpAsText(); Hm, how about removing dumpAsText() here and showing the actual render tree? Would be easier to track the renderer creation with it... (In reply to comment #7) > (From update of attachment 74958 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74958&action=review > > Can you modify the test, then I'll r+ it. > > > LayoutTests/svg/custom/invalid-text-content.svg:5 > > + layoutTestController.dumpAsText(); > > Hm, how about removing dumpAsText() here and showing the actual render tree? Would be easier to track the renderer creation with it... Just chatted with Rob Buis, the author of that test case, and he agreed that line is in error. I will fix the test case, by adding <textPath><a><text><textPath/></text></a></textPath>, as Rob suggested. I will also show the render tree, I agree it makes the renderer creation more tractable. Created attachment 75109 [details] Fix and layout test Actually, <text> inside <a> inside <textPath> shouldn't work either, because the spec says "The ‘a’ element may contain any element that its parent may contain, except itself", and <textPath> cannot contain <text>. In my interpretation of the content model, <textPath> cannot be inside another <textPath> in any way. That line from text-linking.svg must be removed. However, since I don't have the means to regenerate the expected image output of text-linking.svg, I will open a separate bug 50211. This will be committed after that one gets committed. Created attachment 75110 [details] Fix and layout test Oops... the previous patch accidentally included a modification to text-linking.svg. That should be done in bug 50211. Comment on attachment 75110 [details] Fix and layout test Setting cq?, given that the patches for bugs 50211, 50310 have finally landed. Comment on attachment 75110 [details]
Fix and layout test
Why does this test need pixel results? And even so, it could use the Ahem font to avoid needing custom platform results.
(In reply to comment #12) > (From update of attachment 75110 [details]) > Why does this test need pixel results? And even so, it could use the Ahem font to avoid needing custom platform results. I explicitely asked Cosmin to avoid dumpAsText() here, as we want to see the dump of the render tree, to know exactly which renderers are created. The side effect, is that this now needs pixel test results. I don't see how Ahem can help here, if Cosmin wants to display a real text on screen? We could also move the "pass" conditition in a comment, as recently suggested on webkit-dev. Move the pass to a comment, make it a green square and then the results can be shared between all platforms. :) Created attachment 76239 [details]
Fix and layout test
Followed Eric's suggestion.
Comment on attachment 76239 [details]
Fix and layout test
Assuming your test case still would crash on an unfixed webkit, then LGTM!
(In reply to comment #16) > (From update of attachment 76239 [details]) > Assuming your test case still would crash on an unfixed webkit, then LGTM! Yes, it still crashes without this patch. And even without the crash, if I carefully remove the sections that make it crash, I can see (the below-baseline/descent part of) the text "This should not be rendered", in the image capture, and in the dumped render tree. Created attachment 76244 [details]
Fix and layout test
Resubmitting with the layout test slightly changed:
"This should not be rendered." --> "This should not appear in rendering."
With the new test, if the text is inadvertently (incorrectly) rendered, the text descent (the lower part of 'p' in "appear" and the lower part of 'g' in "rendering") shows up as dirt in the rendered image, thus making the failure more obvious, even when it's not crashing.
Could you please redo the r+/cq+?
Comment on attachment 76244 [details] Fix and layout test Clearing flags on attachment: 76244 Committed r73820: <http://trac.webkit.org/changeset/73820> All reviewed patches have been landed. Closing bug. |