Bug 154977 – Add support for the "last" value of the CSS3 hanging-punctuation property

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

Add support for the "last" value of the CSS3 hanging-punctuation property

[Dave Hyatt]

Created attachment 272767 [details]
Patch

[Darin Adler]

Comment on attachment 272767 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:876
> +                float hangEndWidth = renderText.hangablePunctuationEndWidth(run->m_stop - 1);

Is this right when the character just before run->m_stop is a surrogate pair? If not, we should write a simple function that handles that correctly.

> Source/WebCore/rendering/RenderText.cpp:542
> +    // FIXME: We can't hardcode "textLength() - 1" here, since the
> +    // stripping of trailing spaces at the end of a text run could
> +    // cause a character other than the last one to be the one to

Missing the end of this sentence.

Also, there is more than one reason we can’t hardcode textLength() - 1.

> Source/WebCore/rendering/RenderText.cpp:564
> +        if (text[i] != ' ' && (text[i] != '\n' || style().preserveNewline()) && text[i] != '\t')
> +            break;

I’d be tempted to factor this predicate out into an inline helper function member that takes a character. Just so it’s not repeated.

> Source/WebCore/rendering/RenderText.cpp:575
> +        return textLength() - 1;

Is this right when the last character is a surrogate pair? If not, we should write a simple function that handles that correctly.

> Source/WebCore/rendering/RenderText.cpp:580
> +    int i = textLength() - 1;

Is this right when the last character is a surrogate pair? If not, we should write a simple function that handles that correctly.

[Simon Fraser (smfr)]

Comment on attachment 272767 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockFlow.cpp:4363
> +        unsigned endIndex = trailingSpaceChild == lastText ? lastText->lastCharacterIndexStrippingSpaces() : lastText->textLength() - 1;

Do we know that lastText->textLength() > 0?

> Source/WebCore/rendering/RenderText.cpp:515
> +float RenderText::hangablePunctuationStartWidth(int index) const

unsigned?

> Source/WebCore/rendering/RenderText.cpp:532
> +float RenderText::hangablePunctuationEndWidth(int index) const

unsigned index?

[Dave Hyatt]

(In reply to comment #2)
> 
> Is this right when the character just before run->m_stop is a surrogate
> pair? If not, we should write a simple function that handles that correctly.
> 

It should not be an issue, since the hangable punctuation code tests the character at the index to see if it is in Ps/Pf/Pi categories, and so I believe surrogate pairs will fail that test.

> > Source/WebCore/rendering/RenderText.cpp:542
> > +    // FIXME: We can't hardcode "textLength() - 1" here, since the
> > +    // stripping of trailing spaces at the end of a text run could
> > +    // cause a character other than the last one to be the one to
> 
> Missing the end of this sentence.
> 

Oh that comment no longer applies, since I removed the hardcoding. I will take it out.

> Source/WebCore/rendering/RenderText.cpp:564
> > +        if (text[i] != ' ' && (text[i] != '\n' || style().preserveNewline()) && text[i] != '\t')
> > +            break;
> 
> I’d be tempted to factor this predicate out into an inline helper function
> member that takes a character. Just so it’s not repeated.
>

We actually have a static helper in one of the files but it only tests the character at position 0. I could look into cleaning that up and letting it take an index.

[Dave Hyatt]

Landed in r197519.

[yisibl]

(In reply to comment #5)
> Landed in r197519.

Do you intend to support force-end and allow-end?

> The allow-end and force-end are two variations of hanging punctuation used in East Asia.