Bug 117534

Summary: [CSS3-Text] Add rendering support for text-justify: distribute, 8 bit characters
Product: WebKit Reporter: Dongwoo Joshua Im (dwim) <dw.im>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: ASSIGNED ---    
Severity: Normal CC: bfulgham, mmaxfield, syoichi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 99945    
Attachments:
Description Flags
patch
none
patch mmaxfield: review-

Description Dongwoo Joshua Im (dwim) 2013-06-12 02:55:20 PDT
I will implement the rendering part of text-justify, and proper test cases.
Spec URL - http://www.w3.org/TR/css3-text/#text-justify
Comment 1 Zoltan Horvath 2014-12-09 09:55:27 PST
Created attachment 242934 [details]
patch
Comment 2 Zoltan Horvath 2014-12-09 10:20:41 PST
Created attachment 242937 [details]
patch
Comment 3 Brent Fulgham 2016-08-22 11:22:17 PDT
Seems like something Myles should be reviewing!
Comment 4 Myles C. Maxfield 2016-08-22 17:52:01 PDT
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?
Comment 5 Zoltan Horvath 2016-08-24 11:21:18 PDT
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!