Created attachment 42005 [details] Test Case See the attachment. Expected: The bottom of the inline block should be aligned with the bottom of the text. Actual: The baseline of the inline block is aligned with the bottom of the text. The condition at WebCore/rendering/InlineFlowBox.cpp line 493 doesn't seem to be enough. !isReplaced() should be !IsReplaced() || style()->display() == INLINE_BLOCK. yutak, could you attach the patch to this bug?
Created attachment 42306 [details] Fix alignment of vertical-align: text-bottom inside an inline-block.
Created attachment 42307 [details] inline-block-vertical-align-2-expected.png
Comment on attachment 42306 [details] Fix alignment of vertical-align: text-bottom inside an inline-block. Missing binary file in patch. This patch cannot physically be landed.
(In reply to comment #3) > (From update of attachment 42306 [details]) > Missing binary file in patch. This patch cannot physically be landed. Sorry for that, I'll add it.
Created attachment 43272 [details] Fix alignment of vertical-align: text-bottom inside an inline-block. (v2)
Comment on attachment 43272 [details] Fix alignment of vertical-align: text-bottom inside an inline-block. (v2) > - if (!isReplaced()) // lineHeight - baselinePosition is always 0 for replaced elements, so don't bother wasting time in that case. > + if (!isReplaced() || style()->display() == INLINE_BLOCK) // lineHeight - baselinePosition is always 0 for replaced elements, so don't bother wasting time in that case. We'll need someone more familiar with the rendering code to review the substance of this patch. But the comment needs to be changed. It's now definitely wrong.
(In reply to comment #6) > (From update of attachment 43272 [details]) > > - if (!isReplaced()) // lineHeight - baselinePosition is always 0 for replaced elements, so don't bother wasting time in that case. > > + if (!isReplaced() || style()->display() == INLINE_BLOCK) // lineHeight - baselinePosition is always 0 for replaced elements, so don't bother wasting time in that case. > > We'll need someone more familiar with the rendering code to review the > substance of this patch. But the comment needs to be changed. It's now > definitely wrong. Sure - I'll fix this. Thanks for your review!
Created attachment 43415 [details] Fix alignment of vertical-align: text-bottom inside an inline-block. (v3: fixed comment)
Attachment 43415 [details] passed the style-queue
Created attachment 46546 [details] calculate bottom using JS I cannot determine if the code change is correct either (maybe we can ask Dan to review the code change?). Two comments on the layout tests. I'm not sure, but I guess this can be tested by dumpAsText(). Please see the attachment. I think no red should be seen in passing tests. How about changing the color of underline of the inner span?
(In reply to comment #10) > I cannot determine if the code change is correct either (maybe we can ask Dan > to review the code change?). Two comments on the layout tests. I'm not sure, > but I guess this can be tested by dumpAsText(). Please see the attachment. I > think no red should be seen in passing tests. How about changing the color of > underline of the inner span? Thank you for your suggestion. Sounds convincing. I'd like to fix the test.
Do we need the same fix for inline-table and inline-box?
Created attachment 46634 [details] Test case for inline-table and inline-box inline-table and inline-box cases look fine to me.
Created attachment 46635 [details] Test case for inline-table No. inline-table is also misaligned. http://www.w3.org/TR/CSS21/visudet.html#leading says: The baseline of an 'inline-table' is the baseline of the first row of the table. However, this should be a different issue from this.
Created attachment 46636 [details] Test case for display: inline-table + vertical-align: baseline Ah, that specification was for display: inline-table + vertical-align: baseline. So display: inline-table + vertical-align: text-bottom works fine.
Created attachment 46641 [details] Fix alignment of vertical-align: text-bottom inside an inline-block. (v4: fixed test)
Looks like this just needs one more look from mitz.
Emailed mitz requesting review.
Comment on attachment 46641 [details] Fix alignment of vertical-align: text-bottom inside an inline-block. (v4: fixed test) > +if (outerBottom >= innerBottom) { > + result.innerHTML = 'PASS'; > + result.style.color = 'green'; > +} else { > + result.innerHTML = 'FAIL'; > + result.style.color = 'red'; > +} > +result.innerHTML += ' (outerBottom = ' + outerBottom + ', innerBottom = ' + innerBottom + ')'; I think we should output these values (this is platform dependent value, isn't it?) only when the test fails. Otherwise, we need to have expectations for all platforms.
(In reply to comment #19) > (From update of attachment 46641 [details]) > > +if (outerBottom >= innerBottom) { > > + result.innerHTML = 'PASS'; > > + result.style.color = 'green'; > > +} else { > > + result.innerHTML = 'FAIL'; > > + result.style.color = 'red'; > > +} > > +result.innerHTML += ' (outerBottom = ' + outerBottom + ', innerBottom = ' + innerBottom + ')'; > > I think we should output these values (this is platform dependent value, isn't > it?) only when the test fails. Otherwise, we need to have expectations for all > platforms. Indeed. I'll fix it.
Created attachment 49190 [details] Patch (v5: apply hamaji's comment)
(In reply to comment #21) > Created an attachment (id=49190) [details] > Patch (v5: apply hamaji's comment) The change for the test looks good, thanks for the fix! mitz, could you check the updated patch?
Comment on attachment 49190 [details] Patch (v5: apply hamaji's comment) Thank you for your review! Setting cq-?.
Comment on attachment 49190 [details] Patch (v5: apply hamaji's comment) Clearing flags on attachment: 49190 Committed r55141: <http://trac.webkit.org/changeset/55141>
All reviewed patches have been landed. Closing bug.