Bug 16898 – REGRESSION: Text of entire page incorrectly center aligned

• Component: CSS
• Status: RESOLVED FIXED

[Elliott Sprehn]

The referenced page has all text center aligned. Firefox 2, Opera 9 and IE6 all left align the text of the page.

Using "javascript:t = document.getElementsByTagName("table"); for(i=0; i < t.length; i++) { t[i].style.textAlign = "left"; }" in the location bar after the page loads seems to correct this, so I'm going to take a guess this is a table that isn't closed somewhere spilling over across the entire page.

[David Kilzer (:ddkilzer)]

This is a regression from Safari 2.0.4 (419.3) with original WebKit on Mac OS X 10.4.11 (8S165).

Confirmed with a local debug build of WebKit r29623 with Safari 3.0.4 (523.12.2) on 10.4.11.

[David Kilzer (:ddkilzer)]

Unfortunately this bug reproduces with the earliest known WebKit nightly build (r11976), so bisect-builds isn't going to help us.

[David Kilzer (:ddkilzer)]

Created attachment 18542 [details]
Reduced test case

The issue is that one of the stylesheets included in the original page (http://www.ffconsultancy.com/style.css) has an invalid rule, which causes WebKit to stop parsing it, while Firefox 2.0.0.x and Opera 9.2x ignore the rule and continue parsing.  The invalid rule is:

    width: *;

When both copies of the invalid rule are removed or commented out, the page renders like Firefox.

[David Kilzer (:ddkilzer)]

Created attachment 18543 [details]
Test case variation 1

Text is green in Firefox and Opera.

[David Kilzer (:ddkilzer)]

Created attachment 18544 [details]
Test case variation 2

Text is green in Firefox and Opera.

[Robert Blaut]

kevino, who is assigned to this bug?

[Kevin Ollivier]

Created attachment 19476 [details]
Fix for the problem that ignores rules with an invalid value

This is my first foray into CSSGrammar.y and Bison code, so I would really appreciate some extra eyes on this fix. I'm not seeing any new layout test failures after the change, but I don't know if there are any corner cases I'm missing or if the approach might be overly general. 

Also, I assume we want a layout test created for this, so that the regression won't recur later on? Where should I put it? And do we need to generate results on all platforms for it? (New to writing LayoutTests too, sorry... ;-)

[Dave Hyatt]

This looks good to me.  Should attach test cases to the patch.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Also, I assume we want a layout test created for this, so that the regression
> won't recur later on? Where should I put it? And do we need to generate results
> on all platforms for it? (New to writing LayoutTests too, sorry... ;-)

I would put the test(s) in LayoutTests/fast/css/.

If you make it a text-based test (using window.layoutTestController.dumpAsText()), then you don't have to worry about generating results for every platform.

Look in LayoutTests/fast/css for examples that use layoutTestController.dumpAsText() and craft a similar test based on one or more reductions in this bug.

I usually prefer to use the js-test-* assertions instead of custom "logging" solutions like getComputedStyle-zIndex-auto.html.  See legacy-opacity-styles.html for one example.

Darin likes to use the WebKitTools/Scripts/make-js-test-wrappers script to create the resources/*.js file first, then let the script generate the test file for it.

[Kevin Ollivier]

Created attachment 19486 [details]
Version of fix with layout test added

[Dave Hyatt]

Comment on attachment 19486 [details]
Version of fix with layout test added

r=me

[Kevin Ollivier]

Landed in r30704, thanks!

*** Bug 161899 has been marked as a duplicate of this bug. ***