Bug 30846 – display: inline-block + vertical-align: text-bottom causes misalignment.

• Component: CSS
• Status: RESOLVED FIXED

[HIRANO Takahito]

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?

[Yuta Kitamura]

Created attachment 42306 [details]
Fix alignment of vertical-align: text-bottom inside an inline-block.

[Yuta Kitamura]

Created attachment 42307 [details]
inline-block-vertical-align-2-expected.png

[Adam Barth]

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.

[Yuta Kitamura]

(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.

[Yuta Kitamura]

Created attachment 43272 [details]
Fix alignment of vertical-align: text-bottom inside an inline-block. (v2)

[Darin Adler]

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.

[Yuta Kitamura]

(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!

[Yuta Kitamura]

Created attachment 43415 [details]
Fix alignment of vertical-align: text-bottom inside an inline-block. (v3: fixed comment)

[Adam Barth]

Attachment 43415 [details] passed the style-queue

[Shinichiro Hamaji]

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?

[Yuta Kitamura]

(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?

[Yuta Kitamura]

Created attachment 46634 [details]
Test case for inline-table and inline-box

inline-table and inline-box cases look fine to me.

[HIRANO Takahito]

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.

[HIRANO Takahito]

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.

[Yuta Kitamura]

Created attachment 46641 [details]
Fix alignment of vertical-align: text-bottom inside an inline-block. (v4: fixed test)

[Eric Seidel (no email)]

Looks like this just needs one more look from mitz.

[Eric Seidel (no email)]

Emailed mitz requesting review.

[Shinichiro Hamaji]

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.

[Yuta Kitamura]

(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.

[Yuta Kitamura]

Created attachment 49190 [details]
Patch (v5: apply hamaji's comment)

[Shinichiro Hamaji]

(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?

[Yuta Kitamura]

Comment on attachment 49190 [details]
Patch (v5: apply hamaji's comment)

Thank you for your review! Setting cq-?.

[WebKit Commit Bot]

Comment on attachment 49190 [details]
Patch (v5: apply hamaji's comment)

Clearing flags on attachment: 49190

Committed r55141: <http://trac.webkit.org/changeset/55141>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.