[webkit-reviews] review granted: [Bug 105695] Abspos Inline block not positioned correctly in text-aligned container : [Attachment 180630] Patch

Thu Jan 3 15:24:24 PST 2013

Ojan Vafai <ojan at chromium.org> has granted Robert Hogan <robert at webkit.org>'s
request for review:
Bug 105695: Abspos Inline block not positioned correctly in text-aligned
container
https://bugs.webkit.org/show_bug.cgi?id=105695

Attachment 180630: Patch
https://bugs.webkit.org/attachment.cgi?id=180630&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=180630&action=review

I'm a little torn about this because our current behavior matches Opera and
IE8/9. Your patch makes us match Firefox. Also, if you remove the p element,
then all the browsers agree that the text-align should apply. So, I'm inclined
to say that this patch is an improvement, but we'll need to watch to keep an
eye out for regressions.

> Source/WebCore/ChangeLog:3
> +	   Abspos Inline block not positioned correctly in text-aligned
container

nit: s/Inline/inline

> LayoutTests/fast/text/container-align-with-inlines.html:1
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN"
"http://www.w3.org/TR/html4/strict.dtd">

Can you clean up this test case some?

-Use <!DOCTYPE html>
-use offsetWidth on the container instead of getComputedStyle.
-get rid of id="body"
-don't need to set the font
-make the background a different color than the text, even better, make the p
and the reference div have different background colors so it's more clear
what's going on

If I were writing this test, I'd also remove the html and head elements, but
that's not a universally agreed upon style, so up to you.