Last Comment Bug 211030 - Incorrect hack in nsGenericHTMLElement.cpp breaks scroll model
: Incorrect hack in nsGenericHTMLElement.cpp breaks scroll model
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: general
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-06-29 05:00 PDT by Erik Arvidsson
Modified: 2013-04-04 13:53 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case showing the different scroll related properties for the documentElement and the body (2.12 KB, text/html)
2003-06-29 10:31 PDT, Erik Arvidsson
no flags Details
Same test case but forcing IE6 into BackCompat (Quirks) mode (2.14 KB, text/html)
2003-06-29 10:33 PDT, Erik Arvidsson
no flags Details
Same test case again but with HTML border and margin to make it clearer (IE6 in CSS1Compat) (2.36 KB, text/html)
2003-06-30 01:55 PDT, Erik Arvidsson
no flags Details
Same with IE6 in BackCompat mode (2.38 KB, text/html)
2003-06-30 01:56 PDT, Erik Arvidsson
no flags Details
Modified the test case to give you a feel for how IE6 treats the test case (2.65 KB, text/html)
2003-06-30 09:52 PDT, Erik Arvidsson
no flags Details
Proposed patch (1.75 KB, patch)
2003-06-30 10:51 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
Details | Diff | Splinter Review
little tweak (948 bytes, patch)
2003-07-02 10:35 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
Details | Diff | Splinter Review

Description Erik Arvidsson 2003-06-29 05:00:55 PDT
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
Comment 1 Boris Zbarsky [:bz] 2003-06-29 07:51:12 PDT
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....
Comment 2 Erik Arvidsson 2003-06-29 10:19:03 PDT
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
Comment 3 Erik Arvidsson 2003-06-29 10:31:47 PDT
Created attachment 126707 [details]
Test case showing the different scroll related properties for the documentElement and the body
Comment 4 Erik Arvidsson 2003-06-29 10:33:35 PDT
Created attachment 126708 [details]
Same test case but forcing IE6 into BackCompat (Quirks) mode

Note how the scroll model still makes sense in IE since the BODY is now the
scrolled canvas element.
Comment 5 Boris Zbarsky [:bz] 2003-06-29 11:54:59 PDT
Is there not to do what IE does and only have the hack in quirks mode?
Comment 6 Erik Arvidsson 2003-06-29 13:26:09 PDT
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.
Comment 7 Boris Zbarsky [:bz] 2003-06-29 13:28:18 PDT
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".
Comment 8 Erik Arvidsson 2003-06-29 13:36:07 PDT
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?
Comment 9 Boris Zbarsky [:bz] 2003-06-29 13:41:15 PDT
Well, doing that, while making these properties more compatible, would break
other things, including correct CSS rendering.  So "no".

Comment 10 Erik Arvidsson 2003-06-29 13:49:18 PDT
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).
Comment 11 Boris Zbarsky [:bz] 2003-06-29 22:43:52 PDT
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> ?
Comment 12 Erik Arvidsson 2003-06-30 01:55:55 PDT
Created attachment 126734 [details]
Same test case again but with HTML border and margin to make it clearer (IE6 in CSS1Compat)

This makes it clear (mostly for my own need) that the HTML is not the
scrollable viewport at all.
Comment 13 Erik Arvidsson 2003-06-30 01:56:58 PDT
Created attachment 126735 [details]
Same with IE6 in BackCompat mode
Comment 14 Erik Arvidsson 2003-06-30 02:07:13 PDT
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.)
Comment 15 Hixie (not reading bugmail) 2003-06-30 05:28:42 PDT
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_.
Comment 16 Boris Zbarsky [:bz] 2003-06-30 08:17:49 PDT
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....
Comment 17 Erik Arvidsson 2003-06-30 09:40:12 PDT
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.
Comment 18 Erik Arvidsson 2003-06-30 09:52:27 PDT
Created attachment 126760 [details]
Modified the test case to give you a feel for how IE6 treats the test case

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.
Comment 19 Boris Zbarsky [:bz] 2003-06-30 10:51:14 PDT
Created attachment 126765 [details] [diff] [review]
Proposed patch

This should make things a little more sane, I think....
Comment 20 Boris Zbarsky [:bz] 2003-06-30 10:52:01 PDT
Comment on attachment 126765 [details] [diff] [review]
Proposed patch

Erik, if you could test this, that would be great...
Comment 21 Erik Arvidsson 2003-06-30 11:14:43 PDT
This looks good. I'm right now reading through the docs on how to build Moz.
Comment 22 Johnny Stenback (:jst, jst@mozilla.com) 2003-07-01 14:57:11 PDT
Comment on attachment 126765 [details] [diff] [review]
Proposed patch

r+sr=jst
Comment 23 Boris Zbarsky [:bz] 2003-07-01 16:47:49 PDT
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.
Comment 24 Hixie (not reading bugmail) 2003-07-02 00:51:41 PDT
I disagree with doing anything like this in standards mode. 
It's just wrong. Can't we do this as just a quirk?
Comment 25 Erik Arvidsson 2003-07-02 01:26:02 PDT
This is now only done in quirks mode:

+    if ((InNavQuirksMode(doc) && mNodeInfo->Equals(nsHTMLAtoms::body)) ||
+        mNodeInfo->Equals(nsHTMLAtoms::html)) {
Comment 26 Hixie (not reading bugmail) 2003-07-02 01:37:08 PDT
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
Comment 27 Erik Arvidsson 2003-07-02 06:34:34 PDT
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
Comment 28 Boris Zbarsky [:bz] 2003-07-02 09:07:17 PDT
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....
Comment 29 Erik Arvidsson 2003-07-02 09:27:51 PDT
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.)
Comment 30 Hixie (not reading bugmail) 2003-07-02 09:49:06 PDT
(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?
Comment 31 Hixie (not reading bugmail) 2003-07-02 10:32:37 PDT
nevermind any of my comments on this bug, i misunderstood what it was about.
Comment 32 Boris Zbarsky [:bz] 2003-07-02 10:35:58 PDT
Created attachment 126924 [details] [diff] [review]
little tweak

As discussed.
Comment 33 Johnny Stenback (:jst, jst@mozilla.com) 2003-07-02 17:26:38 PDT
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
Comment 34 Boris Zbarsky [:bz] 2003-07-02 19:52:32 PDT
Checked in.
Comment 35 jag (Peter Annema) 2003-07-07 19:11:05 PDT
How about:

    if (InNavQuirksMode(doc) ? mNodeInfo->Equals(nsHTMLAtoms::body)
	                     : mNodeInfo->Equals(nsHTMLAtoms::html)) {

Note You need to log in before you can comment on or make changes to this bug.