Bug 28420 – Implement HTML5 <ruby> rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Roland Steiner]

Implement proper rendering for <ruby>, <rt> and <rp> elements as per the HTML5 spec.

See http://www.whatwg.org/specs/web-apps/current-work/#the-ruby-element .

This bug is similar to https://bugs.webkit.org/show_bug.cgi?id=3749, but focuses on a HTML5-based implementation rather than a CSS3/XHTML-based implementation.

[Roland Steiner]

Created attachment 35020 [details]
patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej

Remove ENABLE_RUBY guards as discussed with Dave Hyatt and Maciej Stachowiak on IRC.

[Roland Steiner]

Created attachment 35033 [details]
patch 2: ruby functionality

Fundamentally the same patch as the last one submitted to https://bugs.webkit.org/show_bug.cgi?id=3749 .

See the descriptions there for discussions and caveats (brief summary: implements HTML5 ruby only, no CSS3/XHTML support, some minor known issues with selection and copy/paste).

[Roland Steiner]

Created attachment 35034 [details]
patch 3: layout tests for ruby

New layout tests for <ruby>. These test 2 main areas:

.) basic rendering
.) correctness of render tree after DOM manipulation

[Roland Steiner]

Created attachment 38526 [details]
patch 2: ruby functionality

Updated patch: first version attempted to be independent of other patches included here. However, this was not feasible and would have resulted in problems when committing. Also improved the change log entry.

Provided these patches are accepted, they should be committed in numbered order, i.e., "patch 1" first, then "patch 2", and "patch 3" last.

[Roland Steiner]

Rebaselined versions of the layout tests for Chromium-Win:

http://codereview.chromium.org/173347

[Eric Seidel (no email)]

Comment on attachment 35020 [details]
patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej

I'll trust you that they agreed to this.  This looks technically correct.

[Eric Seidel (no email)]

Comment on attachment 35020 [details]
patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej

Actually, removing cq+, since the commit-queue will close the bug when it lands this patch (despite the other r=? patches) due to a bug.

[Eric Seidel (no email)]

bug 28230 is the bug I was referring to.

[Dave Hyatt]

Comment on attachment 38526 [details]
patch 2: ruby functionality

Typo in RenderRubyAsInline:

"Note: ':after' content is handled implicitely below"

"implicitely" hould be "implicitly"

Everything else looks good.

r=me

[Yong Li]

Comment on attachment 35020 [details]
patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej

Let commit bot land it

[WebKit Commit Bot]

Comment on attachment 35020 [details]
patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej

Rejecting patch 35020 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=35020 from bug 28420 failed to download and apply.

[WebKit Commit Bot]

Comment on attachment 35034 [details]
patch 3: layout tests for ruby

Rejecting patch 35034 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11484 test cases.
fast/ruby/ruby-empty-rt.html -> failed

Exiting early after 1 failures. 7830 tests run.
141.56s total testing time

7829 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output

[WebKit Commit Bot]

Comment on attachment 38526 [details]
patch 2: ruby functionality

Rejecting patch 38526 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=38526 from bug 28420 failed to download and apply.

[Eric Seidel (no email)]

The patches depend on each other and probably need someone to land them manually.  The commit-queue isn't really designed for this type of multi-patch change.

[Roland Steiner]

(In reply to comment #14)
> The patches depend on each other and probably need someone to land them
> manually.  The commit-queue isn't really designed for this type of multi-patch
> change.

Thanks for handling this so far! I'll get my committer account for WebKit soon, so I should be able to land it myself then.

[Roland Steiner]

Landed patch 1.

[Roland Steiner]

Landed patch 2.

[Eric Seidel (no email)]

http://trac.webkit.org/changeset/50397 broke the windows build. We need to fix it or roll it out.

[Roland Steiner]

Fixed the windows build.

[Roland Steiner]

Created attachment 42475 [details]
patch 4 - re-baselined layout test results for Windows

Added new patch that contains the re-baselined results for the ruby layout tests for the Windows platform.

[Roland Steiner]

Landed patch 3

As it caused another layout test to fail, the ruby layout tests are currently added to the Skipped list for Mac.

After more investigation it turns out that the fault seems to lay with the single failing layout test, however.

[Eric Seidel (no email)]

*** Bug 3749 has been marked as a duplicate of this bug. ***

[Brian Weinstein]

Can we remove the tests from the Windows Skipped List now?

[Brian Weinstein]

I tried re-running these tests with the new Windows specific results, and they still failed on my machine. Any idea why that would be happening?

[Roland Steiner]

(In reply to comment #24)
> I tried re-running these tests with the new Windows specific results, and they
> still failed on my machine. Any idea why that would be happening?

Not really :p - I generated the re-baselined results on an XP laptop, but perhaps something in the setup wasn't quite correct. Is there a way to have these auto-generated instead?

[Roland Steiner]

Created attachment 43029 [details]
patch 5a - remove fast/ruby tests from Skipped list for Mac

After https://bugs.webkit.org/show_bug.cgi?id=31200 has been fixed, there is no reason to skip the ruby layout tests for Mac any longer.

[Eric Seidel (no email)]

Roland: Ping?

[Roland Steiner]

(In reply to comment #27)
> Roland: Ping?

Yes, sorry, I should have posted an update here too: I am currently re-writing the layout tests to use latin characters - see https://bugs.webkit.org/show_bug.cgi?id=31865.

I want to submit the baselines for both Mac and Windows there before re-enabling them, to avoid having some intermediate state between old and new versions.

[Eric Seidel (no email)]

Should we r- these patches then?  I'm unclear on what should be done with this bug.

[Roland Steiner]

(In reply to comment #29)
Sorry, I should have been more clear in my updates - patch 4 is indeed obsolete once I (finally) get to do the Windows baselines of the new Latin-only versions.

As for patch 5a, that's basically still valid, I just wanted to get the baselines in before enabling the layout tests, followed by a "patch 5b" to also enable them for Windows.

However, in order to avoid confusion, it may be easier to r- patch 5a here. I'll just re-add it to https://bugs.webkit.org/show_bug.cgi?id=31865 once I get around with the Windows baselines there (or open a new bug entry to enable both Mac and Windows layout tests).

[Roland Steiner]

Comment on attachment 42475 [details]
patch 4 - re-baselined layout test results for Windows

Mark patch 4 as obsolete.

[Roland Steiner]

Landed patch 5a as rev. 53531

[Roland Steiner]

Created attachment 47101 [details]
patch 5b - remove fast/ruby tests from Skipped list for Win

Last patch for full first implementation of ruby: remove ruby layout tests from Skipped list for Windows.

After the landing of the new Latin-character only layout tests (https://bugs.webkit.org/show_bug.cgi?id=31865), and the resolving of the issue that caused other tests to fail (not actually caused by the ruby layout tests(cf. https://bugs.webkit.org/show_bug.cgi?id=31200, problem not actually caused by ruby), there's no reason to leave the ruby layout tests in the Skipped list for Windows.

[Maciej Stachowiak]

Comment on attachment 47101 [details]
patch 5b - remove fast/ruby tests from Skipped list for Win

r=me

In the future, please try to make new bugs for new patches instead of reusing the same bug - it's confusing when multiple independent changes are associated with the same bug. And webkit-patch makes it super easy to file a new bug for each patch.

[Roland Steiner]

Patch 5b landed as 53791, with follow-up fix in 53793