Bug 56388 - Implement the CSS3 line-box-contain property
: Implement the CSS3 line-box-contain property
Status: RESOLVED FIXED
: WebKit
Layout and Rendering
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2011-03-15 10:36 PST by
Modified: 2011-03-22 12:20 PST (History)


Attachments
Patch (86.46 KB, patch)
2011-03-15 10:39 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch that passes style queue (109.08 KB, patch)
2011-03-15 10:52 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (133.67 KB, patch)
2011-03-15 13:37 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (64.02 KB, patch)
2011-03-18 15:44 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (493.06 KB, patch)
2011-03-21 11:19 PST, Dave Hyatt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (492.92 KB, patch)
2011-03-21 11:26 PST, Dave Hyatt
simon.fraser: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-03-15 10:36:20 PST
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).
------- Comment #1 From 2011-03-15 10:39:05 PST -------
Created an attachment (id=85825) [details]
Patch
------- Comment #2 From 2011-03-15 10:40:16 PST -------
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.
------- Comment #3 From 2011-03-15 10:50:31 PST -------
<rdar://problem/7958703> Extra spacing below line with drop cap
------- Comment #4 From 2011-03-15 10:52:05 PST -------
Created an attachment (id=85826) [details]
Patch that passes style queue
------- Comment #5 From 2011-03-15 11:34:03 PST -------
(From update of attachment 85826 [details])
Clearing flag for now, since some layout tests fail and I want to study them and make sure it's ok before proceeding.
------- Comment #6 From 2011-03-15 13:37:26 PST -------
Created an attachment (id=85851) [details]
Patch
------- Comment #7 From 2011-03-15 13:40:22 PST -------
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.
------- Comment #8 From 2011-03-16 11:03:37 PST -------
(From update of attachment 85851 [details])
Clearing flag. Going to get some www-style discussion going about this.
------- Comment #9 From 2011-03-18 15:44:34 PST -------
Created an attachment (id=86231) [details]
Patch

Not for review.  Just testing builds.
------- Comment #10 From 2011-03-18 15:47:56 PST -------
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.
------- Comment #11 From 2011-03-21 11:19:33 PST -------
Created an attachment (id=86340) [details]
Patch
------- Comment #12 From 2011-03-21 11:23:02 PST -------
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.
------- Comment #13 From 2011-03-21 11:26:07 PST -------
Created an attachment (id=86341) [details]
Patch
------- Comment #14 From 2011-03-21 11:28:20 PST -------
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.
------- Comment #15 From 2011-03-21 14:41:36 PST -------
(From update of attachment 86341 [details])
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.
------- Comment #16 From 2011-03-22 12:04:14 PST -------
Fixed in r81684.
------- Comment #17 From 2011-03-22 12:20:28 PST -------
http://trac.webkit.org/changeset/81684 might have broken EFL Linux Release (Build)