|
Description
David Storey
2005-06-28 13:17:40 PDT
Created attachment 2690 [details]
Test cases for simple and complex Ruby
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. 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.
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 :) Assigning bugs which hyatt is not actively working on to "nobody" for clarity/consistancy. 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.
(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. (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. 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 on attachment 31843 [details]
Implmentation for basic HTML5 ruby support
Not possible to review a 400k patch. :(
Please break this out into smaller pieces.
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. (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. 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 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.
Created attachment 32655 [details]
patch: layout tests for ruby implementation
The layout tests for the ruby implementation - see previous patch attachment.
(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 (?). (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? 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.) Correction: the latest patch requires the patch _attachment_ #32822 in bug entry #27324. (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 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? (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. 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.)
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.
The commit queue might not be smart enough yet to handle multiple patches on the same bug, but we'll see. :) 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" (?). 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. 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. 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 Created attachment 35012 [details]
patch: new ruby implementation (reordering of renderers)
(set reviewer flag)
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 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 on attachment 34045 [details]
patch: layout tests for new ruby implementation
I'm assuming this depends on the other patch and marking it cq-.
@_@, 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?) Created attachment 41482 [details]
dummy file to disable obsolete patch files
Uploading a dummy file to disable obsolete patch files.
You can always obsolete them by clicking on the "details" link for each. Should this bug even still be open? 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 on attachment 34045 [details]
patch: layout tests for new ruby implementation
Clearing r+ on an obsolete patch.
|