Bug 5855 - REGRESSION: revert SGML comment parsing fix (comment parsing causes most of usbank.com page to be missing)
Summary: REGRESSION: revert SGML comment parsing fix (comment parsing causes most of u...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P1 Major
Assignee: Dave Hyatt
URL: https://access.usbank.com/
Keywords: HasReduction, InRadar, Regression
Depends on:
Blocks:
 
Reported: 2005-11-28 13:54 PST by Ken Sayward
Modified: 2006-06-10 12:50 PDT (History)
3 users (show)

See Also:


Attachments
Screenshot of what the page should look like (60.83 KB, image/png)
2005-11-28 14:03 PST, Ken Sayward
no flags Details
Screenshot of Safari (incorrect) rendering (67.54 KB, image/png)
2005-11-28 14:04 PST, Ken Sayward
no flags Details
Minimal Test Case (538 bytes, text/html)
2006-02-11 01:49 PST, Ken Sayward
no flags Details
Screenshot of minimal test page in Safari 1.3.2 (118.11 KB, image/jpeg)
2006-03-20 10:10 PST, Michael Dreimiller
no flags Details
revert SGML comment parsing fix (27.25 KB, patch)
2006-04-27 22:04 PDT, Alexey Proskuryakov
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ken Sayward 2005-11-28 13:54:42 PST
This bank website 'should' have a global navigation area around the initial login form.  Only the core login 
form is rendered in Safari.  Compare to the correct rendering in Opera or IEWin 5.5sp2. There are further 
errors deeper within the site, affecting core functionality, but may be related to the same issue. This is the 
easiest to reproduce without an account with the bank.

In a local copy of the page, if I remove the <!doctype ... > tag, the page renders perfectly. This problem 
did not exist prior to the Safari build prior to OS 10.4.3 update.  Latest nightly build as of 10/27 still has 
the problem.

Page renders incorrectly in Camino 0.8.4 and Firefox 1.5rc3 as well, but renders correctly with Opera 8.02 
(2148)
Comment 1 Ken Sayward 2005-11-28 14:03:34 PST
Created attachment 4837 [details]
Screenshot of what the page should look like

Correct Rendering was from Opera.
Comment 2 Ken Sayward 2005-11-28 14:04:30 PST
Created attachment 4838 [details]
Screenshot of Safari (incorrect) rendering
Comment 3 Thomas Deniau 2006-02-10 02:14:56 PST
The DOCTYPE used is <!doctype html public "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3c.org/TR/html14/loose.dtd">, which doesn't exist. (the correct URL would be "http://www.w3.org/TR/html4/loose.dtd").

So this site is trying to use a non-existent version of HTML ; I'd classify that as an evangelism bug. Also note that Firefox's rendering (1.5.0.1) is the same as Safari's.
Comment 4 Ken Sayward 2006-02-10 03:14:35 PST
(In reply to comment #3)
> The DOCTYPE used is <!doctype html public "-//W3C//DTD HTML 4.01
> Transitional//EN" "http://www.w3c.org/TR/html14/loose.dtd">, which doesn't
> exist. (the correct URL would be "http://www.w3.org/TR/html4/loose.dtd").
> 
> So this site is trying to use a non-existent version of HTML ; I'd classify
> that as an evangelism bug. Also note that Firefox's rendering (1.5.0.1) is the
> same as Safari's.
> 

Yes, there also appears to be a problem related to the way the comments are included (long ----- seperators) ... attempts to get them to fix this got the usual "sorry, we only code for IEWin" ... unfortunate that IEWin actually renders the page as intended.  So, I guess I have to keep Opera around for this site, as it's the only Mac browser I've seen that works with this.
Comment 5 Alexey Proskuryakov 2006-02-10 14:51:31 PST
See also: http://ln.hixie.ch/?start=1137799947
Comment 6 Thomas Deniau 2006-02-10 15:00:58 PST
Replacing the DOCTYPE with a real HTML4 Transitional DOCTYPE doesn't fix the problem, which means that this is indeed a regression which appeared after the acid2 comments parsing fix (removing the DOCTYPE makes Safari switch to quirks mode, where that fix is not applied).
Comment 7 Ian 'Hixie' Hickson 2006-02-10 15:57:43 PST
Note that SGML comment parsing is gone from HTML5.

See: http://ln.hixie.ch/?start=1137799947&count=1

Comment parsing in strict mode in HTML5 is defined in:
   http://whatwg.org/specs/web-apps/current-work/#parsing
Comment 8 Ken Sayward 2006-02-11 01:10:00 PST
Simply removing all of the comment lines where the number of '-' chars are > 2, the page renders correctly, so I'm assuming that the issue is in the comment parsing.  I will work on a reduction to get a small test case for this.
Comment 9 Ken Sayward 2006-02-11 01:49:07 PST
Created attachment 6406 [details]
Minimal Test Case

Small test case showing missing rendering after a malformed comment line (I think ;-). Should display 2 lines of text.  the second line does not render when the comment is left in the code. Remove the comment, and the text renders fine.

Interestingly, if I remove the SCRIPT line in the HEAD section, the page renders correctly as well.  What the heck is that all about??
Comment 10 Alice Liu 2006-03-20 08:38:47 PST
I don't think this is a regression.  downgrading priority.  Ping me if you find out that it is.  This was also seen at http://www.clintonlibrary.gov/. <rdar://problem/4464323>
Comment 11 Michael Dreimiller 2006-03-20 10:10:34 PST
Created attachment 7191 [details]
Screenshot of minimal test page in Safari 1.3.2

Here is a screenshot of Safari 1.3.2 (Panther 10.3.9) rendering the minimal test page that Ken submitted . The test page renders correctly. So, it appears that the bug is a regression.
Comment 12 Alexey Proskuryakov 2006-03-20 21:46:02 PST
I can also confirm that this is a regression since 10.3.7 (that's what I have installed besides 10.4.5). I don't think there is any reason to doubt the reporter's words that it happened between 10.4.2 and 10.4.3.

Please note that the usbank.com page has changed, and no longer has this problem.
Comment 13 Ken Sayward 2006-03-20 21:59:29 PST
Alexey is correct; the USBank home page has been fixed for proper commenting. However, deeper within that site, there are several pages with the problem still there (yes, I've let them know which ones ;-).

The minimal test case is still valid, and correctly represents the problem on the USBank site.
Comment 14 Alexey Proskuryakov 2006-04-27 22:04:37 PDT
Created attachment 8013 [details]
revert SGML comment parsing fix

This patch reverts one change made for acid2, <http://weblogs.mozillazine.org/hyatt/acid6.txt>. The local copies of acid2 are updated to the current version (no SGML comment parsing, some additional whitespace); fast/parser/comments.html results now match WinIE.
Comment 15 Alexey Proskuryakov 2006-04-27 22:06:25 PDT
See also: bug 8626.
Comment 16 Darin Adler 2006-04-28 09:30:55 PDT
This looks great to me, but Hyatt should review. I hope he gives it a prompt review+!
Comment 17 Dave Hyatt 2006-04-28 11:45:34 PDT
Comment on attachment 8013 [details]
revert SGML comment parsing fix

r=me
Comment 18 David Kilzer (:ddkilzer) 2006-06-10 12:50:03 PDT
Committed as revision r14106.

See also bug 9357.