Bug 56388 – Implement the CSS3 line-box-contain property

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Dave Hyatt]

Apply a heuristic to first lines to avoid needlessly growing the line-height.  This would stop the ugly rendering of first lines in books when a larger font is used with only capital letters (e.g., as a first letter or first sentence on the line).

[Dave Hyatt]

Created attachment 85825 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/InlineFlowBox.cpp:576:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/InlineFlowBox.cpp:577:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/InlineFlowBox.cpp:578:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 3 in 12 files


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

[Dave Hyatt]

<rdar://problem/7958703> Extra spacing below line with drop cap

[Dave Hyatt]

Created attachment 85826 [details]
Patch that passes style queue

[Dave Hyatt]

Comment on attachment 85826 [details]
Patch that passes style queue

Clearing flag for now, since some layout tests fail and I want to study them and make sure it's ok before proceeding.

[Dave Hyatt]

Created attachment 85851 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.cpp:399:  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]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:367:  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 24 files


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

[Dave Hyatt]

Comment on attachment 85851 [details]
Patch

Clearing flag. Going to get some www-style discussion going about this.

[Dave Hyatt]

Created attachment 86231 [details]
Patch

Not for review.  Just testing builds.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/GNUmakefile.am', u'Source/W..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.cpp:399:  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]
Source/WebCore/rendering/RenderBlockLineLayout.cpp:365:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 30 files


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

[Dave Hyatt]

Created attachment 86340 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBlockLineLayout.cpp:365:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/style/RenderStyle.cpp:399:  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 79 files


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

[Dave Hyatt]

Created attachment 86341 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderBlockLineLayout.cpp:365:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/rendering/style/RenderStyle.cpp:399:  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 79 files


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

[Simon Fraser (smfr)]

Comment on attachment 86341 [details]
Patch

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

r+ but I would like to see a property parsing test.

> Source/WebCore/GNUmakefile.am:1014
> +        Source/WebCore/css/CSSLineBoxContainValue.cpp \
> +	Source/WebCore/css/CSSLineBoxContainValue.h \

Odd indentation here. Extra tab?

> Source/WebCore/css/CSSLineBoxContainValue.h:38
> +enum LineBoxContain { LineBoxContainNone = 0x0, LineBoxContainBlock = 0x1, LineBoxContainInline = 0x2, LineBoxContainFont = 0x4, LineBoxContainGlyphs = 0x8,
> +                      LineBoxContainReplaced = 0x10, LineBoxContainInlineBox = 0x20 };

I have a minor preference for 0, 1 << 0, 1 << 1 etc. Makes it super-obvious that it's a bitmask.

I think you should also typedef the bitflags:

enum LineBoxContainFlags { ... }
typedef unsigned LineBoxContain

> Source/WebCore/css/CSSLineBoxContainValue.h:50
> +    unsigned value() const { return m_value; }

This could use the typedef then.

> Source/WebCore/platform/graphics/Font.h:72
> +    bool computeBounds;

It's not obvious to me what this means on a struct. It suggests that bounds have to be computed, but by whom, and when? If it's true, does that affect the other members?

> Source/WebCore/platform/graphics/FontFastPath.cpp:457
> +        glyphOverflow->top = max<int>(glyphOverflow->top, ceilf(-it.minGlyphBoundingBoxY()) - (glyphOverflow->computeBounds ? 0  : fontMetrics().ascent()));
> +        glyphOverflow->bottom = max<int>(glyphOverflow->bottom, ceilf(it.maxGlyphBoundingBoxY()) - (glyphOverflow->computeBounds ? 0  : fontMetrics().descent()));

Extra space before the :

> Source/WebCore/platform/graphics/mac/FontComplexTextMac.cpp:115
> +        glyphOverflow->bottom = max<int>(glyphOverflow->bottom, ceilf(controller.maxGlyphBoundingBoxY()) - (glyphOverflow->computeBounds ? 0  : fontMetrics().descent()));

Ditto

> Source/WebCore/rendering/RootInlineBox.cpp:558
> +static void setAscentAndDescent(int& ascent, int& descent, int newAscent, int newDescent, bool& ascentDescentSet)

I'd expect the out params at the end of the parameter list. Maybe rename this to 'storeAscentAndDescent'?

> Source/WebCore/rendering/RootInlineBox.cpp:594
> +        glyphOverflow = it == textBoxDataMap.end() ? 0 : &it->second.second;

Maybe assign "it == textBoxDataMap.end()" to a local variable?

> Source/WebCore/rendering/RootInlineBox.cpp:727
> +        if (verticalAlign == SUB)
> +            verticalPosition += fontSize / 5 + 1;
> +        else if (verticalAlign == SUPER)
> +            verticalPosition -= fontSize / 3 + 1;
> +        else if (verticalAlign == TEXT_TOP)
> +            verticalPosition += renderer->baselinePosition(baselineType(), firstLine, lineDirection) - fontMetrics.ascent(baselineType());
> +        else if (verticalAlign == MIDDLE)
> +            verticalPosition += -static_cast<int>(fontMetrics.xHeight() / 2) - renderer->lineHeight(firstLine, lineDirection) / 2 + renderer->baselinePosition(baselineType(), firstLine, lineDirection);
> +        else if (verticalAlign == TEXT_BOTTOM) {
> +            verticalPosition += fontMetrics.descent(baselineType());
> +            // lineHeight - baselinePosition is always 0 for replaced elements (except inline blocks), so don't bother wasting time in that case.
> +            if (!renderer->isReplaced() || renderer->isInlineBlockOrInlineTable())
> +                verticalPosition -= (renderer->lineHeight(firstLine, lineDirection) - renderer->baselinePosition(baselineType(), firstLine, lineDirection));
> +        } else if (verticalAlign == BASELINE_MIDDLE)
> +            verticalPosition += -renderer->lineHeight(firstLine, lineDirection) / 2 + renderer->baselinePosition(baselineType(), firstLine, lineDirection);
> +        else if (verticalAlign == LENGTH)
> +            verticalPosition -= renderer->style()->verticalAlignLength().calcValue(renderer->lineHeight(firstLine, lineDirection));

Make this a switch? (Yeah, I know you just moved it).

> Source/WebCore/rendering/RootInlineBox.cpp:742
> +    unsigned lineBoxContain = renderer()->style()->lineBoxContain();

Could use the typedef here, and below.

> Source/WebCore/rendering/RootInlineBox.cpp:754
> +    // For now map "glyphs" to "font" in vertical text mode until the bounds returned by glyphs aren't garbage.

Does that need a bug?

> Source/WebCore/rendering/style/RenderStyle.h:776
> +    unsigned lineBoxContain() { return rareInheritedData->m_lineBoxContain; }

Should be const, and use the typedef.

> LayoutTests/ChangeLog:81
> +        * platform/mac/fast/block/lineboxcontain/replaced-expected.png: Added.
> +        * platform/mac/fast/block/lineboxcontain/replaced-expected.txt: Added.
> +

I think you should add a property parsing test also.

[Dave Hyatt]

Fixed in r81684.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81684 might have broken EFL Linux Release (Build)