Bug 139012 – Simple line layout: Add <br> support.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

Extend coverage by adding <br> support to multi renderer simple line layout.
<div>
foobar<br>
</div>

[alan]

Created attachment 242128 [details]
WIP patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/SimpleLineLayoutFlowContents.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 3 files


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

[Build Bot]

Comment on attachment 242128 [details]
WIP patch

Attachment 242128 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6036930961604608

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 242129 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 242128 [details]
WIP patch

Attachment 242128 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4695646833475584

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 242130 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[alan]

Simple line layout's renderer dump needs adjustment.

[alan]

Created attachment 242188 [details]
WIP patch

This needs quite bit of a rebaseline.

[alan]

Created attachment 249341 [details]
Patch

[alan]

WIP patch.

[Build Bot]

Comment on attachment 249341 [details]
Patch

Attachment 249341 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5457275296677888

New failing tests:
fast/parser/open-comment-in-textarea.html
http/tests/navigation/javascriptlink-frames.html
http/tests/navigation/postredirect-goback1.html
http/tests/navigation/postredirect-basic.html

[Build Bot]

Created attachment 249348 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 249341 [details]
Patch

Attachment 249341 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5039804475506688

New failing tests:
fast/parser/open-comment-in-textarea.html
http/tests/navigation/javascriptlink-frames.html
http/tests/navigation/postredirect-goback1.html
http/tests/navigation/postredirect-basic.html

[Build Bot]

Created attachment 249349 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[alan]

Created attachment 249378 [details]
Patch

[alan]

WIP patch -> EWS.

[alan]

Created attachment 249426 [details]
Patch

[alan]

Created attachment 249466 [details]
Patch

[alan]

Created attachment 249502 [details]
Patch

[Antti Koivisto]

Comment on attachment 249502 [details]
Patch

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

r=me with comments

> Source/WebCore/rendering/RenderLineBreak.cpp:182
> +    if (auto layout = simpleLineLayout(*this))

auto* layout (here and elsewhere). We like like to document pointers these days.

