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)
2.36 KB, text/html
Details
Same with IE6 in BackCompat mode
2.38 KB, text/html
Details
Modified the test case to give you a feel for how IE6 treats the test case
2.65 KB, text/html
Details
little tweak
948 bytes, 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

        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)
        — 
          — 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
        — 
          — 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: