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.
Created attachment 193467 [details] Patch
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
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
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
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
Created attachment 198135 [details] Patch
Created attachment 200586 [details] Patch
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
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
Created attachment 200588 [details] Patch
Created attachment 201280 [details] Patch
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()
(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. :)
(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?
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)?
Committed r149929: <http://trac.webkit.org/changeset/149929>
(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.
(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!
(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.
(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.