> Source/WebCore/rendering/RenderLineBreak.cpp:202
> +    if (auto layout = simpleLineLayout(*this)) {

etc.

> Source/WebCore/rendering/SimpleLineLayout.cpp:358
> +            m_trailingWhitespaceLength -= (current.isCollapsed() ? 1 : current.end() - current.start());
> +            m_trailingWhitespaceWidth -= current.width();

You could put !ASSERT_DISABLED around these.

> Source/WebCore/rendering/SimpleLineLayout.cpp:362
> +        ASSERT(!m_trailingWhitespaceLength);
> +        ASSERT(!m_trailingWhitespaceWidth);

Then do

m_trailingWhitespaceLength = 0
m_trailingWhitespaceWidth = 0

after these to avoid unnecessary work in release build.

> Source/WebCore/rendering/SimpleLineLayout.cpp:461
> +    // <br> always produces a run. (required by testing output)
> +    if (currentFragment.type() == TextFragmentIterator::TextFragment::HardLineBreak)
> +        line.appendFragmentAndCreateRunIfNeeded(currentFragment, runs);

:(

> Source/WebCore/rendering/SimpleLineLayout.cpp:476
> +static TextFragmentIterator::TextFragment firstFragment(TextFragmentIterator& textFragmentIterator, LineState& currentLine, const LineState& previousLine, Layout::RunVector& runs)

Could 'textFragmentIterator' argument get a more informative name? What does it represent here?

Why is the argument non-const reference? The function both modifies the argument and returns a value? Can they differ?

Does this function modify currentLine and runs too? This seems surprising since the function name indicates nothing like that.

> Source/WebCore/rendering/SimpleLineLayout.cpp:495
>      // Check if we need to skip the leading whitespace.
> -    if (style.collapseWhitespace) {
> -        while (firstFragment.type() == TextFragmentIterator::TextFragment::Whitespace)
> -            firstFragment = textFragmentIterator.nextTextFragment();
> -    }
> +    firstFragment = skipWhitespaceIfNeeded(firstFragment, textFragmentIterator);

Comment doesn't add much value.

> Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:80
> +struct EndOfSegmentSetter {
> +    EndOfSegmentSetter(std::function<void()> exitFunction)
> +        : scopeExit(WTF::move(exitFunction))
> +    {
> +    }
> +
> +    ~EndOfSegmentSetter()
> +    {
> +        scopeExit();
> +    }
> +private:
> +    std::function<void()> scopeExit;
> +};
> +
>  TextFragmentIterator::TextFragment TextFragmentIterator::nextTextFragment(float xPosition)
>  {
>      // A fragment can either be
> -    // 1. new line character when preserveNewline is on (not considered as whitespace) or
> +    // 1. line break when <br> is present or preserveNewline is on (not considered as whitespace) or
>      // 2. whitespace (collasped, non-collapsed multi or single) or
>      // 3. non-whitespace characters.
> -    // 4. empty, indicating content end.
> +    // 4. content end.
> +    EndOfSegmentSetter segmentEndSetterOnExit([this]() {
> +        m_atEndOfSegment = (m_currentSegment == m_flowContents.end()) | (m_position == m_currentSegment->end);
> +    });

This seems rather eccentric. How about just factoring into two functions where the first one calls the second one and then does the end-of-function actions?

> LayoutTests/ChangeLog:33
> +        This patch enables RenderBlockFlows to use simple line layout on text content when <br> is present.
> +        Simple text with <br> is a fairly common pattern on discussion(forum)-like web pages. This patch reduces memory usage
> +        and speeds up layout for such content.
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * fast/text/simple-line-with-br-expected.html: Added.
> +        * fast/text/simple-line-with-br.html: Added.
> +        * platform/mac-mavericks/fast/parser/open-comment-in-textarea-expected.txt:
> +        * platform/mac-mavericks/http/tests/navigation/javascriptlink-frames-expected.txt:
> +        * platform/mac/fast/css/text-overflow-ellipsis-bidi-expected.txt:
> +        * platform/mac/fast/dom/focus-contenteditable-expected.txt:
> +        * platform/mac/fast/forms/range/slider-padding-expected.txt: Added.
> +        * platform/mac/fast/forms/textarea-scroll-height-expected.txt:
> +        * platform/mac/fast/parser/open-comment-in-textarea-expected.txt:
> +        * platform/mac/fast/text/international/bidi-layout-across-linebreak-expected.txt:
> +        * platform/mac/fast/text/svg-font-face-with-kerning-expected.txt: Added.
> +        * platform/mac/http/tests/navigation/javascriptlink-frames-expected.txt:
> +        * platform/mac/http/tests/navigation/postredirect-basic-expected.txt:
> +        * platform/mac/http/tests/navigation/postredirect-goback1-expected.txt:
> +        * platform/mac/printing/single-line-must-not-be-split-into-two-pages-expected.txt:
> +        * platform/mac/svg/wicd/test-rightsizing-b-expected.txt:
> +        * platform/mac/tables/mozilla/bugs/bug106795-expected.txt:
> +        * platform/mac/tables/mozilla/bugs/bug1224-expected.txt:
> +        * platform/mac/tables/mozilla/bugs/bug131020-expected.txt:
> +        * platform/mac/tables/mozilla/bugs/bug131020_iframe-expected.txt:
> +        * platform/mac/tables/mozilla/bugs/bug1430-expected.txt:
> +        * platform/mac/tables/mozilla/bugs/bug16252-expected.txt:

You should add some comments about of the types of rebaselinings you are doing here and why.

[alan]

Created attachment 250328 [details]
Patch

[alan]

(In reply to comment #20)
> Comment on attachment 249502 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249502&action=review
> 
> r=me with comments
> 
> > Source/WebCore/rendering/RenderLineBreak.cpp:182
> > +    if (auto layout = simpleLineLayout(*this))
> 
> auto* layout (here and elsewhere). We like like to document pointers these
> days.
> 
> > Source/WebCore/rendering/RenderLineBreak.cpp:202
> > +    if (auto layout = simpleLineLayout(*this)) {
> 
> etc.
Fixed.

> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:358
> > +            m_trailingWhitespaceLength -= (current.isCollapsed() ? 1 : current.end() - current.start());
> > +            m_trailingWhitespaceWidth -= current.width();
> 
> You could put !ASSERT_DISABLED around these.
> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:362
> > +        ASSERT(!m_trailingWhitespaceLength);
> > +        ASSERT(!m_trailingWhitespaceWidth);
> 
> Then do
> 
> m_trailingWhitespaceLength = 0
> m_trailingWhitespaceWidth = 0
> 
> after these to avoid unnecessary work in release build.
Fixed.

> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:461
> > +    // <br> always produces a run. (required by testing output)
> > +    if (currentFragment.type() == TextFragmentIterator::TextFragment::HardLineBreak)
> > +        line.appendFragmentAndCreateRunIfNeeded(currentFragment, runs);
> 
> :(
> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:476
> > +static TextFragmentIterator::TextFragment firstFragment(TextFragmentIterator& textFragmentIterator, LineState& currentLine, const LineState& previousLine, Layout::RunVector& runs)
> 
> Could 'textFragmentIterator' argument get a more informative name? What does
> it represent here?
> 
> Why is the argument non-const reference? The function both modifies the
> argument and returns a value? Can they differ?
We call the TextFragmentIterator as textFragmentIterator across these functions. I am happy to change its name if we can come up with a better one.
The input here is the iterator and the return value is the first valid fragment on this line. The argument is non-const reference, because we call .nextFragment() on the iterator until we find the first valid fragment. 

> 
> Does this function modify currentLine and runs too? This seems surprising
> since the function name indicates nothing like that.
:( Yes and it's because of the <br> test output requirement that you commented as ':(' above. When we skip a <br> as part of the first fragment logic, we actually need to create a run for this <br> to accommodate test output requirements. I'll try to refactor it in a follow-up patch (if possible).

> 
> > Source/WebCore/rendering/SimpleLineLayout.cpp:495
> >      // Check if we need to skip the leading whitespace.
> > -    if (style.collapseWhitespace) {
> > -        while (firstFragment.type() == TextFragmentIterator::TextFragment::Whitespace)
> > -            firstFragment = textFragmentIterator.nextTextFragment();
> > -    }
> > +    firstFragment = skipWhitespaceIfNeeded(firstFragment, textFragmentIterator);
> 
> Comment doesn't add much value.
> 
Fixed.

> > Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:80
> > +struct EndOfSegmentSetter {
> > +    EndOfSegmentSetter(std::function<void()> exitFunction)
> > +        : scopeExit(WTF::move(exitFunction))
> > +    {
> > +    }
> > +
> > +    ~EndOfSegmentSetter()
> > +    {
> > +        scopeExit();
> > +    }
> > +private:
> > +    std::function<void()> scopeExit;
> > +};
> > +
> >  TextFragmentIterator::TextFragment TextFragmentIterator::nextTextFragment(float xPosition)
> >  {
> >      // A fragment can either be
> > -    // 1. new line character when preserveNewline is on (not considered as whitespace) or
> > +    // 1. line break when <br> is present or preserveNewline is on (not considered as whitespace) or
> >      // 2. whitespace (collasped, non-collapsed multi or single) or
> >      // 3. non-whitespace characters.
> > -    // 4. empty, indicating content end.
> > +    // 4. content end.
> > +    EndOfSegmentSetter segmentEndSetterOnExit([this]() {
> > +        m_atEndOfSegment = (m_currentSegment == m_flowContents.end()) | (m_position == m_currentSegment->end);
> > +    });
> 
> This seems rather eccentric. How about just factoring into two functions
> where the first one calls the second one and then does the end-of-function
> actions?
Fixed.

> 
> > LayoutTests/ChangeLog:33
> > +        This patch enables RenderBlockFlows to use simple line layout on text content when <br> is present.
> > +        Simple text with <br> is a fairly common pattern on discussion(forum)-like web pages. This patch reduces memory usage
> > +        and speeds up layout for such content.
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * fast/text/simple-line-with-br-expected.html: Added.
> > +        * fast/text/simple-line-with-br.html: Added.
> > +        * platform/mac-mavericks/fast/parser/open-comment-in-textarea-expected.txt:
> > +        * platform/mac-mavericks/http/tests/navigation/javascriptlink-frames-expected.txt:
> > +        * platform/mac/fast/css/text-overflow-ellipsis-bidi-expected.txt:
> > +        * platform/mac/fast/dom/focus-contenteditable-expected.txt:
> > +        * platform/mac/fast/forms/range/slider-padding-expected.txt: Added.
> > +        * platform/mac/fast/forms/textarea-scroll-height-expected.txt:
> > +        * platform/mac/fast/parser/open-comment-in-textarea-expected.txt:
> > +        * platform/mac/fast/text/international/bidi-layout-across-linebreak-expected.txt:
> > +        * platform/mac/fast/text/svg-font-face-with-kerning-expected.txt: Added.
> > +        * platform/mac/http/tests/navigation/javascriptlink-frames-expected.txt:
> > +        * platform/mac/http/tests/navigation/postredirect-basic-expected.txt:
> > +        * platform/mac/http/tests/navigation/postredirect-goback1-expected.txt:
> > +        * platform/mac/printing/single-line-must-not-be-split-into-two-pages-expected.txt:
> > +        * platform/mac/svg/wicd/test-rightsizing-b-expected.txt:
> > +        * platform/mac/tables/mozilla/bugs/bug106795-expected.txt:
> > +        * platform/mac/tables/mozilla/bugs/bug1224-expected.txt:
> > +        * platform/mac/tables/mozilla/bugs/bug131020-expected.txt:
> > +        * platform/mac/tables/mozilla/bugs/bug131020_iframe-expected.txt:
> > +        * platform/mac/tables/mozilla/bugs/bug1430-expected.txt:
> > +        * platform/mac/tables/mozilla/bugs/bug16252-expected.txt:
> 
> You should add some comments about of the types of rebaselinings you are
> doing here and why.
Fixed.

[WebKit Commit Bot]

Comment on attachment 250328 [details]
Patch

Clearing flags on attachment: 250328

Committed r182536: <http://trac.webkit.org/changeset/182536>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This change made accessibility/table-sections.html time out on all debug bots.

[Alexey Proskuryakov]

Zalan asked me to roll out the patch for now. Will do.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 143523

[alan]

Created attachment 250479 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 250479 [details]
Patch

Clearing flags on attachment: 250479

Committed r182620: <http://trac.webkit.org/changeset/182620>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Zalan, this one test is still broken on Windows after this patch, and after all the result updates: printing/single-line-must-not-be-split-into-two-pages.html

[Brent Fulgham]

(In reply to comment #31)
> Zalan, this one test is still broken on Windows after this patch, and after
> all the result updates:
> printing/single-line-must-not-be-split-into-two-pages.html

It looks like the Windows bot is emitting an extra EOL at the end of the test file. Maybe this should just be handled as a windows-specific test expectation?

Zalan, do you think this extra newline is significant?