RESOLVED FIXED Bug 31247
text-indent is unexpectedly applied to <ruby> 
https://bugs.webkit.org/show_bug.cgi?id=31247
Summary text-indent is unexpectedly applied to <ruby>
Kent Tamura
Reported 2009-11-08 19:11:32 PST
Created attachment 42726 [details] Example HTML Please see the attachments. There should be no space at the left of the <ruby> elements.
Attachments
Example HTML (257 bytes, text/html)
2009-11-08 19:11 PST, Kent Tamura 		no flags
Details
Rendering result with the latest WebKit + Safari (10.63 KB, image/png)
2009-11-08 19:12 PST, Kent Tamura 		no flags
Details
patch - reset 'text-indent' on <ruby> and <rt> elements (38.36 KB, patch)
2009-11-18 01:20 PST, Roland Steiner 		no flags
DetailsFormatted DiffDiff
patch - reset 'text-indent' on <ruby> and <rt> elements, change layout test to use dumpAsText() (3.87 KB, patch)
2009-11-18 20:47 PST, Roland Steiner 		darin: review+
commit-queue: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kent Tamura
Comment 1 2009-11-08 19:12:19 PST
Created attachment 42727 [details] Rendering result with the latest WebKit + Safari
Roland Steiner
Comment 2 2009-11-18 01:20:01 PST
Created attachment 43412 [details] patch - reset 'text-indent' on <ruby> and <rt> elements The ruby uses block objects to align the ruby text with its base: RenderRubyRun (anonymous, wrapping a base and a text), RenderRubyBase (anonymous, containing the base text) and RenderRubyText (<rt>, containing the annotation text). RenderRuby, corresponding to the <ruby> element is an inline renderer. However, <ruby> is just a means to add annotations to a base text and thus not seen to open a block. Therefore, text-indent should not apply. Thus, the patch changes the default UA style-sheet to reset 'text-indent' to 0. I was considering whether a hard-coded approach to take out the "block-ness" wouldn't be better, but at least for now I went with the KISS principle. There is also the question whether we we need to consider other CSS constructs that may have similar effects, such as :first-line (?). However, as a pseudo-element this needs a selector subject, so AFAICT you'd need some contrived CSS though for those to effect ruby in an non-intended way. Therefore I left the patch with only addressing text-indent for the time being.
Eric Seidel (no email)
Comment 3 2009-11-18 15:51:34 PST
Comment on attachment 43412 [details] patch - reset 'text-indent' on <ruby> and <rt> elements Can't this be a dumpAsText test? Since all we need to do is check the getComputedStyle(element).textIndent value? Also, we don't need this extra stuff at the top of the html file: +<head> +<style> +</style> +</head> +<body> This will cause non-mac platforms to fail since it only has mac results. That's OK, as we can update them after commit, but it would be better if we could just make this dumpAsText and avoid the trouble all together.
Roland Steiner
Comment 4 2009-11-18 20:47:23 PST
Created attachment 43480 [details] patch - reset 'text-indent' on <ruby> and <rt> elements, change layout test to use dumpAsText() I guess I'm just more of a visual person... But you're right of course that a full pixel test isn't really necessary here - changed the layout test to use dumpAsText().
WebKit Commit Bot
Comment 5 2009-11-25 00:08:52 PST
Comment on attachment 43480 [details] patch - reset 'text-indent' on <ruby> and <rt> elements, change layout test to use dumpAsText() Rejecting patch 43480 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Darin Adler', '--force']" exit_code: 1 patching file WebCore/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebCore/css/html.css Hunk #1 FAILED at 609. 1 out of 1 hunk FAILED -- saving rejects to file WebCore/css/html.css.rej patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/fast/ruby/ruby-text-indent-expected.txt patching file LayoutTests/fast/ruby/ruby-text-indent.html
Roland Steiner
Comment 6 2009-11-25 01:21:10 PST
(In reply to comment #5) Oops, sorry - I had already committed the patch.
Kent Tamura
Comment 7 2009-11-25 01:40:05 PST
Oh, I see. It's r51227.
Note You need to log in before you can comment on or make changes to this bug.