Bug 96592

Summary:

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

Product:

WebKit

Reporter:

Yuki Sekiguchi <yuki.sekiguchi>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eric, esprehn+autocc, morrita, ojan.autocc, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
reproduced content	none
reproduced content	none
Patch	none
Fix test indications	none
Fix test indications	none
Rebase	none
Patch	none
Patch	none
Patch	none

Yuki Sekiguchi

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

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Yuki Sekiguchi

Comment 1 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>

Yuki Sekiguchi

Comment 2 2012-09-12 19:38:49 PDT

Created attachment 163763 [details] reproduced content

Yuki Sekiguchi

Comment 3 2012-09-12 19:46:00 PDT

Created attachment 163764 [details] Patch

Hajime Morrita

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

Yuki Sekiguchi

Comment 5 2012-10-31 22:57:16 PDT

Created attachment 171772 [details] Fix test indications

Yuki Sekiguchi

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

Hajime Morrita

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

Yuki Sekiguchi

Comment 8 2012-11-05 23:57:49 PST

Created attachment 172496 [details] Fix test indications

Yuki Sekiguchi

Comment 9 2012-12-10 20:31:06 PST

Created attachment 178699 [details] Rebase

Yuki Sekiguchi

Comment 10 2013-01-07 19:10:29 PST

Created attachment 181622 [details] Patch

Yuki Sekiguchi

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

Yuki Sekiguchi

Comment 12 2013-01-07 21:58:10 PST

Morrita-san, could you review this?

Yuki Sekiguchi

Comment 13 2013-03-06 22:38:03 PST

Created attachment 191920 [details] Patch

Yuki Sekiguchi

Comment 14 2013-03-06 23:56:28 PST

Morrita-san, could you review this?

Hajime Morrita

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

Yuki Sekiguchi

Comment 16 2013-03-11 04:52:09 PDT

Created attachment 192441 [details] Patch

Yuki Sekiguchi

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

WebKit Review Bot

Comment 18 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>

WebKit Review Bot

Comment 19 2013-03-11 18:14:30 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.