Bug 135349 – [CSS3-Text] Add rendering support for text-justify: none

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Zoltan Horvath]

Add rendering support for the none value of text-justify property.
Spec: http://dev.w3.org/csswg/css-text-3/#valdef-text-justify.none

[Zoltan Horvath]

Created attachment 235629 [details]
Patch

[Darin Adler]

Comment on attachment 235629 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:681
> +            if (textAlign == JUSTIFY && r != trailingSpaceRun
> +#if ENABLE(CSS3_TEXT)
> +                && style().textJustify() != TextJustifyNone) {
> +#else
> +            ) {
> +#endif

It’s better to not have repeated ) { characters to confuse code editors. How about this instead?

                if (textAlign == JUSTIFY && r != trailingSpaceRun
    #if ENABLE(CSS3_TEXT)
                    && style().textJustify() != TextJustifyNone
    #endif
                ) {

Does this really need to be done down here at such a low level? Can’t we change the textAlign value at some higher level instead?

[Zoltan Horvath]

I tried to satisfy the style-checker, that's why I did it this way. Probably the best would be to file a bug against it, and use the simpler version. Do you agree?

> Does this really need to be done down here at such a low level? Can’t we change the textAlign value at some higher level instead?

I'll take a look at this.

[Myles C. Maxfield]

I agree with Darin, but other than that, unofficial r=me as well.

[Zoltan Horvath]

Created attachment 235749 [details]
Patch

[Zoltan Horvath]

I moved the logic up into "RenderBlockFlow::textAlignmentForLine".

[Dave Hyatt]

Comment on attachment 235749 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 235749 [details]
Patch

Clearing flags on attachment: 235749

Committed r172524: <http://trac.webkit.org/changeset/172524>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 235749 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:360
> -#if !ENABLE(CSS3_TEXT)
> -    return (alignment == JUSTIFY) ? TASTART : alignment;
> -#else
> +#if ENABLE(CSS3_TEXT)

We intentionally put the short half of the #if first for easier readability, which is why the !CSS3_TEXT branch came before the #else instead of the other way around. I'm a little disappointed to see that changed in a patch that doesn't give any rationale for the change.

[Darin Adler]

Comment on attachment 235749 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:353
> +    if (alignment == JUSTIFY &&  textJustify == TextJustifyNone)

Extra space here after the "&&".

[Zoltan Horvath]

(In reply to comment #10)
> We intentionally put the short half of the #if first for easier readability, which is why the !CSS3_TEXT branch came before the #else instead of the other way around. I'm a little disappointed to see that changed in a patch that doesn't give any rationale for the change.

It seemed easier my to read the other way. I'm going to update it in a follow up patch to be consistent with the rest of the code. I will CC you.

(In reply to comment #11)
> (From update of attachment 235749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235749&action=review
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:353
> > +    if (alignment == JUSTIFY &&  textJustify == TextJustifyNone)
> 
> Extra space here after the "&&".

And will remove the extra space.