WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
143609
Consider implementing Document.scrollingElement
https://bugs.webkit.org/show_bug.cgi?id=143609
Summary
Consider implementing Document.scrollingElement
Rick Byers
Reported
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.
Attachments
Patch
(8.41 KB, patch)
2015-05-07 17:30 PDT
,
Sam Weinig
simon.fraser
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Mathias Bynens
Comment 1
2015-04-11 03:50:45 PDT
Polyfill:
https://github.com/mathiasbynens/document.scrollingElement
Benjamin Poulain
Comment 2
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 :)
Simon Pieters (:zcorpan)
Comment 3
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.
Benjamin Poulain
Comment 4
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?
Rick Byers
Comment 5
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?
Benjamin Poulain
Comment 6
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.
Rick Byers
Comment 7
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.
Benjamin Poulain
Comment 8
2015-04-13 14:17:45 PDT
What is Mozilla's take on this?
Rick Byers
Comment 9
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
Benjamin Poulain
Comment 10
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.
Radar WebKit Bug Importer
Comment 11
2015-05-06 15:20:57 PDT
<
rdar://problem/20845213
>
Sam Weinig
Comment 12
2015-05-07 17:30:24 PDT
Created
attachment 252661
[details]
Patch
Simon Fraser (smfr)
Comment 13
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!
Sam Weinig
Comment 14
2015-05-07 18:00:21 PDT
Committed
r183967
: <
http://trac.webkit.org/changeset/183967
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug