Bug 30846 - display: inline-block + vertical-align: text-bottom causes misalignment.
Summary: display: inline-block + vertical-align: text-bottom causes misalignment.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-28 01:11 PDT by HIRANO Takahito
Modified: 2010-02-23 04:04 PST (History)
8 users (show)

See Also:


Attachments
Test Case (383 bytes, text/html)
2009-10-28 01:11 PDT, HIRANO Takahito
no flags Details
Fix alignment of vertical-align: text-bottom inside an inline-block. (5.49 KB, patch)
2009-11-02 00:16 PST, Yuta Kitamura
abarth: review-
Details | Formatted Diff | Diff
inline-block-vertical-align-2-expected.png (15.67 KB, image/png)
2009-11-02 00:17 PST, Yuta Kitamura
no flags Details
Fix alignment of vertical-align: text-bottom inside an inline-block. (v2) (26.93 KB, patch)
2009-11-16 00:24 PST, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Fix alignment of vertical-align: text-bottom inside an inline-block. (v3: fixed comment) (27.08 KB, patch)
2009-11-18 02:00 PST, Yuta Kitamura
no flags Details | Formatted Diff | Diff
calculate bottom using JS (556 bytes, text/html)
2010-01-14 01:28 PST, Shinichiro Hamaji
no flags Details
Test case for inline-table and inline-box (590 bytes, text/html)
2010-01-14 18:49 PST, Yuta Kitamura
no flags Details
Test case for inline-table (496 bytes, text/html)
2010-01-14 19:09 PST, HIRANO Takahito
no flags Details
Test case for display: inline-table + vertical-align: baseline (496 bytes, text/html)
2010-01-14 19:13 PST, HIRANO Takahito
no flags Details
Fix alignment of vertical-align: text-bottom inside an inline-block. (v4: fixed test) (4.63 KB, patch)
2010-01-14 21:41 PST, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch (v5: apply hamaji's comment) (4.61 KB, patch)
2010-02-22 00:40 PST, Yuta Kitamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description HIRANO Takahito 2009-10-28 01:11:35 PDT
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?
Comment 1 Yuta Kitamura 2009-11-02 00:16:13 PST
Created attachment 42306 [details]
Fix alignment of vertical-align: text-bottom inside an inline-block.
Comment 2 Yuta Kitamura 2009-11-02 00:17:44 PST
Created attachment 42307 [details]
inline-block-vertical-align-2-expected.png
Comment 3 Adam Barth 2009-11-14 22:28:49 PST
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.
Comment 4 Yuta Kitamura 2009-11-15 18:54:51 PST
(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.
Comment 5 Yuta Kitamura 2009-11-16 00:24:31 PST
Created attachment 43272 [details]
Fix alignment of vertical-align: text-bottom inside an inline-block. (v2)
Comment 6 Darin Adler 2009-11-16 13:54:00 PST
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.
Comment 7 Yuta Kitamura 2009-11-16 18:19:58 PST
(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!
Comment 8 Yuta Kitamura 2009-11-18 02:00:54 PST
Created attachment 43415 [details]
Fix alignment of vertical-align: text-bottom inside an inline-block. (v3: fixed comment)
Comment 9 Adam Barth 2009-11-30 12:25:52 PST
Attachment 43415 [details] passed the style-queue
Comment 10 Shinichiro Hamaji 2010-01-14 01:28:37 PST
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?
Comment 11 Yuta Kitamura 2010-01-14 02:39:58 PST
(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.
Comment 12 mitz 2010-01-14 07:43:39 PST
Do we need the same fix for inline-table and inline-box?
Comment 13 Yuta Kitamura 2010-01-14 18:49:50 PST
Created attachment 46634 [details]
Test case for inline-table and inline-box

inline-table and inline-box cases look fine to me.
Comment 14 HIRANO Takahito 2010-01-14 19:09:45 PST
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.
Comment 15 HIRANO Takahito 2010-01-14 19:13:12 PST
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.
Comment 16 Yuta Kitamura 2010-01-14 21:41:49 PST
Created attachment 46641 [details]
Fix alignment of vertical-align: text-bottom inside an inline-block. (v4: fixed test)
Comment 17 Eric Seidel (no email) 2010-01-26 14:54:15 PST
Looks like this just needs one more look from mitz.
Comment 18 Eric Seidel (no email) 2010-02-17 14:45:03 PST
Emailed mitz requesting review.
Comment 19 Shinichiro Hamaji 2010-02-18 19:44:38 PST
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.
Comment 20 Yuta Kitamura 2010-02-18 19:55:21 PST
(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.
Comment 21 Yuta Kitamura 2010-02-22 00:40:15 PST
Created attachment 49190 [details]
Patch (v5: apply hamaji's comment)
Comment 22 Shinichiro Hamaji 2010-02-22 00:53:34 PST
(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 23 Yuta Kitamura 2010-02-22 21:19:05 PST
Comment on attachment 49190 [details]
Patch (v5: apply hamaji's comment)

Thank you for your review! Setting cq-?.
Comment 24 WebKit Commit Bot 2010-02-23 04:04:45 PST
Comment on attachment 49190 [details]
Patch (v5: apply hamaji's comment)

Clearing flags on attachment: 49190

Committed r55141: <http://trac.webkit.org/changeset/55141>
Comment 25 WebKit Commit Bot 2010-02-23 04:04:51 PST
All reviewed patches have been landed.  Closing bug.