| Summary: | text-indent is unexpectedly applied to <ruby> | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||
| Component: | Layout and Rendering | Assignee: | Roland Steiner <rolandsteiner> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | ||||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 42727 [details]
Rendering result with the latest WebKit + Safari
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.
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.
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().
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
(In reply to comment #5) Oops, sorry - I had already committed the patch. |
Created attachment 42726 [details] Example HTML Please see the attachments. There should be no space at the left of the <ruby> elements.