Bug 117534 – [CSS3-Text] Add rendering support for text-justify: inter-character/distribute, 8 bit characters

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: CSS

[Dongwoo Joshua Im (dwim)]

I will implement the rendering part of text-justify, and proper test cases.
Spec URL - http://www.w3.org/TR/css3-text/#text-justify

[Zoltan Horvath]

Created attachment 242934 [details]
patch

[Zoltan Horvath]

Created attachment 242937 [details]
patch

[Brent Fulgham]

Seems like something Myles should be reviewing!

[Myles C. Maxfield]

Comment on attachment 242937 [details]
patch

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

This is a good start, but it seems that the patch is incomplete.

> LayoutTests/fast/css3-text/css3-text-justify/text-justify-8bits.html:20
> +    <b>text-justify: auto</b>

We have a fair amount of smarts with regard to "ruby" interaction with justification (we try to include ruby bases when computing justification expansions). I think we should add some tests which test the interaction between ruby and the text-justify property.

The interaction of ruby and justification is important in e-book readers based off WebKit. It is common for CJK books to be both justified and have ruby.

Speaking of that, we should also add some tests for CJK text with these new justification values to make sure they work as intended.

> Source/WebCore/platform/graphics/Font.cpp:971
> +#if !ENABLE(CSS3_TEXT)

This is yucky. I think we should keep the type declared even without ENABLE(CSS3_TEXT), use a default argument here, and unify the two functions. Then, we can disable the feature by consulting CSS3_TEXT in the parser (if it's undefined, pretend its always "auto").

I don't see any parsing in this patch.... do we currently parse the property, but then do nothing with it?

> Source/WebCore/platform/graphics/Font.cpp:1075
>      if (stringView.is8Bit())

Please add { }

> Source/WebCore/platform/graphics/Font.cpp:1079
> +        return expansionOpportunityCountInternal(stringView.characters8(), stringView.length(), direction, isAfterExpansion, textJustify);

Only for 8-bit strings? What about 8-bit strings which have been promoted to 16-bit?

> Source/WebCore/platform/graphics/TextRun.h:45
> +enum TextJustify {

We should probably make sure this property opts us out of simple line layout. (Or make sure it's supported correctly).

> Source/WebCore/platform/graphics/TextRun.h:266
> +    TextJustify m_textJustify;

TextRuns are ephemeral, so I don't think the space saving is worth guarding this behind the CSS3_TEXT flag. Ultimately, I want to remove the CSS3_TEXT flag entirely.

> Source/WebCore/platform/graphics/WidthIterator.cpp:237
> +            bool isExpansionOpportunity = Font::treatAsSpace(character) || m_run.textJustify() == TextJustifyDistribute;

See my comment below about updating expansionLocation() (this time in WidthIterator.cpp) to know about these new properties.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:150
> +        unsigned expansionOpportunityCount = Font::expansionOpportunityCount(m_run.text(), m_run.ltr() ? LTR : RTL, isAfterExpansion, m_run.textJustify());

I don't think this is complete. Certainly, correctly handling text-justify will affect the expansion opportunity count (and thereby the expansions per opportunity), but I think the expansionLocation() function also needs to be educated about these new expansion locations.

> Source/WebCore/rendering/style/RenderStyle.h:88
> +#include "TextRun.h"

Why is this necessary?

[Zoltan Horvath]

Hey Myles,

Thanks for looking into the patch. I'll reset the assignee to default, because we've shifted priorities, so I don't have time to update or rework it. It would be nice if anyone could pick the topic up!

[Brent Fulgham]

It looks like this work was finally completed in Bug 229084.

[Brent Fulgham]

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