Bug 117534 - [CSS3-Text] Add rendering support for text-justify: distribute, 8 bit characters
Summary: [CSS3-Text] Add rendering support for text-justify: distribute, 8 bit characters
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 99945
  Show dependency treegraph
 
Reported: 2013-06-12 02:55 PDT by Dongwoo Joshua Im (dwim)
Modified: 2016-08-24 11:21 PDT (History)
3 users (show)

See Also:


Attachments
patch (20.96 KB, patch)
2014-12-09 09:55 PST, Zoltan Horvath
no flags Details | Formatted Diff | Diff
patch (20.99 KB, patch)
2014-12-09 10:20 PST, Zoltan Horvath
mmaxfield: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!