Text flow broken in elements with vertical align top/bottom and inline elements taller than line-height

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Miguel Ventura]

See example URL for demo: http://jsfiddle.net/gqL5D/

In an element with vertical-align top or bottom, if there's an element in the line that's taller than line-height, the text nodes will be vertically aligned differently from the inline elements.

IE and Firefox render correctly whereas in Safari and Chrome the example given seems like a roller coaster.

[Robert Hogan]

Created attachment 193467 [details]
Patch

[Build Bot]

Comment on attachment 193467 [details]
Patch

Attachment 193467 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17145512

New failing tests:
editing/execCommand/query-command-state.html

[WebKit Review Bot]

Comment on attachment 193467 [details]
Patch

Attachment 193467 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17181507

New failing tests:
editing/execCommand/query-command-state.html

[Build Bot]

Comment on attachment 193467 [details]
Patch

Attachment 193467 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17222259

New failing tests:
editing/execCommand/query-command-state.html

[Build Bot]

Comment on attachment 193467 [details]
Patch

Attachment 193467 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17174374

New failing tests:
editing/execCommand/query-command-state.html

[Robert Hogan]

Created attachment 198135 [details]
Patch

[Robert Hogan]

Created attachment 200586 [details]
Patch

[Build Bot]

Comment on attachment 200586 [details]
Patch

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

New failing tests:
tables/mozilla/bugs/bug14323.html

[Build Bot]

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

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

[Robert Hogan]

Created attachment 200588 [details]
Patch

[Robert Hogan]

Created attachment 201280 [details]
Patch

[Glenn Adams]

Comment on attachment 201280 [details]
Patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:486
> +    return !(curr->isText() && !curr->parent()->isInline() && !curr->parent()->isTableCell());

Is this correct? i.e., what you have is the same as !curr->isText() || curr->parent()->isInline() || curr->parent()->isTableCell()

[Robert Hogan]

(In reply to comment #12)
> (From update of attachment 201280 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201280&action=review
> 
> > Source/WebCore/rendering/InlineFlowBox.cpp:486
> > +    return !(curr->isText() && !curr->parent()->isInline() && !curr->parent()->isTableCell());
> 
> Is this correct? i.e., what you have is the same as !curr->isText() || curr->parent()->isInline() || curr->parent()->isTableCell()

Well they're both correct - but yours is better. :)

[Glenn Adams]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 201280 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=201280&action=review
> > 
> > > Source/WebCore/rendering/InlineFlowBox.cpp:486
> > > +    return !(curr->isText() && !curr->parent()->isInline() && !curr->parent()->isTableCell());
> > 
> > Is this correct? i.e., what you have is the same as !curr->isText() || curr->parent()->isInline() || curr->parent()->isTableCell()
> 
> Well they're both correct - but yours is better. :)

Yes, it's best to use De Morgan's laws to simplify, but I also wanted to check that when !curr->isText() is true that it's always something to which vertical position applies.

For example, there is nothing stopping curr from being RenderBlock, yes?

[Ryosuke Niwa]

Comment on attachment 201280 [details]
Patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:485
> +    // The vertical-align property only applies to inline elements and table cells.

Instead of repeating the code, refer to http://www.w3.org/TR/2011/REC-CSS2-20110607/visudet.html#line-height.

Should we also check this condition in RenderStyle::diff (for optimization)?

[Robert Hogan]

Committed r149929: <http://trac.webkit.org/changeset/149929>

[Glenn Adams]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 201280 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=201280&action=review
> > > 
> > > > Source/WebCore/rendering/InlineFlowBox.cpp:486
> > > > +    return !(curr->isText() && !curr->parent()->isInline() && !curr->parent()->isTableCell());
> > > 
> > > Is this correct? i.e., what you have is the same as !curr->isText() || curr->parent()->isInline() || curr->parent()->isTableCell()
> > 
> > Well they're both correct - but yours is better. :)
> 
> Yes, it's best to use De Morgan's laws to simplify, but I also wanted to check that when !curr->isText() is true that it's always something to which vertical position applies.
> 
> For example, there is nothing stopping curr from being RenderBlock, yes?

It would have been nice if you'd simplified the logic and added an assert on curr being an inline renderer before committing.

[Robert Hogan]

(In reply to comment #17)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > (From update of attachment 201280 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=201280&action=review
> > > > 
> > > > > Source/WebCore/rendering/InlineFlowBox.cpp:486
> > > > > +    return !(curr->isText() && !curr->parent()->isInline() && !curr->parent()->isTableCell());
> > > > 
> > > > Is this correct? i.e., what you have is the same as !curr->isText() || curr->parent()->isInline() || curr->parent()->isTableCell()
> > > 
> > > Well they're both correct - but yours is better. :)
> > 
> > Yes, it's best to use De Morgan's laws to simplify, but I also wanted to check that when !curr->isText() is true that it's always something to which vertical position applies.
> > 
> > For example, there is nothing stopping curr from being RenderBlock, yes?
> 
> It would have been nice if you'd simplified the logic and added an assert on curr being an inline renderer before committing.

I implemented your suggestion before landing!

[Robert Hogan]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 201280 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=201280&action=review
> > > 
> > > > Source/WebCore/rendering/InlineFlowBox.cpp:486
> > > > +    return !(curr->isText() && !curr->parent()->isInline() && !curr->parent()->isTableCell());
> > > 
> > > Is this correct? i.e., what you have is the same as !curr->isText() || curr->parent()->isInline() || curr->parent()->isTableCell()
> > 
> > Well they're both correct - but yours is better. :)
> 
> Yes, it's best to use De Morgan's laws to simplify, but I also wanted to check that when !curr->isText() is true that it's always something to which vertical position applies.
> 
> For example, there is nothing stopping curr from being RenderBlock, yes?

Correct but it will be an inline block so that's alright.

[Glenn Adams]

(In reply to comment #19)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > (From update of attachment 201280 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=201280&action=review
> > > > 
> > > > > Source/WebCore/rendering/InlineFlowBox.cpp:486
> > > > > +    return !(curr->isText() && !curr->parent()->isInline() && !curr->parent()->isTableCell());
> > > > 
> > > > Is this correct? i.e., what you have is the same as !curr->isText() || curr->parent()->isInline() || curr->parent()->isTableCell()
> > > 
> > > Well they're both correct - but yours is better. :)
> > 
> > Yes, it's best to use De Morgan's laws to simplify, but I also wanted to check that when !curr->isText() is true that it's always something to which vertical position applies.
> > 
> > For example, there is nothing stopping curr from being RenderBlock, yes?
> 
> Correct but it will be an inline block so that's alright.

In that case, an assert would have been useful, since it in fact assumes that.

Thanks for implementing the simplification. I looked at the last patch, which didn't have it. It would be useful for you to use land-safely if you make a change so that at least the final form of the commit is uploaded as a patch to the bug.