Bug 16898 - REGRESSION: Text of entire page incorrectly center aligned
Summary: REGRESSION: Text of entire page incorrectly center aligned
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Kevin Ollivier
URL: http://www.ffconsultancy.com/language...
Keywords: HasReduction, Regression
: 161899 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-01-16 18:22 PST by Elliott Sprehn
Modified: 2016-09-13 02:51 PDT (History)
4 users (show)

See Also:


Attachments
Reduced test case (90 bytes, text/html)
2008-01-19 09:29 PST, David Kilzer (:ddkilzer)
no flags Details
Test case variation 1 (89 bytes, text/html)
2008-01-19 09:34 PST, David Kilzer (:ddkilzer)
no flags Details
Test case variation 2 (89 bytes, text/html)
2008-01-19 09:34 PST, David Kilzer (:ddkilzer)
no flags Details
Fix for the problem that ignores rules with an invalid value (1.08 KB, patch)
2008-03-01 18:10 PST, Kevin Ollivier
no flags Details | Formatted Diff | Diff
Version of fix with layout test added (3.18 KB, patch)
2008-03-02 15:48 PST, Kevin Ollivier
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2008-01-16 18:22:43 PST
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.
Comment 1 David Kilzer (:ddkilzer) 2008-01-19 04:09:56 PST
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.

Comment 2 David Kilzer (:ddkilzer) 2008-01-19 04:12:54 PST
Unfortunately this bug reproduces with the earliest known WebKit nightly build (r11976), so bisect-builds isn't going to help us.

Comment 3 David Kilzer (:ddkilzer) 2008-01-19 09:29:12 PST
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.
Comment 4 David Kilzer (:ddkilzer) 2008-01-19 09:34:07 PST
Created attachment 18543 [details]
Test case variation 1

Text is green in Firefox and Opera.
Comment 5 David Kilzer (:ddkilzer) 2008-01-19 09:34:46 PST
Created attachment 18544 [details]
Test case variation 2

Text is green in Firefox and Opera.
Comment 6 Robert Blaut 2008-03-01 02:54:25 PST
kevino, who is assigned to this bug?
Comment 7 Kevin Ollivier 2008-03-01 18:10:12 PST
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... ;-)
Comment 8 Dave Hyatt 2008-03-01 18:14:12 PST
This looks good to me.  Should attach test cases to the patch.
Comment 9 David Kilzer (:ddkilzer) 2008-03-02 08:33:33 PST
(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.
Comment 10 Kevin Ollivier 2008-03-02 15:48:50 PST
Created attachment 19486 [details]
Version of fix with layout test added
Comment 11 Dave Hyatt 2008-03-02 17:31:25 PST
Comment on attachment 19486 [details]
Version of fix with layout test added

r=me
Comment 12 Kevin Ollivier 2008-03-02 18:04:11 PST
Landed in r30704, thanks!
Comment 13 codecolorist 2016-09-13 02:51:56 PDT
*** Bug 161899 has been marked as a duplicate of this bug. ***