Bug 3749 – XHTML 1.1 Ruby Annotation

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKit Misc.

[David Storey]

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

[David Storey]

Created attachment 2690 [details]
Test cases for simple and complex Ruby

[Ian 'Hixie' Hickson]

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.

[Nicholas Shanks]

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.

[Joost de Valk (AlthA)]

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 :)

[Eric Seidel (no email)]

Assigning bugs which hyatt is not actively working on to "nobody" for clarity/consistancy.

[Robert Burns]

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.

[Robert Burns]

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

[Robert Burns]

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

[Roland Steiner]

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)

[Eric Seidel (no email)]

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.

[Shinichiro Hamaji]

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.

[Roland Steiner]

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

[Shinichiro Hamaji]

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

[Roland Steiner]

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.

[Roland Steiner]

Created attachment 32655 [details]
patch: layout tests for ruby implementation

The layout tests for the ruby implementation - see previous patch attachment.

[Roland Steiner]

(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 (?).

[Shinichiro Hamaji]

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

[Roland Steiner]

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

[Roland Steiner]

Correction: the latest patch requires the patch _attachment_ #32822 in bug entry #27324.

[Roland Steiner]

(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! >_<)

[Dave Hyatt]

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?

[Roland Steiner]

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

[Roland Steiner]

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

[Roland Steiner]

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.

[Adam Barth]

The commit queue might not be smart enough yet to handle multiple patches on the same bug, but we'll see.  :)

[Roland Steiner]

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" (?).

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Roland Steiner]

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

[Roland Steiner]

Created attachment 35012 [details]
patch: new ruby implementation (reordering of renderers)

(set reviewer flag)

[Roland Steiner]

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 .

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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

[Roland Steiner]

@_@, 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?)

[Roland Steiner]

Created attachment 41482 [details]
dummy file to disable obsolete patch files

Uploading a dummy file to disable obsolete patch files.

[Eric Seidel (no email)]

You can always obsolete them by clicking on the "details" link for each.  Should this bug even still be open?

[Maciej Stachowiak]

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

[Eric Seidel (no email)]

Comment on attachment 34045 [details]
patch: layout tests for new ruby implementation

Clearing r+ on an obsolete patch.

[Eric Seidel (no email)]

Marking as a duplicate of bug 28420 per above comment.

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