Bug 143609

Summary: Consider implementing Document.scrollingElement
Product: WebKit Reporter: Rick Byers <rbyers>
Component: DOMAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, fred.wang, mathias, sam, simon.fraser, webkit-bug-importer, zcorpan
Priority: P2 Keywords: InRadar, WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 5991, 182053, 182241    
Attachments:
Description Flags
Patch simon.fraser: review+

Description Rick Byers 2015-04-10 09:09:54 PDT
Document.scrollingElement would always return 'body' in WebKit's (non-spec-compliant) implementation, until bug 106133 is fixed.  Today some frameworks are forced to rely on a UA check to try to predict this behavior.  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.

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.
Comment 1 Mathias Bynens 2015-04-11 03:50:45 PDT
Polyfill: https://github.com/mathiasbynens/document.scrollingElement
Comment 2 Benjamin Poulain 2015-04-12 00:45:28 PDT
I saw your email on www-style. Thanks for filing a bug report here.

I'd rather not have scrollingElement and just fix the damn bug :)
Comment 3 Simon Pieters (:zcorpan) 2015-04-13 06:06:10 PDT
Even when the bug is fixed in WebKit and Blink, scrollingElement still makes sense to use for libraries that want to support being used in quirks mode.
Comment 4 Benjamin Poulain 2015-04-13 12:37:46 PDT
(In reply to comment #3)
> Even when the bug is fixed in WebKit and Blink, scrollingElement still makes
> sense to use for libraries that want to support being used in quirks mode.

Tell me more. How would you use scrollingElement in a way that can't be covered by existing APIs?
Comment 5 Rick Byers 2015-04-13 12:39:27 PDT
I'd rather just fix the damn bug too.  The problem is that we're finding lots of sites that have been forced to guess at the scrollingElement using a UA check (here's an example: https://github.com/google/closure-library/blob/32365aba43acb36c5d693256ef5d4dbe3bddddfe/closure/goog/dom/dom.js#L632, and Facebook had something similar).  I need some good/simple/reliable guidance for those sites to migrate to the spec-compatible behavior.  After a number of failed attempts, I don't see any path for switching blink to the correct behavior without first adding such an API.  Thoughts?
Comment 6 Benjamin Poulain 2015-04-13 12:50:44 PDT
(In reply to comment #5)
> I'd rather just fix the damn bug too.  The problem is that we're finding
> lots of sites that have been forced to guess at the scrollingElement using a
> UA check (here's an example:
> https://github.com/google/closure-library/blob/
> 32365aba43acb36c5d693256ef5d4dbe3bddddfe/closure/goog/dom/dom.js#L632, and
> Facebook had something similar).  I need some good/simple/reliable guidance
> for those sites to migrate to the spec-compatible behavior.  After a number
> of failed attempts, I don't see any path for switching blink to the correct
> behavior without first adding such an API.  Thoughts?

The problem here is the UA check for detecting behavior.

If it is just a matter of feature detection, one could change scrollTop/Left on body, observe which viewport has been modified, then restore the scroll position.

What I do not like about "scrollingElement" is that it looks like a temporary hack. It goes against keeping the platform simple.
Comment 7 Rick Byers 2015-04-13 13:35:52 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > I'd rather just fix the damn bug too.  The problem is that we're finding
> > lots of sites that have been forced to guess at the scrollingElement using a
> > UA check (here's an example:
> > https://github.com/google/closure-library/blob/
> > 32365aba43acb36c5d693256ef5d4dbe3bddddfe/closure/goog/dom/dom.js#L632, and
> > Facebook had something similar).  I need some good/simple/reliable guidance
> > for those sites to migrate to the spec-compatible behavior.  After a number
> > of failed attempts, I don't see any path for switching blink to the correct
> > behavior without first adding such an API.  Thoughts?
> 
> The problem here is the UA check for detecting behavior.
> 
> If it is just a matter of feature detection, one could change scrollTop/Left
> on body, observe which viewport has been modified, then restore the scroll
> position.

That might result in a noticeable visible artifact (especially for code that's invoking such 'find me the scrolling element' logic regularly).  If you plan to cache the result then you need to also force the page to be scrollable somehow during the test.  The polyfill referenced above addresses all these issues by creating a temporary iframe to run the test, and as a result is starting to get a little complex (hard to argue that it's a good drop in replacement for the simple UA check folks are doing).  But I can see the argument saying that's a better path forward.

> What I do not like about "scrollingElement" is that it looks like a
> temporary hack. It goes against keeping the platform simple.

I agree it would be nicer if we could correct our mistakes only through simplifying the platform API surface area.  But breaking changes (even bug fixes) are really hard to do.  Personally I'm willing to add simple, well-defined APIs when it simplifies the platform mental model even while adding more surface area.  FWIW I expect the presence of this API to simplify the spec text around all this dramatically, as it has simplified our implementation in blink - crrev.com/1075393002 (both of course could have been done by introducing the concept without exposing it).

Anyway, pragmatically it feels to me like our choices are to add an API like this to answer the exact question developers are attempting (and failing) to guess or to stick with the status quo where browsers that pretend to be WebKit in their UA (now including MS EdgeHTML) have to have a different implementation than those that don't.  I'm happy to be proven wrong if someone else wants to pick up the torch of trying to get this platform wart fixed a different way - it looks like bug 106133 hasn't had any love in awhile :-(

Also there is the minor argument Simon alluded to which says it's simpler and more reliably for library developers to use scrollingElement than to figure out they need to determine whether or not they're in quirks mode.  But I agree that's a pretty minor simplicity benefit, not enough to justify this API on it's own.
Comment 8 Benjamin Poulain 2015-04-13 14:17:45 PDT
What is Mozilla's take on this?
Comment 9 Rick Byers 2015-04-13 14:23:48 PDT
(In reply to comment #8)
> What is Mozilla's take on this?

Looks like they plan to implement it: https://bugzilla.mozilla.org/show_bug.cgi?id=1153322
Comment 10 Benjamin Poulain 2015-04-13 14:33:26 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > What is Mozilla's take on this?
> 
> Looks like they plan to implement it:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1153322

I am never a fan of adding something that can easily be implemented in JavaScript. But if both Mozilla and Chrome want the added complexity...let's do it.
Comment 11 Radar WebKit Bug Importer 2015-05-06 15:20:57 PDT
<rdar://problem/20845213>
Comment 12 Sam Weinig 2015-05-07 17:30:24 PDT
Created attachment 252661 [details]
Patch
Comment 13 Simon Fraser (smfr) 2015-05-07 17:31:58 PDT
Comment on attachment 252661 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=252661&action=review

> Source/WebCore/ChangeLog:3
> +        Consider implementing Document.scrollingElement

No longer considering!
Comment 14 Sam Weinig 2015-05-07 18:00:21 PDT
Committed r183967: <http://trac.webkit.org/changeset/183967>