Bug 3749 - XHTML 1.1 Ruby Annotation
Summary: XHTML 1.1 Ruby Annotation
Status: RESOLVED DUPLICATE of bug 28420
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 312.x
Hardware: Macintosh OS X 10.3
: P3 Enhancement
Assignee: Nobody
URL: http://www.w3.org/TR/2001/REC-ruby-20...
Keywords: HasReduction
Depends on: 26829 26832 27324
Blocks:
  Show dependency treegraph
 
Reported: 2005-06-28 13:17 PDT by David Storey
Modified: 2009-11-06 11:29 PST (History)
8 users (show)

See Also:


Attachments
Test cases for simple and complex Ruby (2.33 KB, text/html)
2005-06-28 13:19 PDT, David Storey
no flags Details
Some CSS I found which adds ruby support. (819 bytes, text/css)
2005-08-08 14:38 PDT, Nicholas Shanks
no flags Details
Ruby Test Case with prposed default user agent css (3.07 KB, text/html)
2006-09-19 18:25 PDT, Robert Burns
no flags Details
Implmentation for basic HTML5 ruby support (deleted)
2009-06-25 02:27 PDT, Roland Steiner
eric: review-
Details | Formatted Diff | Diff
patch: implementation of minimal HTML5 ruby support (91.79 KB, patch)
2009-07-13 02:52 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
patch: layout tests for ruby implementation (deleted)
2009-07-13 03:02 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
patch: HTML5 ruby functionality only - requires patch in bug #32822 for build option (70.37 KB, patch)
2009-07-15 17:51 PDT, Roland Steiner
hyatt: review-
Details | Formatted Diff | Diff
patch: new ruby implementation (reordering of renderers) (55.97 KB, patch)
2009-08-04 01:06 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
patch: layout tests for new ruby implementation (deleted)
2009-08-04 01:08 PDT, Roland Steiner
eric: commit-queue-
Details | Formatted Diff | Diff
patch: new ruby implementation (reordering of renderers) (53.85 KB, patch)
2009-08-17 21:24 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
patch: new ruby implementation (reordering of renderers) (53.85 KB, patch)
2009-08-17 21:26 PDT, Roland Steiner
no flags Details | Formatted Diff | Diff
dummy file to disable obsolete patch files (117 bytes, text/plain)
2009-10-19 23:39 PDT, Roland Steiner
rolandsteiner: review-
rolandsteiner: commit-queue-
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Storey 2005-06-28 13:17:40 PDT
Safari does not support Ruby Annotation, as added to the XHTML spec in 1.1.  Information on Ruby can 
be found in the w3c document stated above.  Currently only IE win/mac supports this (for simple, and 
makes an attempt but fails at complex annotations).  I have created a test case I will attach with this 
report (create from W3C examples).
Comment 1 David Storey 2005-06-28 13:19:31 PDT
Created attachment 2690 [details]
Test cases for simple and complex Ruby
Comment 2 Ian 'Hixie' Hickson 2005-06-28 18:28:04 PDT
Before Safari can really implement the Ruby spec, the Ruby spec has to be
corrected to include error handling rules for what to do when the markup doesn't
match the required syntax. IMHO.
Comment 3 Nicholas Shanks 2005-08-08 14:38:03 PDT
Created attachment 3285 [details]
Some CSS I found which adds ruby support.

This can be used to demonstrate what ruby should look like, and give
rudimentary support if included in your user.css file. It's not perfect though,
but I thought readers might like it as a stop-gap till this bug is fixed.
Comment 4 Joost de Valk (AlthA) 2005-09-08 22:23:03 PDT
I'm confirming this doesn't work. This is, IMHO, an enhancement more than a bug, so i'm marking it p3, 
enhancement. Setting HasReduction too, since it does have a testcase :)
Comment 5 Eric Seidel (no email) 2005-12-27 13:29:39 PST
Assigning bugs which hyatt is not actively working on to "nobody" for clarity/consistancy.
Comment 6 Robert Burns 2006-09-19 18:25:37 PDT
Created attachment 10658 [details]
Ruby Test Case with prposed default user agent css

