Bug 115149 - Implement ruby-align
Summary: Implement ruby-align
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-04-24 23:56 PDT by Yuki Sekiguchi
Modified: 2022-08-08 11:20 PDT (History)
15 users (show)

See Also:


Attachments
Patch (67.61 KB, patch)
2013-04-25 00:47 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (75.86 KB, patch)
2013-05-10 01:24 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff
Patch (68.60 KB, patch)
2014-09-03 03:44 PDT, Yuki Sekiguchi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuki Sekiguchi 2013-04-24 23:56:15 PDT
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.
Comment 1 Yuki Sekiguchi 2013-04-25 00:47:43 PDT
Created attachment 199625 [details]
Patch
Comment 2 Dave Hyatt 2013-04-30 14:57:33 PDT
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.
Comment 3 Yuki Sekiguchi 2013-05-10 01:24:24 PDT
Created attachment 201325 [details]
Patch
Comment 4 Yuki Sekiguchi 2013-05-10 03:28:26 PDT
(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.
Comment 5 Yuki Sekiguchi 2013-05-10 03:37:16 PDT
Hi David,
Could you re-review this patch?
Comment 6 Yuki Sekiguchi 2014-09-02 03:29:44 PDT
I'm rebasing this patch and fixing to match newer spec.
Comment 7 Yuki Sekiguchi 2014-09-03 03:44:04 PDT
Created attachment 237553 [details]
Patch
Comment 8 Yuki Sekiguchi 2014-09-03 03:58:06 PDT
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.
Comment 9 Michael Catanzaro 2016-09-17 07:08:09 PDT
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.
Comment 10 Radar WebKit Bug Importer 2022-08-08 11:20:55 PDT
<rdar://problem/98330990>