[Webkit-unassigned] [Bug 96592] Ruby text is incorrectly positioned when its writing-mode is changed to vertical after layout is done

Tue Oct 30 03:21:30 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=96592

Hajime Morrita <morrita at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #163764|review?                     |review-
               Flag|                            |

--- Comment #4 from Hajime Morrita <morrita at google.com>  2012-10-30 03:22:47 PST ---
(From update of attachment 163764)
View in context: https://bugs.webkit.org/attachment.cgi?id=163764&action=review

The change itself looks sane. Could you tidy a few points up?

> Source/WebCore/rendering/RenderRubyRun.cpp:238
> +    // Logical left of RenderRubyText should always be 0.

We don't need this line. The comment says something obvious from the code.

> LayoutTests/ChangeLog:8
> +        Update LayoutTest to use testRunner not layoutTestController

This explanation doesn't looks making sense. I'd rather drop this.

> LayoutTests/ChangeLog:15
> +        RenderRubyText::y remain old one.

We don't need to duplicate this. It's sufficient to have the explanation in  WebCore/ChangeLog .

> LayoutTests/fast/writing-mode/ruby-text-logical-left-expected.html:3
> +<style>

Please put <sytle> in <head> unless there is reason to do it.
Also, please make CSS syntax valid by adding ";"

> LayoutTests/fast/writing-mode/ruby-text-logical-left-expected.html:10
> +</body></html>

It would be great if you have more coverage.
- What happens if the style is removed?
- What if ruby has wider width than decorated text?

> LayoutTests/fast/writing-mode/ruby-text-logical-left.html:11
> +  setTimeout(function () {

Please indent the code appropriately.

> LayoutTests/fast/writing-mode/ruby-text-logical-left.html:14
> +  if (testRunner) {

Do check window.testRunner instead of just referring testRunner.
In that way we can run the test not only on DRT but also in a plain browser.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.