Bug 96592 - Ruby text is incorrectly positioned when its writing-mode is changed to vertical after layout is done
Summary: Ruby text is incorrectly positioned when its writing-mode is changed to verti...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-12 19:35 PDT by Yuki Sekiguchi
Modified: 2013-03-11 18:14 PDT (History)
5 users (show)

See Also:


Attachments
reproduced content (228 bytes, text/html)
2012-09-12 19:35 PDT, Yuki Sekiguchi
no flags Details
reproduced content (358 bytes, text/html)
2012-09-12 19:38 PDT, Yuki Sekiguchi
no flags Details
Patch (4.66 KB, patch)
2012-09-12 19:46 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Fix test indications (5.29 KB, patch)
2012-10-31 22:57 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Fix test indications (5.77 KB, patch)
2012-11-05 23:57 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Rebase (5.72 KB, patch)
2012-12-10 20:31 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2013-01-07 19:10 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (5.71 KB, patch)
2013-03-06 22:38 PST, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (5.75 KB, patch)
2013-03-11 04:52 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuki Sekiguchi 2012-09-12 19:35:33 PDT
Created attachment 163762 [details]
reproduced content

Ruby text is incorrectly positioned when its writing-mode is changed to vertical after layout is done.

This bug is reproduced by the attached HTML content.

So the content don't apply any style to ruby text, the ruby text "b" should be at the center of ruby base "aaaaa".

This bug is reproduced by real content when the content is laid out without CSS and apply vertical CSS after that.
Comment 1 Yuki Sekiguchi 2012-09-12 19:38:14 PDT
Comment on attachment 163762 [details]
reproduced content

><!DOCTYPE html>
><html><body>
><style>
>   #i {border: solid 5px blue; -webkit-writing-mode: vertical-rl}
></style>
>ruby text should be center of ruby base.
><div id="i" >
><ruby>aaaaa<rt>b</rt></ruby>
></div>
></body></html>
Comment 2 Yuki Sekiguchi 2012-09-12 19:38:49 PDT
Created attachment 163763 [details]
reproduced content
Comment 3 Yuki Sekiguchi 2012-09-12 19:46:00 PDT
Created attachment 163764 [details]
Patch
Comment 4 Hajime Morrita 2012-10-30 03:22:47 PDT
Comment on attachment 163764 [details]
Patch

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.
Comment 5 Yuki Sekiguchi 2012-10-31 22:57:16 PDT
Created attachment 171772 [details]
Fix test indications
Comment 6 Yuki Sekiguchi 2012-10-31 23:02:23 PDT
Thank you for reviewing, Morrita-san!

(In reply to comment #4)
> > 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.

Removed.

> > LayoutTests/ChangeLog:8
> > +        Update LayoutTest to use testRunner not layoutTestController
> 
> This explanation doesn't looks making sense. I'd rather drop this.

Removed and add description of test case.

> > 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 .

Removed.

> > 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 ";"

Fixed.

> > 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?

Add test cases.

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

Fixed.

> > 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.

I understand it and fix it.
Comment 7 Hajime Morrita 2012-11-04 17:21:14 PST
Comment on attachment 171772 [details]
Fix test indications

View in context: https://bugs.webkit.org/attachment.cgi?id=171772&action=review

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

Although there is no JS style guide for WebKit project, we usually follows one of C++,
that means we does 4-space indent. See other tests as examples.

> LayoutTests/fast/writing-mode/ruby-text-logical-left.html:27
> +        var e = document.getElementById("test1");

Please don't use abbreviation for the variable name.
Comment 8 Yuki Sekiguchi 2012-11-05 23:57:49 PST
Created attachment 172496 [details]
Fix test indications
Comment 9 Yuki Sekiguchi 2012-12-10 20:31:06 PST
Created attachment 178699 [details]
Rebase
Comment 10 Yuki Sekiguchi 2013-01-07 19:10:29 PST
Created attachment 181622 [details]
Patch
Comment 11 Yuki Sekiguchi 2013-01-07 21:57:33 PST
(In reply to comment #7)
> (From update of attachment 171772 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171772&action=review
> 
> > LayoutTests/fast/writing-mode/ruby-text-logical-left.html:26
> > +      setTimeout(function () {
> 
> Although there is no JS style guide for WebKit project, we usually follows one of C++,
> that means we does 4-space indent. See other tests as examples.

I understand it and fixed.

> > LayoutTests/fast/writing-mode/ruby-text-logical-left.html:27
> > +        var e = document.getElementById("test1");
> 
> Please don't use abbreviation for the variable name.

Fixed.
Comment 12 Yuki Sekiguchi 2013-01-07 21:58:10 PST
Morrita-san, could you review this?
Comment 13 Yuki Sekiguchi 2013-03-06 22:38:03 PST
Created attachment 191920 [details]
Patch
Comment 14 Yuki Sekiguchi 2013-03-06 23:56:28 PST
Morrita-san, could you review this?
Comment 15 Hajime Morrita 2013-03-10 23:00:51 PDT
Comment on attachment 191920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191920&action=review

The code looks good. ChangeLog needs a bit polish.

> Source/WebCore/ChangeLog:8
> +        Fix logical left of RenderRubyText is not cleared.

s/Fix/Fixed/ - WebKit usually uses past tense here.

> Source/WebCore/ChangeLog:10
> +        This cause trouble when RenderRubyTest is laid out as vertical after as horizontal.

s/causes/
s/as vertical/vertically/
s/as horizontal/horizontally/

> Source/WebCore/ChangeLog:13
> +        RenderRubyText::y remain old one.

You should explain not only what the problem was, but also how you fixed it.

> LayoutTests/ChangeLog:8
> +        Add a test that change block flow direction of Ruby after layout.

You don't need any extra description in this case. This is kinda obvious.
Comment 16 Yuki Sekiguchi 2013-03-11 04:52:09 PDT
Created attachment 192441 [details]
Patch
Comment 17 Yuki Sekiguchi 2013-03-11 05:10:18 PDT
Hi Morrita-san,

Thank you for reviewing.

(In reply to comment #15)
> (From update of attachment 191920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191920&action=review

> > Source/WebCore/ChangeLog:8
> > +        Fix logical left of RenderRubyText is not cleared.
> 
> s/Fix/Fixed/ - WebKit usually uses past tense here.
> 
> > Source/WebCore/ChangeLog:10
> > +        This cause trouble when RenderRubyTest is laid out as vertical after as horizontal.
> 
> s/causes/
> s/as vertical/vertically/
> s/as horizontal/horizontally/

Fixed.

> > Source/WebCore/ChangeLog:13
> > +        RenderRubyText::y remain old one.
> 
> You should explain not only what the problem was, but also how you fixed it.

Added the description of how to fix.

> > LayoutTests/ChangeLog:8
> > +        Add a test that change block flow direction of Ruby after layout.
> 
> You don't need any extra description in this case. This is kinda obvious.

OK. Removed.
Comment 18 WebKit Review Bot 2013-03-11 18:14:25 PDT
Comment on attachment 192441 [details]
Patch

Clearing flags on attachment: 192441

Committed r145451: <http://trac.webkit.org/changeset/145451>
Comment 19 WebKit Review Bot 2013-03-11 18:14:30 PDT
All reviewed patches have been landed.  Closing bug.