I tried adding the css to the test case and think this would certainly be an improvement over the current (do nothing) approach. I also think some tweaking to the css may be all that's necessary for WebKit to have full ruby annotation support. I'll take a look at the css, but someone with more css experience than I might make quick work of this.
Comment 7 Robert Burns 2006-09-19 18:33:44 PDT
(In reply to comment #2)
> Before Safari can really implement the Ruby spec, the Ruby spec has to be
> corrected to include error handling rules for what to do when the markup doesn't
> match the required syntax. IMHO.

The issue does not at all seem specific to ruby annotation.  Isn't the current recommendation to not display ill-formed xml (or only up until the error) and if its invalild (e.g., ruby elements containing elements other than rbc, rtc, rt, rb, or rp, then the policiy could be similar to WebKits reponse to similar invalid tables for instance. 
Comment 8 Robert Burns 2006-09-25 19:45:21 PDT
(In reply to comment #6)
> Created an attachment (id=10658) [edit]
> Ruby Test Case with prposed default user agent css
> 
... some tweaking
> to the css may be all that's necessary for WebKit to have full ruby annotation
> support. 

After further reflection, support also for the rspan attribute woudl be necessary for complex ruby base. This attribute is much like the span attributes as implemented in HTML tables. Again some redeployment of code by someone more familiar with this could make relatively quick work of this moving Safarai to XHTML 1.1 compliance.
Comment 9 Roland Steiner 2009-06-25 02:27:15 PDT
Created attachment 31843 [details]
Implmentation for basic HTML5 ruby support

As described in the release notes, this patch implements basic HTML5 ruby support with minor nods towards CSS3. In particular, it does not honor any CS3 ruby properties, nor render complex ruby.

(Put in Dave Hyatt as Reviewer, since we had some discussion on the topic on the mailing list)
Comment 10 Eric Seidel (no email) 2009-06-26 01:24:20 PDT
Comment on attachment 31843 [details]
Implmentation for basic HTML5 ruby support

Not possible to review a 400k patch. :(

Please break this out into smaller pieces.
Comment 11 Shinichiro Hamaji 2009-06-26 10:35:44 PDT
A few unsolicited code reviews for some style issues:

- CSSStyleSelector.cpp has some /* */ comments. Are they really necessary? If so, adding some comments would help readers. Also, ruby.css seems to have a line which is commented out.
- I think destructors should be virtual. Otherwise we may have resource leaks in future.
- It would be better to add "// namespace WebCore" when you close namespaces.
- I think specialHeight() needs some comments to describe how it is special.
- For RenderRuby*::isChildAllowed, WebKit doesn't use line breaks around || , I think.
Comment 12 Roland Steiner 2009-06-28 20:44:11 PDT
(In reply to comment #10 and comment #11)

Will try to break up the patch into smaller chunks, as suggested here and on the mailing list.

> A few unsolicited code reviews for some style issues:
[snipped]

Thanks! Will correct those places.

> - For RenderRuby*::isChildAllowed, WebKit doesn't use line breaks around || , I
> think.

FWIW, I saw both styles in the code (IMHO, line breaks make it easier to add/remove entries), but as the code has solidified, I can remove them.

Comment 13 Shinichiro Hamaji 2009-06-28 23:47:00 PDT
Thanks for fixing!

> FWIW, I saw both styles in the code (IMHO, line breaks make it easier to
> add/remove entries), but as the code has solidified, I can remove them.

Ah, you are right. I thought we should not have line breaks in expressions, but the style guide contains an example like yours.

http://webkit.org/coding/coding-style.html

Comment 14 Roland Steiner 2009-07-13 02:52:47 PDT
Created attachment 32654 [details]
patch: implementation of minimal HTML5 ruby support

Reduced-size patch for HTML5 ruby support. The patch contains no layout tests, which will be contained in a separate patch file, to make things more manageable.
Comment 15 Roland Steiner 2009-07-13 03:02:50 PDT
Created attachment 32655 [details]
patch: layout tests for ruby implementation

The layout tests for the ruby implementation - see previous patch attachment.
Comment 16 Roland Steiner 2009-07-13 04:26:49 PDT
(In reply to comment #14)

The patch is still rather large, but I'm a bit at a loss what else to factor out. The only thing that comes to mind is to separate the configuration files for the ENABLE_RUBY flag from the actual implementation (?).
Comment 17 Shinichiro Hamaji 2009-07-15 00:58:26 PDT
(In reply to comment #14)
> Created an attachment (id=32654) [details]
> patch: implementation of minimal HTML5 ruby support

I found some style issues when I'm testing linter for WebKit:

- WebCore/rendering/RenderRubyBase.h:32:  Missing space before {  [whitespace/braces] [5]
- WebCore/rendering/RubyInlineFlowBox.cpp:40:  Missing space before ( in if(  [whitespace/parens] [5]
- WebCore/rendering/RubyInlineFlowBox.cpp:48:  Missing space before ( in if(  [whitespace/parens] [5]
- This patch contains some tab characters?
Comment 18 Roland Steiner 2009-07-15 17:51:33 PDT
Created attachment 32823 [details]
patch: HTML5 ruby functionality only - requires patch in bug #32822 for build option

In order to further reduce patch sizes, I split out the build option from the actual functionality.

Note that the patch in bug #32822 that adds ENABLE_RUBY must be used/committed before this patch can be applied!

(Also removed the mentioned linting issues.)
Comment 19 Roland Steiner 2009-07-15 17:53:34 PDT
Correction: the latest patch requires the patch _attachment_ #32822 in bug entry #27324.
Comment 20 Roland Steiner 2009-07-15 18:15:53 PDT
(In reply to comment #19)
> Correction: the latest patch requires the patch _attachment_ #32822 in bug
> entry #27324.

Note: updated patch attachment to #27324 (definitely need more coffee! >_<)
Comment 21 Dave Hyatt 2009-07-23 22:40:10 PDT
Comment on attachment 32823 [details]
patch: HTML5 ruby functionality only - requires patch in bug #32822 for build option

Why an extra ruby stylesheet?  I think the rules can just go in html.css.  There are only 4!

ruby { display: inline } is unnecessary.  The default display type is inline, so you don't need that rule.

Can rp just always be display:none?

When does rt occur outside a ruby?

There is funny indentation in InlineFlowBox.h... it looks like you 3-space indented instead of 4.

I do not like the addition of an extra virtual function call in InlineFlowBox.cpp's placeBoxesVertically method.  Possible to avoid it?

In RenderBlock.cpp I worked hard to devirtualize the container walk, so having it be re-virtualized is bothersome.  I'm curious why that is needed.  It looks like you're using some CSS style constant to scramble order.  Is that actually part of HTML5?
Comment 22 Roland Steiner 2009-07-23 23:52:34 PDT
(In reply to comment #21)
> (From update of attachment 32823 [details])
> Why an extra ruby stylesheet?  I think the rules can just go in html.css. 
> There are only 4!

The idea was that ruby.css would not be included if ENABLE_RUBY isn't set. Esp, the CSS rule "rp { display: none; }" shouldn't be used in that case.

> Can rp just always be display:none?
>
> When does rt occur outside a ruby?

Both shouldn't occur outside of <ruby>, but the HTML5 spec says: "An rt element that is not a child of a ruby element represents the same thing as its children." (same for <rp>).

> I do not like the addition of an extra virtual function call in
> InlineFlowBox.cpp's placeBoxesVertically method.  Possible to avoid it?

Will remove this and re-order the children instead, as discussed on #webkit. 
 
> In RenderBlock.cpp I worked hard to devirtualize the container walk, so having
> it be re-virtualized is bothersome.  I'm curious why that is needed.  It looks
> like you're using some CSS style constant to scramble order.  Is that actually
> part of HTML5?

My very first implementation was more CSS3 oriented . That constant and the box model are the only holdover from that implementation: I felt that the ability to choose where to put the ruby text (in the 'before' or 'after' position) was worthwhile to keep. At the moment, this is more of an implementation detail, though - while you CAN put the ruby text into the 'after' position with the current implementation, I haven't added layout tests for that feature yet, because I felt that it may be controversial.

Even without that constant the virtual function however would have been necessary anyway in the current approach, to scramble the layout order as you say.
Comment 23 Roland Steiner 2009-08-04 01:06:19 PDT
Created attachment 34043 [details]
patch: new ruby implementation (reordering of renderers)

New implementation of the HTML5 ruby support.

As discussed with Dave Hyatt, this now works by re-ordering the render children of otherwise standard RenderBlock objects, rather than doing special layouting.

This new implementation made it important to work in the face of DOM manipulation, which I hope I have addressed fully.

Known limitations/bugs: 

.) Selection follows visual layout order, but text copy-paste follows HTML order
.) In some cases there is some incorrect selection painting when selecting right-to-left over the start of a ruby or ruby run.
   (There seems to be no problems if the selection is performed in the "normal" left-to-right direction.)
Comment 24 Roland Steiner 2009-08-04 01:08:20 PDT
Created attachment 34045 [details]
patch: layout tests for new ruby implementation

These are the layout tests for the new ruby implementation.

As the new implementation reorders the renderer children, the layout tests also cover DOM manipulation within <ruby> elements.
Comment 25 Adam Barth 2009-08-04 11:40:21 PDT
The commit queue might not be smart enough yet to handle multiple patches on the same bug, but we'll see.  :)
Comment 26 Roland Steiner 2009-08-04 17:35:50 PDT
Re-reading the bug description, and given the change of focus in my implementation towards HTML5, I wonder if it wouldn't be more correct to open a new bug entry "HTML5 Ruby Annotation" (?).
Comment 27 Eric Seidel (no email) 2009-08-04 19:28:57 PDT
Too big for me to review, but possibly Hyatt can help you.  No need to set commit-queue=?  I'm sure Adam will write up some docs on commit-queue soon.
Comment 28 Eric Seidel (no email) 2009-08-14 17:21:55 PDT
This is too big for me to review, but maybe Hyatt can help you here.  If there is any way to split this patch down into smaller stages, I think that will help your review times substantially.

We don't normally use "get" in method names.  Sometimes "lookup" for when there is some computation needed, but normally instead of "getFoo" we just use "foo". This is covered in the style guide.
Comment 29 Roland Steiner 2009-08-17 21:24:00 PDT
Created attachment 35011 [details]
patch: new ruby implementation (reordering of renderers)

Same as previous "new ruby implementation" patch, with the following minor changes:

.) factored out 'isAfterContent' handling -> separate bug #28376 (reviewed+ by Eric)
.) renamed methods as per Eric's suggestions
Comment 30 Roland Steiner 2009-08-17 21:26:59 PDT
Created attachment 35012 [details]
patch: new ruby implementation (reordering of renderers)

(set reviewer flag)
Comment 31 Roland Steiner 2009-08-17 22:17:17 PDT
As the focus of the patches for this bug entry drifted towards a HTML5-based implementation rather than a CSS3/XHTML-based implementation, new patches will be posted under https://bugs.webkit.org/show_bug.cgi?id=28420 .
Comment 32 Eric Seidel (no email) 2009-09-24 12:56:24 PDT
Comment on attachment 34045 [details]
patch: layout tests for new ruby implementation

These are just tests, and look fine to me.  I assume they depend on other patches so should not be cq+'d?
Comment 33 Eric Seidel (no email) 2009-09-29 18:24:14 PDT
Comment on attachment 34045 [details]
patch: layout tests for new ruby implementation

I'm assuming this depends on the other patch and marking it cq-.
Comment 34 Roland Steiner 2009-10-19 23:34:54 PDT
@_@, Oops, I didn't realize that these patches here are still active. Everything concerning my ruby implementation has moved to https://bugs.webkit.org/show_bug.cgi?id=28420 and the patches here are outdated!

Please disable the patches here (is there a way apart from uploading a dummy file?)
Comment 35 Roland Steiner 2009-10-19 23:39:07 PDT
Created attachment 41482 [details]
dummy file to disable obsolete patch files

Uploading a dummy file to disable obsolete patch files.
Comment 36 Eric Seidel (no email) 2009-10-19 23:46:05 PDT
You can always obsolete them by clicking on the "details" link for each.  Should this bug even still be open?
Comment 37 Maciej Stachowiak 2009-10-20 02:23:44 PDT
Comment on attachment 41482 [details]
dummy file to disable obsolete patch files

Marking final attachment obsolete. (Note: it's possible to mark an attachment obsolete without posting a new one superseding it. Use the "Details" view, there is a checkbox.)
Comment 38 Eric Seidel (no email) 2009-11-06 11:28:56 PST
Comment on attachment 34045 [details]
patch: layout tests for new ruby implementation

Clearing r+ on an obsolete patch.
Comment 39 Eric Seidel (no email) 2009-11-06 11:29:42 PST
Marking as a duplicate of bug 28420 per above comment.

*** This bug has been marked as a duplicate of bug 28420 ***