Bug 10228 – CSS3: Support the hyphenate property

• Component: CSS
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

I imagine with the help of iconv, WebKit could support CSS3 hypehnate relatively easily.

[Dave Hyatt]

CSS3 Text is still in a state of extreme flux right now though, so I'd be reluctant to implement any of it yet.

[kangax]

Amazing how after almost 4 years "the hyphenation feature is very much up-in-the-air" — quoting relevant CSS3 section (<http://www.w3.org/TR/css3-text/#hyphenate>).

[Nicholas Shanks]

It's still "up‐in‐the‐air" because Hixie is not sure how to spec it and is waiting for a rendering engine that prides itself on good typography to take the lead. I wonder who he might look towards for that inspiration?

[kangax]

It looks like previous version of CSS3 Text module defined few hyphenation-related properties (http://www.w3.org/TR/2007/WD-css3-gcpm-20070205/#hyphenation):

hyphenate: none | auto
hyphenate-dictionary: auto | <uri>
hyphenate-before: <integer>
hyphenate-after: <integer>
hyphenate-lines: none | <integer>
hyphenate-character: string

There was also a related discussion on WHATWG mailing list (http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2007-January/009018.html)

[sw]

I'd like to implement the CSS tag hyphenate
---------
hyphenate: none | auto
--------------

Will the patch get accepted by WebKit?

Thanks,

[sw]

Created attachment 58505 [details]
Add CSS Hyphenate tag support.

Not sure how to test this patch.  Please advise.  Since this patch is a limited change to add CSS Hyphenate tag parsing support.

[sw]

Created attachment 58822 [details]
Add CSS hyphenate tag support -- latest patch with layout tests.

[WebKit Review Bot]

Attachment 58822 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/CSSPrimitiveValueMappings.h:1639:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/css/CSSPrimitiveValueMappings.h:1651:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/style/RenderStyle.h:176:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/RenderStyle.h:200:  _hyphenate is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/rendering/style/RenderStyle.cpp:441:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 5 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

hyphenate is not a tag, it's a property.

[Dave Hyatt]

Comment on attachment 58822 [details]
Add CSS hyphenate tag support -- latest patch with layout tests.

No vendor prefix.  This is CSS3 WD remember.  You can't add an unprefixed property.

[Dave Hyatt]

Also not sure you're looking at current syntax:

http://www.w3.org/TR/2010/WD-css3-gcpm-20100608/

I actually have a patch in progress that adds -webkit-hyphens and -webkit-hyphenate-character from the aforelinked spec. I will clean it up and post it soon.

[sw]

Thanks all for the review.  For the style, I followed the existing code.

Anyway, I'll wait for mitz's patch then.  My patch has a layout test also, if it can be of any use.

Thanks all.

Created attachment 58842 [details]
[WIP] Support hyphens and hyphenate-character

I’d like rendering experts to take a look at the rendering changes. I’ve chosen to not use a separate box for the hyphen, as this greatly simplifies selection, hit-testing, editing etc., and also allows for negative kerning between the last character and the hyphen. I’ve also decided not to subclass InlineTextBox for this purpose.

Still missing:
1. Support for hyphens: none;
2. Support for platforms that can’t hyphenate
3. Better locale selection, and locale-dependent hyphen character for 'auto'
4. Tests

[sw]

hi, Mitz,

Seems you didn't set the review flag to "?", right?

(In reply to comment #15)
> hi, Mitz,
> 
> Seems you didn't set the review flag to "?", right?

Right, the patch is incomplete, but I would like Hyatt’s and others’ feedback as I keep working on it.

I got some feedback from Hyatt saying that the approach is fine (not creating extra line boxes) and suggesting better handling of computed style and some refactoring of the InlineTextBox code that’s repeated three times. I am going to continue my work based on that.

<rdar://problem/7592435>

Created attachment 59053 [details]
[WIP] Support hyphens and hyphenate-character

Cleaned up and added "hyphens: none" support

[sw]

hi, Mitz,

Just wondering whether your hyphenation logic considers the case where a word can have multiple hyphenation points.  Seems from your logic, you only consider lastHyphenPosition.  For example, for word "ex-per-i-ment-al", do you just consider the first hypen between ex and per and other following also?

Thanks,

(In reply to comment #20)
> hi, Mitz,
> 
> Just wondering whether your hyphenation logic considers the case where a word can have multiple hyphenation points.  Seems from your logic, you only consider lastHyphenPosition.  For example, for word "ex-per-i-ment-al", do you just consider the first hypen between ex and per and other following also?
> 
> Thanks,

It will consider the last possible hyphenation location: see how prefixLength is initially computed by measuring (or rather hit-testing), then passed as a parameter to lastHyphenLocation(), which will return the last hyphen location at or before it.

Created attachment 59196 [details]
Implement the 'hyphens' and 'hyphenate-character' properties

[WebKit Review Bot]

Attachment 59196 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/css/CSSPrimitiveValueMappings.h:2165:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/css/CSSPrimitiveValueMappings.h:2180:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
WebCore/rendering/style/RenderStyle.cpp:389:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/RenderStyle.cpp:390:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/RootInlineBox.cpp:120:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 5 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 59196 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3333438

[Darin Adler]

Comment on attachment 59196 [details]
Implement the 'hyphens' and 'hyphenate-character' properties

> +            if (style->hyphenateCharacter() == nullAtom)
> +                return CSSPrimitiveValue::createIdentifier(CSSValueAuto);

I see the use of "== nullAtom" earlier in the file. But I'm surprised we do that instead of calling the isNull function.

> +size_t lastHyphenLocation(const UChar*, size_t length, size_t maxLocation);

To me it's not obvious what the relationship between maxLocation and length is.

> +    static CFLocaleRef locale = 0;
> +    if (!locale) {
> +        RetainPtr<CFStringRef> localeIdentifier(AdoptCF, CFStringCreateWithBytesNoCopy(kCFAllocatorDefault, currentSearchLocaleID(), strlen(currentSearchLocaleID()), kCFStringEncodingASCII, False, kCFAllocatorNull));
> +        locale = CFLocaleCreate(kCFAllocatorDefault, localeIdentifier.get());
> +    }

I prefer initializing with a create function called in the initializer to a separate if statement like this one.

> +    static bool checkedLocale = false;
> +    static bool localeIsEnglish = false;
> +    if (!checkedLocale)
> +        localeIsEnglish = !strcmp("en", currentSearchLocaleID());
> +    if (!localeIsEnglish)
> +        return 0;

Why not just this?

    static bool localeIsEnglish = !strcmp("en", currentSearchLocaleID());

Seems better to me than the two booleans.

> Index: WebCore/rendering/InlineTextBox.cpp
> ===================================================================
> --- WebCore/rendering/InlineTextBox.cpp	(revision 61502)
> +++ WebCore/rendering/InlineTextBox.cpp	(working copy)
> @@ -1,7 +1,7 @@
>  /*
>   * (C) 1999 Lars Knoll ([e-mail])
>   * (C) 2000 Dirk Mueller ([e-mail])
> - * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
> + * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010 Apple Inc. All rights reserved.
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Library General Public
> @@ -103,6 +103,16 @@ RenderObject::SelectionState InlineTextB
>      return state;
>  }
>  
> +static void adjustCharactersAndLengthForHyphen(Vector<UChar, 256> charactersWithHyphen, RenderStyle* style, const UChar*& characters, int& length)

Did you mean to pass a reference to the Vector here?

> -    InlineIterator findNextLineBreak(InlineBidiResolver&, bool firstLine, bool& isLineEmpty, bool& previousLineBrokeCleanly, EClear* clear = 0);
> +    InlineIterator findNextLineBreak(InlineBidiResolver&, bool firstLine, bool& isLineEmpty, bool& previousLineBrokeCleanly, bool& hyphenated, EClear* clear = 0);

The argument name "clear" here seems unneeded.

I think you might want a typedef for the repeated Vector<UChar, 256> type.

The use of "const Hyphens h" also seems wrong. Just "Hyphens h" will do.

The line breaking code looks right to me. I think the test cases are the important bit here.

Looks like EWS builds are failing on this patch. I’ll say review- based on the EWS failures and the lack of the "&" in the Vector argument.

Created attachment 59219 [details]
Implement the 'hyphens' and 'hyphenate-character' properties

(In reply to comment #25)
> (From update of attachment 59196 [details])
> > +            if (style->hyphenateCharacter() == nullAtom)
> > +                return CSSPrimitiveValue::createIdentifier(CSSValueAuto);
> 
> I see the use of "== nullAtom" earlier in the file. But I'm surprised we do that instead of calling the isNull function.

Changed to use isNull().

> > +size_t lastHyphenLocation(const UChar*, size_t length, size_t maxLocation);
> 
> To me it's not obvious what the relationship between maxLocation and length is.

I considered appending BeforeIndex to the function name and renaming the last parameter, but settled on just renaming maxLocation to beforeIndex.

> > +    static CFLocaleRef locale = 0;
> > +    if (!locale) {
> > +        RetainPtr<CFStringRef> localeIdentifier(AdoptCF, CFStringCreateWithBytesNoCopy(kCFAllocatorDefault, currentSearchLocaleID(), strlen(currentSearchLocaleID()), kCFStringEncodingASCII, False, kCFAllocatorNull));
> > +        locale = CFLocaleCreate(kCFAllocatorDefault, localeIdentifier.get());
> > +    }
> 
> I prefer initializing with a create function called in the initializer to a separate if statement like this one.

Done.

> Why not just this?
> 
>     static bool localeIsEnglish = !strcmp("en", currentSearchLocaleID());
> 
> Seems better to me than the two booleans.

Changed (when I wrote this I thought that checking if the locale is English was going to require multiple statements).

> > +static void adjustCharactersAndLengthForHyphen(Vector<UChar, 256> charactersWithHyphen, RenderStyle* style, const UChar*& characters, int& length)
> 
> Did you mean to pass a reference to the Vector here?

Yes.

> > -    InlineIterator findNextLineBreak(InlineBidiResolver&, bool firstLine, bool& isLineEmpty, bool& previousLineBrokeCleanly, EClear* clear = 0);
> > +    InlineIterator findNextLineBreak(InlineBidiResolver&, bool firstLine, bool& isLineEmpty, bool& previousLineBrokeCleanly, bool& hyphenated, EClear* clear = 0);
> 
> The argument name "clear" here seems unneeded.

Removed.

> I think you might want a typedef for the repeated Vector<UChar, 256> type.

Done, although I couldn’t come up with a good name.

> The use of "const Hyphens h" also seems wrong. Just "Hyphens h" will do.

Fixed.

> Looks like EWS builds are failing on this patch. I’ll say review- based on the EWS failures and the lack of the "&" in the Vector argument.

I am not sure what’s causing the failures. Windows appears to be failing to merge the vcproj changes, perhaps due to different line endings. Mac might be failing because I didn’t include an updated WebKitSystemInterface binary. As for Chromium, I don’t know what to do besides adding Hyphenation.cpp to the gypi file.

Created attachment 59220 [details]
Implement the 'hyphens' and 'hyphenate-character' properties

[WebKit Review Bot]

Attachment 59220 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/style/RenderStyle.cpp:389:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/RenderStyle.cpp:390:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 59220 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3290488

Fixed in <http://trac.webkit.org/changeset/61548>.

[James Robinson]

http://trac.webkit.org/changeset/61548/ appears to have regressed performance on one of the Chromium page cyclers by ~10%.  The page cycler consists of lots of pages with different sorts of international content and probably stress the line breaking algorithm.  I would not expect these pages to be using -webkit-hyphen so this is probably unintentional.

Do you know what the issue is likely to be?

(In reply to comment #31)
> http://trac.webkit.org/changeset/61548/ appears to have regressed performance on one of the Chromium page cyclers by ~10%.  The page cycler consists of lots of pages with different sorts of international content and probably stress the line breaking algorithm.  I would not expect these pages to be using -webkit-hyphen so this is probably unintentional.
> 
> Do you know what the issue is likely to be?

I am looking into this now.

r61552 measures as <1.03x slower than r61547 in my tests. Not surprisingly, rolling out just the RenderBlock changes recovers the performance (so the slowness does appear to be in line layout, not drawing, nor style, WKSI etc.).

Created attachment 59347 [details]
Speedup

[David Kilzer (:ddkilzer)]

Comment on attachment 59347 [details]
Speedup

r=me

Created attachment 59349 [details]
Potential speedup

[Kent Tamura]

Created attachment 59357 [details]
Performance regression chart

[Kent Tamura]

(In reply to comment #37)
> Created an attachment (id=59357) [details]
> Performance regression chart

This chart shows rendering time difference between r61547 (just before hyphenate change) and r61565 (hyphenate change + build fix).  The following sites have visible regressions.

www.jagran.com
www.startimes2.com
www.mthai.com
sgkalesh.blogspot.com
sh3bwah.com
news.google.com
manager.co.th
isna.ir
arabicnews.google.com

Note: we use local copies of these sites for performance regression.  So the site as of now might reproduce the regression.

[Kent Tamura]

(In reply to comment #38)
> Note: we use local copies of these sites for performance regression.  So the site as of now might reproduce the regression.

might not reproduce ...

Quick update on performance: I was surprised to see that often there’s higher impact on pages in languages that use the complex text code path. Then I was surprised to find indications in profiles that more time is spent measuring text in r61552 than in r61547. Next I am going to try to confirm that text-measuring is invoked more often or in a way that makes it slower, which was not supposed to happen.

[James Robinson]

Awesome.  Since you can reproduce locally, I think we should revert this patch while you work on the perf issue.  That way our perf bots will be able to catch any other regressions that might occur in the meantime.  Any objections?

(In reply to comment #41)
> Awesome.  Since you can reproduce locally, I think we should revert this patch while you work on the perf issue.  That way our perf bots will be able to catch any other regressions that might occur in the meantime.  Any objections?

I object. I am surprised that posting progress updates which show that I am actively working to resolve the performance issues somehow meant that the changes should be reverted while I am investigating.

[James Robinson]

I don't want to hinder your work in any way and intend no offense.  Since you are able to reproduce the issue locally, however, I see no reason that it's easier to work on the issue with the patch in trunk or outside of it.  Given that my preference would be for rolling it out for now so that the perf bots have a better chance of noticing any regressions that might be introduced by changes elsewhere in the codebase.  If it is easier for you to work on the regression with the patch still in trunk then that is plenty of reason to leave it in.

Historically it's been extremely difficult to track down perf regressions if they are not caught very quickly.

Filed bug 41012.