Closed Bug 211030 Opened 21 years ago Closed 21 years ago

Incorrect hack in nsGenericHTMLElement.cpp breaks scroll model

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: erik, Unassigned)

Details

Attachments

(4 files, 3 obsolete files)

Same test case again but with HTML border and margin to make it clearer (IE6 in CSS1Compat)
(deleted), text/html
Details
Same with IE6 in BackCompat mode
(deleted), text/html
Details
Modified the test case to give you a feel for how IE6 treats the test case
(deleted), text/html
Details
little tweak
(deleted), patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review
The hack that maps the view port scroll values to document body are incorrect and breaks the scoll model. See http://bugzilla.mozilla.org/show_bug.cgi?id=62536#c122 The incorrect code can now be found at: http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#1010
This was done quite purposefully to be compatible with IE when getting the body's scroll info (since in IE the body corresponds to the viewport as far as any scrolling-related info is concerned). Could you please clearly describe the problem (like attach a small testcase that gets some scroll position and where Mozilla is "buggy")? We also need to determine the exact behavior of IE5 and IE6, the latter in both standards and quirks mode. If you could test those, that would be very much appreciated....
I'm well aware that this was added to be /compatible/ with IE. The problem is that it makes Moz compatible with IE in Quirks mode and incompatible with IE in Strict mode. In IE standards mode the HTML element is the canvas element just like in Mozilla. I think it makes more sense to be compatible with IE6 strict mode since both IE and Moz use the HTML as the canvas here. Making Moz behave like IE5 when the HTML element is still the canvas element does not really make sense and it breaks the logic and consistency behind scrollWidth/Height (and clientWidth/Height). I think it would makes sense to remove this hack and if any site breaks due to this make that into an evangelism bug. Test case coming up
Note how the scroll model still makes sense in IE since the BODY is now the scrolled canvas element.
Is there not to do what IE does and only have the hack in quirks mode?
Of course it is better than nothing to at least remove the hack in standards mode. Still I would prefer that these properties are correct to the element they do reflect; No hacks and no special cases which does not follow the rules.
Since this is an IE-compatibility property, it should be IE-compatible. Period. There is no "correct" behavior for this property other than "do what IE does".
If the goal is to make it IE compatible than it is fine to keep the hack when in quirks mode. Note that to make it compatible with IE the BODY should be the document canvas when in quirks mode. Should I file another bug for this?
Well, doing that, while making these properties more compatible, would break other things, including correct CSS rendering. So "no".
I can understand that (not changing the quirks mode to match IE's incorrect box model). I guess a was just a bit too provocative ;-) (and unclear, because I would rather have an incorrect scroll model for quirks mode than an incorrect box model).
Hmm... one more issue here. In Mozilla, the <html> content object's layout box is NOT scrollable. The scrollable thing (the viewport) is actually outside the <html> entirely.... Do you build, by any chance? Would you be able to test a patch and let me know how it compares to IE when getting scroll info on <body> and <html> ?
This makes it clear (mostly for my own need) that the HTML is not the scrollable viewport at all.
Attachment #126707 - Attachment is obsolete: true
Attached file Same with IE6 in BackCompat mode (deleted) —
Attachment #126708 - Attachment is obsolete: true
Now that I think I understand how the scroll model is supposed to work for the body, html and the viewport I'm not really sure what would be the best way to make this compatible with IE and still be usable for Mozilla developers. Feel free to mark this as invalid. Btw. I checked Opera 7.x and the following relation can be found window.pageXOffset = document.documentElement.scrollLeft = document.body scrollLeft // same for Y I don't know if this is better? Note that I'm totally satisfied with how doc.docEl/body.scrollWidth/Height work. docEl.clientWidth/Height is still wrong but one thing at a time. (I haven't built Mozilla before but maybe now is the time to invest the time to figure out how to do this. I'll invest a few hours in this after work this evening.)
CSS2.1 will (probably) say that the 'overflow' property on the <body> node in HTML (not XHTML) _may_ be used to refer to the _viewport_.
Erik, could you write up a description of IE's behavior, possibly? Sorry to be asking all these questions, but I have no IE available to test with, so I'm sort of trying to feel out what exact behavior we are emulating....
Ok. This is how IE behaves. In IE6 standards mode the HTML element is the canvas/viewport. The HTML element has a fixed size so it takes up the entire browser viewport. The scrolling is done using the CSS overflow property. This works exactly like any element (except html/body) in Gecko so setting a border on the HTML element puts a border outside the scroll bars. Margins are ignored on the HTML element. In IE4-IE55 the BODY element is the canvas/viewport. This works exactly like above except that CSS for the HTML is ignored. Putting a border on the BODY puts the border outside the scroll bars. Margins on the body are treated as an extra padding on the body element (!) (padding 10px and margin 10px gives padding 20px) When it comes to the scroll model there are no special cases in IE. There isn't any need since the HTML (BODY in IE4-IE5.5) is the scrollable viewport with a known width and height limited by the browser viewport. This all means that setting document.documentElement.scrollTop scrolls the browser viewport in IE6 (and document.body.scrollTop in IE4-IE55). The reason for all these incompatibilities is of course that IE does not follow CSS2 when it comes to the viewport.
Changed the test case to make it work (almost) identical in Mozilla as in IE6 using the following css: :root { margin-left: 0 !important; margin-right: 0 !important; margin-top: 0 !important; margin-bottom: 0 !important; width: 100%; height: 100%; overflow: auto; -moz-box-sizing: border-box; } This was just done to give you a feel for how IE6 treats the HTML element as the viewport.
Attached patch Proposed patch (obsolete) (deleted) — — Splinter Review
This should make things a little more sane, I think....
Comment on attachment 126765 [details] [diff] [review] Proposed patch Erik, if you could test this, that would be great...
Attachment #126765 - Flags: superreview?(jst)
Attachment #126765 - Flags: review?(jst)
This looks good. I'm right now reading through the docs on how to build Moz.
Comment on attachment 126765 [details] [diff] [review] Proposed patch r+sr=jst
Attachment #126765 - Flags: superreview?(jst)
Attachment #126765 - Flags: superreview+
Attachment #126765 - Flags: review?(jst)
Attachment #126765 - Flags: review+
Patch checked in; if people could let me know what remains to be done on this bug to make life better, that would be good.
I disagree with doing anything like this in standards mode. It's just wrong. Can't we do this as just a quirk?
This is now only done in quirks mode: + if ((InNavQuirksMode(doc) && mNodeInfo->Equals(nsHTMLAtoms::body)) || + mNodeInfo->Equals(nsHTMLAtoms::html)) {
The comment near that test suggests otherwise, and a quick test suggests that that logic would be true when not in quirks mode if HTML is the node: if (false && false || true) => this is true
Sorry, I misinterpreted the parenthesis (did not look close enough). I kind of agree with Hixie that no quirks should be done in non quirks mode. The question then is what is the best solution to this? Standards mode: No quirk Quirks mode: Map viewport to BODY or HTML? Can Mozilla be in quirks mode when IE6 is not? If not, then the answer is to map it to the BODY. Note that O7 maps the viewport to both BODY and HTML at the same time
Um. The "quirk" here is the existence of this function at all. As I said before, this function is present solely for IE compat; there is no standard describing it past "do what IE does". And no, we're not removing it in standards mode, sorry. Also, the mapping is _not_ "viewport mapped to something", but "something mapped to viewport". In other words, when asking for the scroll properties of "something", you get the scroll properties of the viewport. With that patch in, if you ask for the scroll properties of <html>, you get those of the viewport. If you ask for the scroll properties of the <body> _and_ you are in quirks mode, you also get the viewport. Note that there is no way to ask for the scroll properties of the viewport per se, using this API, since there is no element corresponding to it. One change I would be willing to make is to not map <html> to viewport in quirks mode (because I can see that breaking pages in quirks mode that walk up and collect scroll info and would double-count the viewport with this patch as is). Please let me know if this needs doing....
Agreed. I'm aware that the element asks the viewport and not the other way around. I just wasn't chosing my words correctly. I do believe that it would be better to map the body in quirks (and not html) and map html in standards (and not body). Just like you said some scripts sum up the scroll values and mapping both at the same time will give wrong result. - if ((InNavQuirksMode(doc) && mNodeInfo->Equals(nsHTMLAtoms::body)) || - mNodeInfo->Equals(nsHTMLAtoms::html)) { + if ((InNavQuirksMode(doc) && mNodeInfo->Equals(nsHTMLAtoms::body)) || + (!InNavQuirksMode(doc) && mNodeInfo->Equals(nsHTMLAtoms::html))) { Maybe there is a better way of expressing not in quirks mode. (I still agree with Hixie about the issue of this in standards mode but I'm willing to accept what we have so far and I do understand that compatibility with IE has a high priority.)
(No need to cc me, I'm the QA contact.) Ok, the exact text from the internal draft of 2.1 is: : HTML UAs may apply the overflow property from the BODY or : HTML elements to the viewport. This only applies to HTML, not XHTML, but it does suggest that we can do it in standards mode as well. So I propose that: In quirks mode, we map viewport to <body>. In standards mode with HTML, we map viewport to <html>. In XML mode, we don't map the viewport to any element. How does that sound?
nevermind any of my comments on this bug, i misunderstood what it was about.
Attached patch little tweak (deleted) — — Splinter Review
As discussed.
Attachment #126765 - Attachment is obsolete: true
Attachment #126924 - Flags: superreview?(jst)
Attachment #126924 - Flags: review?(jst)
Comment on attachment 126924 [details] [diff] [review] little tweak if ((InNavQuirksMode(doc) && mNodeInfo->Equals(nsHTMLAtoms::body)) || - mNodeInfo->Equals(nsHTMLAtoms::html)) { + (!InNavQuirksMode(doc) && mNodeInfo->Equals(nsHTMLAtoms::html))) { How about a local boolean to hold the return value from InNavQuirksMode(doc)? Faster, and probablt less code too... r+sr=jst
Attachment #126924 - Flags: superreview?(jst)
Attachment #126924 - Flags: superreview+
Attachment #126924 - Flags: review?(jst)
Attachment #126924 - Flags: review+
Checked in.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
How about: if (InNavQuirksMode(doc) ? mNodeInfo->Equals(nsHTMLAtoms::body) : mNodeInfo->Equals(nsHTMLAtoms::html)) {
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: