Closed Bug 153032 Opened 23 years ago Closed 23 years ago

Implement almost-standards rendering mode

Categories

(Core :: Layout, defect, P1)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0.1

People

(Reporter: moz, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: topembed+, Whiteboard: [Hixie-P1])

Attachments

(3 files, 3 obsolete files)

documentation, v 1.1
24.64 KB, text/html
Details
patch, v. 1.2
94.98 KB, patch
dbaron
: review+
waterson
: superreview+
jud
: approval+
Details | Diff | Splinter Review
Testcase showing whitespace problem
1.38 KB, text/html
Details
Spin-off from bug 151620#64 Doctypes with "transitional" should get rendered as in standards mode with a few exceptions: Foremost we wouldn't follow the CSS2 inline box model when images are embedded in a <td>. Then we probably would allow non-text/css-stylesheets and a few yet to be determined things.
(from bug 153047 -- sorry, didn't notice this bug had been filed until too late) As a compromise, I propose that we increase the granularity of our quirks/ standards modes. I propose we use the following three modes: Standards -- default rendering mode, follows the specs as best we can. Triggered by all unrecognised DOCTYPEs, all Strict DOCTYPEs, all text/xml content, and all documents with internal subsets. Transitional -- triggered for documents with Transitional DOCTYPEs, including: HTML 4.01 Transitional with URI XHTML 1.0 Transitional sent as text/html <!DOCTYPE html SYSTEM "http://www.ibm.com/data/dtd/v11/ibmxhtml1-transitional.dtd"> Acts the same as Standard mode, except for the two most common quirks: * use the compatability inline box model in certain cases * handle text/plain stylesheets as text/css Quirks -- triggered by documents using the DOCTYPEs that currently trigger our compatability mode, plus the following: <!doctype html public "-//w3c//dtd html 4.01 transitional//en"> Acts the same as our current compatability mode, including the use of the quirks.css stylesheet. This suggestion will increase the acceptance of our standard mode, while accepting that most transitional documents do not expect to use the fully standard inline box model, and accepting that many servers are not correctly configured to sent CSS files as text/css.
QA Contact: petersen → ian
Whiteboard: [Hixie-P1]
*** Bug 153047 has been marked as a duplicate of this bug. ***
*** Bug 153039 has been marked as a duplicate of this bug. ***
Blocks: 151620, 152959, 153035
Summary: Implement additional mode for transitional pages → Implement transitional rendering mode
No longer blocks: 153035
Targetting optimistically.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.1beta
I should have a patch later today.
I don't like the term transitional because it reminds people of DTDs.
Summary: Implement transitional rendering mode → Implement almost-standards rendering mode
well what goes between "quirks mode" and "standards mode"? "somewhat quirky mode"?
Quirks Mode Almost Standards Mode Full Standards Mode
Attached patch patch, v. 1.0 (#3) (obsolete) — — Splinter Review
This still needs quite a bit more testing, and perhaps also some more discussion.
Attached file documentation, v 1.0 (obsolete) —
This is a version of http://mozilla.org/docs/web-developer/quirks/doctypes.html updated according to the patch (and pointing to a different CGI script). We'd probably want to save the old version since it was a description of what was in use for a long period of time. I suspect there are also a few DOCTYPEs missing from the list.
Attachment #88755 - Attachment is patch: false
Attachment #88755 - Attachment mime type: text/plain → text/html
I like this patch, thank you very much. The w3c doesn't plan to any new xhtml/html standards with transitional/frameset DTDs, does it? Is there anything else than text/css-stylesheets and the inline box model we have to address? I would like to keep the differences between almost-standard and standard as small as possible.
As a note, that patch has identical text/css behavior in standards and almost-standards mode... The tests in nsCSSLoader.cpp should test against eCompatibility_FullStandards instead of eCompatibility_NavQuirks and flip the comparison sense.
I've compiled a fresh CVS pull with the patch applied and compared it to a few days old 1.0 branch build. I've taken a part of Hixie's URL list, most cases render correctly with the patch. I've put a question mark where I'm not sure why something renders different than I'd expect it. URI DOCTYPE w patch w/o patch http://www.alterion.com/ 4.0 strict problem problem http://perl9.tripod.com/doctype-error.html 4.01 tran w/uri ok problem http://pbskids.org/sagwa/characters/ 4.01 tran w/uri ok problem http://www.rit.edu/~wjl3191/daemons/ 4.01 tran w/uri ok problem http://home.arcor.de/yajirobi/ 4.01 tran w/uri problem (?)problem http://www.gigabee.com/ xhtml transition ok problem http://www.offenburg.de/ xhtml transition ok ok (?) http://siteware.ch/digichat/ xhtml transition problem (?)problem http://sites.canaan.co.il/ xhtml transition better (?) problem http://sjostedt.no/mozilla/doctype.php xhtml transition ok problem http://www.earthtone9.fire-bug.co.uk/debug xhtml transition ok problem http://www.kamborio.com/ xhtml transition ok problem http://www.jazzinberlin.de/ xhtml transition ok problem
Referring to comment 12: >>The w3c doesn't plan to any new xhtml/html standards with transitional/frameset DTDs, does it?<< Presumably not, since the W3C has released XHTML 1.1 and it just comes in one flavor. (It's basically a reformulation of XHTML 1.0 Strict. <
Re comment 13: That is intentional. choess convinced me not to do that, although I'm still considering making that change later...
Blocks: 115280
Blocks: 133347
Blocks: 152552
bzbarsky: Why are bug 152552 and bug 133347 blocked by this bug? They are/were (presumably) due to malformed html-comments which this bug doesn't address.
Comment on attachment 88755 [details] documentation, v 1.1 I've tested that the patch displays all of the DOCTYPEs listed here as described.
A detailed summary of the patch (attachment 88752 [details] [diff] [review]): * It moves the definition of the nsCompatibility enum into its own header file so we don't need to pull in nsIPresContext.h or something similar (and everything else that it drags in) into files that don't need all of those includes. * It splits the "Strict" mode in nsDTDMode and nsCompatibility into "AlmostStandards" and "FullStandards". There are two reasons I wanted new names : 1. I wanted the compiler to catch all the places that were using strict so that I could correct them. 2. I wanted new names that were less associated with the idea of the "Strict DTD", which is a separate idea. Standards mode is triggered by things other than a "Strict" DTD. * This removes the nsDTDMode argument from nsIContentSink::AddDocTypeDecl since it's no longer necessary due to the patch in bug 98218. There's no need to set the mode twice, and the mechanism that patch added is more reliable. * This changes a bunch of places where the quirks mode was treated as a boolean into an |nsCompatibility| enum value so that we can pass any of three values through. Where it is still used as a boolean, AlmostStandards and FullStandards map to the same value. These changes are associated mainly with the pres shell, the CSS loader, and nsIFrame::IsEmpty. I also changed nsHTMLDocument to store an nsCompatibility instead of an nsDTDMode. * It backs out the patch for bug 151620, and refixes that bug by... * It changes our behavior so that we use the quirks mode line-height algorithm in AlmostStandards mode. In all other respects, AlmostStandards mode is the same as FullStandards mode. (I was thinking of doing the non-text/css-stylesheets thing as well, but choess convinced me to change my mind, although I did propagate the nsCompatibility enum down to the CSS loader, where we would need to make the change.) * It changes the DOCTYPE parsing code so that: + ParseDocTypeDecl parses the system identifier into an out parameter. + A special case is added for the IBM system DOCTYPE (AlmostStandards mode). + The PubIDInfo struct has separate values for what to do with and without a system identifier, to simplify the code (since there are more modes now). + The behavior of XHTML transitional and XHTML frameset is added to the array of |PubIDInfo|s, and the behavior of HTML 4.01 transitional and frameset with a URI is changed to AlmostStandards. + It removes some always unused code in DetermineHTMLParseMode that modified the buffer if there was no DOCTYPE. (The only caller of |DetermineParseMode| throws away that modification.) + It no longer checks for an XML-declaration, which is bogus in text/html anyway and which would mess up our new behavior of choosing AlmostStandards for XHTML Transitional.
Comment on attachment 88752 [details] [diff] [review] patch, v. 1.0 (#3) r=karnaze, it looks good. Is there any chance of updating viewer so that it can force almost standards mode?
Attachment #88752 - Flags: review+
That stuff that viewer did to force the mode never really worked (it only affected some things that are affected by the mode, which is quite misleading), and I took out the remnants of what supported it a while back. If we want it, it should be rewritten to hook into the mode setting code in nsParser.cpp, so that it's really doing what would happen if the mode selection were different.
Although this patch has the undesirable effect of pushing off the type of evangelism we have to do, it has the much more desirable effect of giving us a simple fix to evangelize ("switch to transitional DOCTYPEs and your layout will be fine") as well as covering the majority of potential problem cases before they actually become problems. I very much prefer this patch as described to the other proposed patches we've seen so far (cf. bug 151620). Let's give this a spin and see if it's a keeper-- we can worry about what to call this new mode once we're sure it's staying.
Arthur, see comment 1 of this bug. Those pages use the IBM doctype in question. Depending on what happens in this bug, those will either be fixed or stay evang bugs. Hence the tracking dependency. David, could you update the "list of quirks" docs to include the list of differences between standards and almost standards? In nsRange.cpp, could we add a "default:" statement to that switch that asserts on unknown DTD modes? Just in case? In nsHTMLContentSink.cpp I would be happier with explicitly having |case eDTDMode_quirks| in the switch and again asserting in the |default| case (this is in WillBuildModel()). Or do we actually see eDTDMode_unknown and eDTDMode_autodetect in this case statement? If so, ignore me. ;) Could we rename the CSSLoader compat mode setter to SetCompatibilityMode? In nsCSSStyleSheet.cpp: > + // case sensitivity: bug 93371 is indented too much, methinks > - nsCompatibility quirkMode = eCompatibility_Standard; > + nsCompatibility quirkMode; Why the change? In nsParser.cpp: > - // Why do this? If it weren't for this, |aBuffer| could be > - // |const nsString&|, which it really should be. > - aBuffer.InsertWithConversion( > - "<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01 Transitional//EN\">\n", > - 0); Does editor depend on that or something like that? That would be my first guess.. CVS blame is being unhelpful as to why this is present (you seem to have introduced it in rev 3.263 of nsParser.cpp... nsLineLayout: Are these objects temporary? If not, is adding the extra member really justified? nsTableCellFrame.cpp: > + PRInt32 pixHeight = (eCompatibility_NavQuirks != compatMode) ? 1 : 2; The use of "!=" here is confusing, imo... I'd use "==" and swap the "1" and "2". I'd also put |compatMode| first in the comparison, but that's just me. nsTableFrame.cpp: > + if ((eCompatibility_NavQuirks != mode) || aInnerBorderOnly) { Again, I'd do |mode != eCompatibility_NavQuirks| (easier to read, I think). Other than that, looks pretty good!
Boris, at one time, there used to be a coding standard of placing constants on the lhs of comparisons (e.g. eCompatibility_NavQuirks == mode), to catch errors (e.g. using = instead of ==).
really? wow. can we reinstate that policy? [I know, it won't happen, but..]
Attached patch patch, v. 1.1 (obsolete) — — Splinter Review
Revised patch, based on Boris's comments. > David, could you update the "list of quirks" docs to include the list > of differences between standards and almost standards? I will when I update the other docs. > In nsRange.cpp, could we add a "default:" statement to that switch > that asserts on unknown DTD modes? Just in case? Done. > In nsHTMLContentSink.cpp I would be happier with explicitly having |case > eDTDMode_quirks| in the switch and again asserting in the |default| case (this> is in WillBuildModel()). Or do we actually see eDTDMode_unknown and > eDTDMode_autodetect in this case statement? If so, ignore me. ;) I'd rather just leave it as-is. Who knows what we see... > Could we rename the CSSLoader compat mode setter to SetCompatibilityMode? Done. > In nsCSSStyleSheet.cpp: > > + // case sensitivity: bug 93371 > is indented too much, methinks Done. > > - nsCompatibility quirkMode = eCompatibility_Standard; > > + nsCompatibility quirkMode; > Why the change? It's guaranteed to be set by the next line. > In nsParser.cpp: > > > - // Why do this? If it weren't for this, |aBuffer| could be > > - // |const nsString&|, which it really should be. > > - aBuffer.InsertWithConversion( > > - "<!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01 Transitional//EN\">\n", > > - 0); > > Does editor depend on that or something like that? That would be my first > guess.. CVS blame is being unhelpful as to why this is present (you seem to > have introduced it in rev 3.263 of nsParser.cpp... Nothing depends on it. The only caller of DetermineHTMLParseMode is DetermineParseMode, and the only caller of DetermineParseMode is nsParser::WillBuildModel, which constructed that string as a copy and doesn't do anything with it other than call FindSuitableDTD, which calls only nsIDTD::CanParse, and none of the implementations of CanParse care about the buffer when the mime type is text/html, which is the only case for which we call DetermineHTMLParseMode. > nsLineLayout: > > Are these objects temporary? If not, is adding the extra member > really justified? Yes, they're temporary (during reflow). > nsTableCellFrame.cpp: > > + PRInt32 pixHeight = (eCompatibility_NavQuirks != compatMode) ? 1 : 2;> > The use of "!=" here is confusing, imo... I'd use "==" and swap the "1" and "2". > I'd also put |compatMode| first in the comparison, but that's just me. > > nsTableFrame.cpp: > > + if ((eCompatibility_NavQuirks != mode) || aInnerBorderOnly) { > > Again, I'd do |mode != eCompatibility_NavQuirks| (easier to read, I think). I'm trying to stick to the original style of the modules in question.
Attachment #88752 - Attachment is obsolete: true
Comment on attachment 89028 [details] [diff] [review] patch, v. 1.1 > + PRInt32 pixHeight = (eCompatibility_NavQuirks != compatMode) ? 1 : 2; I still think this would be more readable as: PRInt32 pixHeight = (eCompatibility_NavQuirks == compatMode) ? 2 : 1; but either way r=bzbarsky
Attachment #89028 - Flags: review+
Attached patch patch, v. 1.2 — — Splinter Review
I see what you mean about the nsTableCellFrame.cpp change now. Fixed. Otherwise this patch is the same as the previous one (so I'll transfer the "has-review").
Attachment #89028 - Attachment is obsolete: true
Comment on attachment 89083 [details] [diff] [review] patch, v. 1.2 sr=waterson
Attachment #89083 - Flags: superreview+
Comment on attachment 89083 [details] [diff] [review] patch, v. 1.2 >+ switch ((resultFlags & PARSE_DTD_HAVE_SYSTEM_ID) >+ ? kPublicIDs[index].mode_if_sysid >+ : kPublicIDs[index].mode_if_no_sysid) { Break this into two lines. Other than than r=harishd for the parser changes.
Re comment 30: That's already three lines... but I'm guessing you mean you want the brace on a separate line?
Fix checked in 2002-06-25 14:15/16 PDT.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Keywords: adt1.0.1, mozilla1.0.1
Resolution: --- → FIXED
Whiteboard: [Hixie-P1] → [Hixie-P1][open1.0.1]
Transferring some of the keywords over from #151620 since we need to get this onto the Mozilla 1.01 branch. dbaron : Can you please request driver/adt approval for branch checkin...thanks
Keywords: nsbeta1+, topembed+
>Re comment 30: That's already three lines... but I'm guessing you mean you want >the brace on a separate line No I meant something like: selection = (resultFlags & PARSE_DTD_HAVE_SYSTEM_ID) ? kPublicIDs[index].mode_if_sysid : kPublicIDs[index].mode_if_no_sysid; switch(selection) { ... } but no big deal.
The patch works to fix layout problems with match.com and IBM.com. Awesome! Thanks to all who've debated and worked on this issue! Still, I have to bring up a problem. The patch does not seem to be triggering almost-standards mode with XHTML Transitional+URI DOCTYPEs sent as text/html. I used a local copy of http://www.zeldman.com/ to test this-- I commented out his rule making images block-level, which of course returns them to inline. I've double-checked that the document is being sent as text/html from his server, and is being treated as such when loaded from my hard drive. Without the block-image rule, the "Daily Report" section at the top of the page spreads apart due to Mozilla using using standards-mode line layout. If you check the source, he is using HTML 1.0 Transitional, so it looks like we missed a case... or else I misunderstood how we'd be handling such cases. Will we insist on treating any URI'ed DOCTYPE (that isn't HTML 4.0 or ealier) as standards mode? I hope not, since that wasn't what was listed in comment 1, but I'd like to be sure. Plus a nit: what should the new mode return in the case of 'javascript:alert(document.compatMode);'? It currently hands back 'CSS1Compat', which is slightly misleading. 'NearCompat'? "AlmostCompat'? There should be some kind of distinction.
Sorry for the spam, but a followup shows that simple testcases with XHTML1.0 Transitional+URI do in fact trigger almost-compat mode, so it seems like there's something about zeldman.com that leads to full standards-mode layout of the images in that table. Any guesses as to what it might be? We'll need to either update the patch to go to almost-compat in that case, or figure out what the site does that prevents it so we can document it.
Note that document.compatMode is there for compatibility with IE. Using values that IE does not use for it seems a little odd to me. I just checked the zeldman.com site. I don't have your local copy, but the version at http://www.zeldman.com/ is detected as "almost-standards" by the parser....
More testing has revelaed what I did. If I simply comment out the rule 'img {display: block}' then the layout is as expected for almost-Compat mode. If I do that and/or write a rule that states 'a img {display: inline;}' then the descender space appears underneath images in tables (and, presumably, elsewhere). Is this an expected result? I don't think I object to it, and in fact it could be highly useful in certain edge cases. I just want to be sure it isn't some sort of regression.
Page info does not seem to reflect the new mode. Filed Page Info bug 154461 and cc'd dbaron.
please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword.
Keywords: mozilla1.0.1 → mozilla1.0.1+
Attachment #89083 - Flags: approval+
This testcase shows that when in almost-Compat mode, if there is whitespace in a cell along with an image, the cell is laid out in full compatibility mode. The patch only works so long as there's no whitespace at all in the cell. This is not how quirks mode behaves when it comes to laying out cells. We need to be able to ignore the presence of whitespace for the purposes of triggering the almost-Compat behavior-- and obviously we're ignoring it somewhere, because there is no space to either side of the images in the row that has whitespace-containing cells.
Sorry-- reopening bug due to problem shown in attachment 89403 [details].
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
HTML 4.0 (not 4.01) with system ID is triggering the quirks mode only because of bug 22274. Now that there's the almost standards mode, should Mozilla be consistent with IE or with the way Mozilla has been (and has been documented) for the past two years? It is probably safer to be consistent with earlier versions of Mozilla.
I filed bug 154683 for the issue mentioned in comment 43.
Marking as fixed again. We display attachment 89403 [details] exactly the same way as WinIE does, and pretty much the same way as Netscape 4.x (which draws the gap but doesn't manage to put the red background color in it). If it requires something you believe is a bug to do that, then we might want to consider fixing it in full standards mode, but not quirks mode and perhaps not even almost-standards mode.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
adding adt1.0.1+. Please check this in today.
Keywords: adt1.0.1 → adt1.0.1+
Fix checked in to branch, 2002-06-27 23:19/20 PDT.
Keywords: mozilla1.0.1+ → fixed1.0.1
Whiteboard: [Hixie-P1][open1.0.1] → [Hixie-P1]
Target Milestone: mozilla1.1beta → mozilla1.0.1
Re comment 45: I stand corrected, and accept that the patch's behavior is actually more desirable as it is than if we ignored whitespace as I suggested we should in comment 41. I'm verifying the bug, with thanks to everyone for their patience while I picked nits that should have been left alone.
Status: RESOLVED → VERIFIED
Is this considered stable enough to be documented? Should evang bugs that go away with this feature be marked FIXED, WFM or INVALID?
It is documented at http://mozilla.org/docs/web-developer/quirks/ . I don't know about the procedure for evang. bugs.
evang bugs should probably be worksforme.
*** Bug 149348 has been marked as a duplicate of this bug. ***
emeyer - did you verify this on the branch (since you marked this bug verified)? If so, pls change the "fixed1.0.1" keyword to "verified1.0.1" which is how we're handling the bug markings for branch. We've been using the "verified" status of the bug to track trunk verifications. Thanks
Is there a spinoff bug for discussing whether text/plain stylesheets should be accepted in AlmostStandards mode? It's mentioned in several comments on this bug but no spinoff bug is suggested. I'm curious also as to choess's reasoning for not wanting to do this...
Verified on branch; updating keywords to match. I'm curious too: is there a bug about adding 'text/plain' handling to "almost standards" mode? Or can we know the reasoning against it?
Keywords: fixed1.0.1 → verified1.0.1
There isn't a bug that I know of. The reasoning against it is that being strict with MIME types is very important for future-compatibility.
'What? Standard? Is that all it's got to say? Standard! One word! Ford shrugged. 'Well, there are a hundred billion doctypes on the Web, and only a limited amount of code in the browser,' he said, 'and no one knew much about Transitional of course.' 'Well for God's sake I hope you managed to rectify that a bit!' 'Oh yes, well I managed to transmit a new patch off to the developer. He had to trim it a bit, but it's still an improvement.' 'And what does it say now?' asked Arthur. 'Mostly standard,' admitted Ford with a slightly embarrassed cough.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: