Bug 115149 – Implement ruby-align

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

[Yuki Sekiguchi]

Ruby-align is defined by CSS3 Ruby:
http://www.w3.org/TR/css3-ruby/#rubyalign

I know that CSS3 ruby is outdated, but ruby-align is important feature for Japanese typography.

IE implemented this property except start and end value, and the property works well.
Other browsers don't support this property.

[Yuki Sekiguchi]

Created attachment 199625 [details]
Patch

[Dave Hyatt]

Comment on attachment 199625 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=199625&action=review

r-

Don't forget to patch RenderStyle::diff to do a relayout if ruby-align changes. You should add a test that dynamically changes ruby-align in order to test this.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:671
> +            if (rubyRun->rubyBase())
> +                rubyRun->rubyBase()->layoutIfNeeded();

Can't you fold this code into setIsEndEdge?

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:734
> +        rubyRun->setIsEndEdge(true);
> +        if (rubyRun->rubyBase())
> +            rubyRun->rubyBase()->layoutIfNeeded();

Same here.

> Source/WebCore/rendering/RenderRubyRun.cpp:243
> -    
> +

Remove this accidental whitespace change.

> Source/WebCore/rendering/RenderRubyRun.h:83
> +    void setIsStartEdge(bool isEdge)
> +    {
> +        if (m_isStartEdge != isEdge && rubyBase())
> +            rubyBase()->setNeedsLayout(true, MarkOnlyThis);
> +        m_isStartEdge = isEdge;
> +    }
> +    bool isEndEdge() const { return m_isEndEdge; }
> +    void setIsEndEdge(bool isEdge)
> +    {
> +        if (m_isEndEdge != isEdge && rubyBase())
> +            rubyBase()->setNeedsLayout(true, MarkOnlyThis);
> +        m_isEndEdge = isEdge;
> +    }

Go ahead and do the layoutIfNeeded directly inside the set methods. I would un-inline them also and put them in the .cpp file.

> Source/WebCore/rendering/RenderRubyRun.h:96
> +    bool m_isStartEdge;
> +    bool m_isEndEdge;

I know you set these to false on every layout, but just to avoid future issues in case things change, let's init them to false in the constructor as well.

[Yuki Sekiguchi]

Created attachment 201325 [details]
Patch

[Yuki Sekiguchi]

(In reply to comment #2)
> Don't forget to patch RenderStyle::diff to do a relayout if ruby-align changes. You should add a test that dynamically changes ruby-align in order to test this.
Added test cases and fixed RenderStyle::diff()

> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:671
> > +            if (rubyRun->rubyBase())
> > +                rubyRun->rubyBase()->layoutIfNeeded();
> 
> Can't you fold this code into setIsEndEdge?
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:734
> > +        rubyRun->setIsEndEdge(true);
> > +        if (rubyRun->rubyBase())
> > +            rubyRun->rubyBase()->layoutIfNeeded();
> 
> Same here.
> 
> 
> > Source/WebCore/rendering/RenderRubyRun.h:83
> > +    void setIsStartEdge(bool isEdge)
> > +    {
> > +        if (m_isStartEdge != isEdge && rubyBase())
> > +            rubyBase()->setNeedsLayout(true, MarkOnlyThis);
> > +        m_isStartEdge = isEdge;
> > +    }
> > +    bool isEndEdge() const { return m_isEndEdge; }
> > +    void setIsEndEdge(bool isEdge)
> > +    {
> > +        if (m_isEndEdge != isEdge && rubyBase())
> > +            rubyBase()->setNeedsLayout(true, MarkOnlyThis);
> > +        m_isEndEdge = isEdge;
> > +    }
> 
> Go ahead and do the layoutIfNeeded directly inside the set methods. I would un-inline them also and put them in the .cpp file.
Moved layoutIfNeeded() to setIs*Edge() and setIs*Edge() to .cpp file.


> > Source/WebCore/rendering/RenderRubyRun.cpp:243
> > -    
> > +
> 
> Remove this accidental whitespace change.
Removed.
> 
> > Source/WebCore/rendering/RenderRubyRun.h:96
> > +    bool m_isStartEdge;
> > +    bool m_isEndEdge;
> 
> I know you set these to false on every layout, but just to avoid future issues in case things change, let's init them to false in the constructor as well.
Fixed.

[Yuki Sekiguchi]

Hi David,
Could you re-review this patch?

[Yuki Sekiguchi]

I'm rebasing this patch and fixing to match newer spec.

[Yuki Sekiguchi]

Created attachment 237553 [details]
Patch

[Yuki Sekiguchi]

I changed the following from the old patch:
1. Removed auto, left, end and right value
2. Removed line-edge value and related code
3. Checked ruby-align of RenderRubyBase and RenderRubText, not of RenderRuby

Detail description:
1. Removed auto, left, end and right

The newer spec removed these values.

2. Removed line-edge and related code

The newer spec removed these values. This made me able to remove isStartEdge() and isEndEdge() code.

3. Checked ruby-align of RenderRubyBase and RenderRubText, not of RenderRuby

The newer spec says that ruby-align only apply to ruby bases, ruby annotations, ruby base containers, ruby annotation containers.
This is very different from IE's behavior which the test case in the old patch checks. However, I changed to match with the new spec. This change allow to remove getRubyAlignFromRenderRuby() and make the code simpler.
Since currently RenderRubyBase is not created from rb element, setting ruby-align to rb element is just ignored.
I also removed IE compatibility test from the layout test.

[Michael Catanzaro]

Comment on attachment 237553 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.

[Radar WebKit Bug Importer]

<rdar://problem/98330990>

[Kent Tamura]

FYI: We ship ruby-align in Google Chrome.  https://t.co/Oz16Po3eFE

[fantasai]

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