Bug 28420 - Implement HTML5 <ruby> rendering
: Implement HTML5 <ruby> rendering
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering
: 528+ (Nightly build)
: All All
: P3 Normal
Assigned To: Roland Steiner
:
Depends on:
Blocks: 32934
  Show dependency treegraph
 
Reported: 2009-08-17 22:15 PDT by Roland Steiner
Modified: 2010-01-25 01:05 PST (History)
5 users (show)

See Also:


Attachments
patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej (31.15 KB, patch)
2009-08-17 23:22 PDT, Roland Steiner
eric: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
patch 2: ruby functionality (54.72 KB, patch)
2009-08-18 03:25 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
patch 3: layout tests for ruby (713.83 KB, patch)
2009-08-18 03:26 PDT, Roland Steiner
hyatt: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
patch 2: ruby functionality (54.26 KB, patch)
2009-08-24 22:31 PDT, Roland Steiner
hyatt: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
patch 4 - re-baselined layout test results for Windows (675.77 KB, patch)
2009-11-04 04:03 PST, Roland Steiner
eric: review+
Details | Formatted Diff | Diff
patch 5a - remove fast/ruby tests from Skipped list for Mac (1.15 KB, patch)
2009-11-11 21:07 PST, Roland Steiner
darin: review+
Details | Formatted Diff | Diff
patch 5b - remove fast/ruby tests from Skipped list for Win (1.61 KB, patch)
2010-01-20 23:34 PST, Roland Steiner
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Steiner 2009-08-17 22:15:50 PDT
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.
Comment 1 Roland Steiner 2009-08-17 23:22:22 PDT
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.
Comment 2 Roland Steiner 2009-08-18 03:25:03 PDT
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).
Comment 3 Roland Steiner 2009-08-18 03:26:35 PDT
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
Comment 4 Roland Steiner 2009-08-24 22:31:02 PDT
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.
Comment 5 Roland Steiner 2009-08-25 05:43:32 PDT
Rebaselined versions of the layout tests for Chromium-Win:

http://codereview.chromium.org/173347
Comment 6 Eric Seidel 2009-09-03 01:38:07 PDT
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.
Comment 7 Eric Seidel 2009-09-03 01:39:08 PDT
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.
Comment 8 Eric Seidel 2009-09-03 01:39:42 PDT
bug 28230 is the bug I was referring to.
Comment 9 Dave Hyatt 2009-09-29 11:22:49 PDT
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
Comment 10 Yong Li 2009-10-19 08:50:00 PDT
Comment on attachment 35020 [details]
patch 1: remove ENABLE_RUBY flag as discussed with Dave and Maciej

Let commit bot land it
Comment 11 WebKit Commit Bot 2009-10-19 09:27:14 PDT
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.
Comment 12 WebKit Commit Bot 2009-10-19 09:32:15 PDT
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
Comment 13 WebKit Commit Bot 2009-10-19 09:32:49 PDT
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.
Comment 14 Eric Seidel 2009-10-19 15:15:28 PDT
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.
Comment 15 Roland Steiner 2009-10-19 19:33:41 PDT
(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.
Comment 16 Roland Steiner 2009-10-30 02:17:17 PDT
Landed patch 1.
Comment 17 Roland Steiner 2009-11-01 19:17:13 PST
Landed patch 2.
Comment 18 Eric Seidel 2009-11-01 21:04:02 PST
http://trac.webkit.org/changeset/50397 broke the windows build. We need to fix it or roll it out.
Comment 19 Roland Steiner 2009-11-01 21:19:59 PST
Fixed the windows build.
Comment 20 Roland Steiner 2009-11-04 04:03:17 PST
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.
Comment 21 Roland Steiner 2009-11-04 19:59:14 PST
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.
Comment 22 Eric Seidel 2009-11-06 11:29:42 PST
*** Bug 3749 has been marked as a duplicate of this bug. ***
Comment 23 Brian Weinstein 2009-11-10 22:20:38 PST
Can we remove the tests from the Windows Skipped List now?
Comment 24 Brian Weinstein 2009-11-11 10:45:39 PST
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?
Comment 25 Roland Steiner 2009-11-11 16:23:28 PST
(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?
Comment 26 Roland Steiner 2009-11-11 21:07:56 PST
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.
Comment 27 Eric Seidel 2009-12-02 12:17:56 PST
Roland: Ping?
Comment 28 Roland Steiner 2009-12-02 19:30:02 PST
(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.
Comment 29 Eric Seidel 2009-12-14 14:04:19 PST
Should we r- these patches then?  I'm unclear on what should be done with this bug.
Comment 30 Roland Steiner 2009-12-14 18:13:50 PST
(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).
Comment 31 Roland Steiner 2010-01-20 00:43:28 PST
Comment on attachment 42475 [details]
patch 4 - re-baselined layout test results for Windows

Mark patch 4 as obsolete.
Comment 32 Roland Steiner 2010-01-20 03:34:21 PST
Landed patch 5a as rev. 53531
Comment 33 Roland Steiner 2010-01-20 23:34:38 PST
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.
Comment 34 Maciej Stachowiak 2010-01-21 00:49:39 PST
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.
Comment 35 Roland Steiner 2010-01-25 01:05:54 PST
Patch 5b landed as 53791, with follow-up fix in 53793