Closed Bug 1153322 Opened 9 years ago Closed 8 years ago

Consider implementing Document.scrollingElement

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed
relnote-firefox --- 48+

People

(Reporter: rbyers, Assigned: smaug)

References

(
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

scrollingelement.diff
10.03 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
+more tests
11.11 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2334.0 Safari/537.36

Steps to reproduce:

Document.scrollingElement is designed to be a trivial API for the engine to tell JS exactly which element (body or documentElement) it considers to be the one that scrolls the viewport.  Implementation in Firefox will be trivial, but will help developers who need to support non-compliant engines (currently WebKit, blink and EdgeHTML) and hopefully enable migration of those engines.

See http://dev.w3.org/csswg/cssom-view/#dom-document-scrollingelement
And https://lists.w3.org/Archives/Public/www-style/2015Apr/0108.html

We're planning on shipping this API in blink ASAP to help ease the transition to spec-compliant scrollTop/scrollLeft behavior (https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/mJ5SlCj3CHc).  However we'd like some feedback from other vendors before we do so.
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Robert, you were in favor of this, right?

Seems pretty reasonable to me.
Flags: needinfo?(roc)
OK, so just to make sure we're on the same page here, the algorithm for document.scrollingElement for Firefox (and any other UA following the CSSOM draft for scroll*) should be as follows:

  if (!isQuirks()) {
    return document.documentElement
  }
  return document.querySelector("body");

This doesn't agree with the polyfill linked in comment 2 for this testcase:

  data:text/html,<script>document.documentElement.textContent = ""; document.documentElement.appendChild(document.createElement("frameset")); document.documentElement.appendChild(document.createElement("body"));</script>

for which I believe per CSSOM spec the scroll* of the <body> will match those of the viewport (and I've verified that in Firefox they do) but the polyfill returns null for document.scrollingElement.
Flags: needinfo?(rbyers)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ah, just found the spec at http://dev.w3.org/csswg/cssom-view/#dom-document-scrollingelement and looks like my interpretation is correct and the polyfill is wrong.
Er, the querySelector behavior is wrong too, since it would match an element named "BODY" or one that's not in the HTML namespace.  Need to be a bit more careful there.
I agree with the description in comment 4, and I've confirmed that my prototype implementation for blink behaves as you expect for your test case (both document.scrollingElement and document.body return the frameset element in blink, and it's scroll APIs match the window ones).

Note that I'm asking for a change to the spec for a somewhat related scenario (when both body and html are overflow:scroll): https://lists.w3.org/Archives/Public/www-style/2015Apr/0198.html.  Feedback appreciated.
Yeah. (document.querySelector("body") is also wrong if <body> is not a child of the root or if the root is not <html>, etc.)

Filed https://github.com/mathiasbynens/document.scrollingElement/issues/9
> https://lists.w3.org/Archives/Public/www-style/2015Apr/0198.html. 

Yeah, I saw that.

It complicates API implementation to some extent, because now it becomes layout-dependent, but it makes some sense.  I guess I can live with it, as long as we spec it clearly.

Speaking of which, the CSSOM spec for scrollTop is now not making sense to me.  What should .scrollTop on the <body> of a quirks-mode non-rendered (e.g. DOMParser-created) be?  Seems like it should be 0, but the spec is talking about the viewport (which one, exactly)?  I'd like to understand what the intended behavior of scrollTop here is before I can decide what to do for .scrollingElement.
Flags: needinfo?(zcorpan)
And in particular, cssom-view should really say _which_ object's scrollY is used for scrollTop as needed, and what to do when (if?) that object is null.
(In reply to Not doing reviews right now from comment #9)
> > https://lists.w3.org/Archives/Public/www-style/2015Apr/0198.html. 
> 
> Yeah, I saw that.
> 
> It complicates API implementation to some extent, because now it becomes
> layout-dependent, but it makes some sense.  I guess I can live with it, as
> long as we spec it clearly.

Oh that's a good point.  It might be nice (eg. from a perf perspective) if this API wasn't layout dependent.  At least (for performance) we can keep the dependency to only the quirks mode codepath.  I don't have a strong preference here - happy to debate further on www-style if you'd prefer the definition stay as-is.
Flags: needinfo?(rbyers)
Release Note Request (optional, but appreciated)
[Why is this notable]: New feature that should move to a standard implementation for this functionality across browsers.
[Suggested wording]: Implemented Document.scrollingElement to tell which element's scroll* attributes reflect the viewport scroll state
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
(In reply to Not doing reviews right now from comment #9)
> Speaking of which, the CSSOM spec for scrollTop is now not making sense to
> me.  What should .scrollTop on the <body> of a quirks-mode non-rendered
> (e.g. DOMParser-created) be?  Seems like it should be 0, but the spec is
> talking about the viewport (which one, exactly)?  I'd like to understand
> what the intended behavior of scrollTop here is before I can decide what to
> do for .scrollingElement.

I've tried to fix this: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28488
Flags: needinfo?(zcorpan)
I believe it would have been easier to fix the differences between Webkit/Blink and the W3C specifications about which element scrolls the "viewport" in Standards and Quirks modes instead of adding more quirks.

Up to now all browsers (except Webkit/Blink) have already agreed with the specs that:

- in Quirks mode the scrolling element is the "body"
- in Standards mode the scrolling element is the "documentElement"

Why change the specification and introduce new properties and new specs changes because one engine doesn't follow the specs ?

Currently Webkit/Blink (and derived browsers like Opera) have a bug that will scroll the "viewport" even when modifying the "scrollTop" property of a detached "body" element.

This strange detached "body" bug is now being fixed in Webkit:

    http://trac.webkit.org/changeset/182677

here is the bug report:

    https://bugs.webkit.org/show_bug.cgi?id=143651
That WebKit bug refers to a special case of multiple body elements.  The primary problem is https://bugs.webkit.org/show_bug.cgi?id=106133 which (along with an earlier version of the bug) has been around for 9 years with multiple failed attempts to fix it because too many sites rely on UA sniffing to expect the webkit behavior.

Our hope is that the new API will give us a transition path to fix blink and webkit.  The trade-offs here are non-obvious, but I can say that I have no intention of trying again to fix this bug in blink without something new to ease the transition (like this API).  See more debate in TAG review here: https://github.com/w3ctag/spec-reviews/issues/51
If the property is supposed to describe current browser differences, it seems like the spec for it should actually say what it should be tied to rather than describing a particular behavior, so that implementors keep document.scrollingElement matching the thing that it's supposed to match.

(This was discussed in the CSS WG meeting today, and it wasn't really clear to me that the spec is currently correct.  The spec could certainly use some more explanation of motivation for why the property exists.)
(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #16)
> If the property is supposed to describe current browser differences, it
> seems like the spec for it should actually say what it should be tied to
> rather than describing a particular behavior, so that implementors keep
> document.scrollingElement matching the thing that it's supposed to match.

It has a note to that effect. Is it not sufficient?

> (This was discussed in the CSS WG meeting today, and it wasn't really clear
> to me that the spec is currently correct.

Can you be more specific?

> The spec could certainly use some
> more explanation of motivation for why the property exists.)

The note tries to explain why the property exists. What is missing?

For reference, the current note is:

[[
Note: For non-conforming user agents that always use the quirks mode behavior for scrollTop and scrollLeft, the scrollingElement attribute is expected to also always return the HTML body element (or null if it does not exist). This API exists so that Web developers can use it to get the right element to use for scrolling APIs, without making assumptions about a particular user agent’s behavior or having to invoke a scroll to see which element scrolls the viewport.
]]
(In reply to Simon Pieters from comment #17)
> For reference, the current note is:
> 
> [[
> Note: For non-conforming user agents that always use the quirks mode
> behavior for scrollTop and scrollLeft, the scrollingElement attribute is
> expected to also always return the HTML body element (or null if it does not
> exist). This API exists so that Web developers can use it to get the right
> element to use for scrolling APIs, without making assumptions about a
> particular user agent’s behavior or having to invoke a scroll to see which
> element scrolls the viewport.
> ]]

I think this needs to not be in a note, but actually be a conformance requirement.

I also think the "without making assumptions about a particular user agent's behavior" should be a lot clearer that it's talking about non-conformance to other parts of the spec, and should describe that it's a transitional mechanism so that those user agents can fix their behavior.

I can't comment on the other things since the spec doesn't load right now (pending automatic generation, and has been for a few minutes), but it certainly wasn't clear enough to me while I was trying to read it during the teleconference.
This is fixed in Blink, WebKit, and Microsoft Edge
Keywords: dev-doc-needed
OS: Linux → All
Hardware: x86_64 → All
See Also: → http://code.google.com/p/chromium/issues/detail?id=475531, https://bugs.webkit.org/show_bug.cgi?id=143609, https://github.com/w3ctag/spec-reviews/issues/51
There's a bug in AMPhtml which returns document.body in their getScrollingElement_() method: <https://github.com/ampproject/amphtml/blob/c11ed15d75de2817eb138742f154ec9a498ff593/src/service/viewport-impl.js#L643> (reported via https://github.com/webcompat/web-bugs/issues/1793#issuecomment-170914657). 

I've filed a bug against AMP, but having document.scrollingElement would also solve the bug, FWIW.
This is scary stuff given that blink returns different value than what the spec says unless one enables some experimental stuff.
Assignee: nobody → bugs
We're trying hard to fix the long-standing WebKit bug here: https://dev.opera.com/articles/fixing-the-scrolltop-bug/ - tons of sites have come to depend on it when "WebKit" is in the UserAgent string.
I'm mostly worried that sites using scrollingElement end up relying on the behavior where it returns always body, since that is what Chrome does now.

        Attachment #8711186 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 8711186 [details] [diff] [review]
scrollingelement.diff

r=me, but might be worth adding a test that if in quirks mode we have two <body> elements in the <html> the first one is returned.

Also that in quirks if we have a <body> that's documentElement it is NOT returned.

        Attachment #8711186 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
ok, will add such test.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/246768182c61
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox47:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

    
    

    
  
What's preventing us from shipping this?
Flags: needinfo?(bugs)

    
    

    
  
Looks like we haven't seen pages broken because of this very scary change. I'll file a bug to enable the feature.
Flags: needinfo?(bugs)
Blocks: 1265032

    
    

    
  
Ritu, this feature was actually shipped in Firefox 48. See bug 1265032. So, can you please move it to the Firefox 48 release notes and also link it with https://developer.mozilla.org/en-US/docs/Web/API/Document/scrollingElement?

Sebastian
Flags: needinfo?(rkothari)

    
    

    
  
(In reply to Sebastian Zartner [:sebo] from comment #35)
> Ritu, this feature was actually shipped in Firefox 48. See bug 1265032. So,
> can you please move it to the Firefox 48 release notes and also link it with
> https://developer.mozilla.org/en-US/docs/Web/API/Document/scrollingElement?
> 
> Sebastian

Done, I've moved this relnote from 47 to 48 aurora release notes and added a link to dev.m.o

          relnote-firefox:
          47+ → 48+
Flags: needinfo?(rkothari)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: