Bug Tracker

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#7864 closed enhancement (patchwelcome)

Resolve "scrollable" root (body or html/documentElement)

Reported by: musicisair Owned by: musicisair
Priority: undecided Milestone: 1.6
Component: unfiled Version: 1.4.4
Keywords: Cc: dmethvin, danheberden
Blocked by: Blocking:

Description

Several places (6, I think) within jQuery perform a check similar to:

documentElement[scrollTop|scrollLeft] || body[scrollTop|scrollLeft]

Adding a flag in jQuery.support for "documentElementScrolls" (or something like that) would be helpful for developers as well as assist in keeping jQuery DRY.

fuse.js has a check for this located at [src/dom/features.js](https://github.com/jdalton/fusejs/blob/master/src/dom/features.js#L138) on line 138 (well, 156 really) which may be useful.

Change History (10)

comment:1 Changed 9 years ago by danheberden

Thanks for providing a bug report :) Could you provide some examples of where this is? I see https://github.com/jquery/jquery/blob/master/src/event.js#L516 and while there are other tests involving scrollTop or similar, they are in a different context

comment:2 Changed 9 years ago by danheberden

Owner: set to musicisair
Status: newpending

comment:3 Changed 9 years ago by musicisair

Status: pendingnew

I just searched for "body.scroll" in the jQuery 1.4.4 source code. So, if changes have been made to the 1.4.4 scrollTop/Left parts in the repository my numbers will be off.

Either way, it would be nice to be able to check which element is scrollable with a simple var scrollEl = $.support.documentElementScrolls ? "$("html") : $("body");.

comment:4 Changed 9 years ago by danheberden

musicisair, I wasn't just referring to the line numbers, but rather, the actual content of the source. Are you looking at the latest source code in the repo, or just the 1.4.4 release from the website?

As for adding to support, to me, it seems trivial to add something like that to core that could easily be done outside of the application or as a plugin.

comment:5 Changed 9 years ago by dmethvin

Status: newpending

Usually the guiding factor for whether something is added to jQuery.support is whether it is required in the core and provides a DRY improvement or performance boost. We often get requests to add support checks for features like SVG for example, but if we do that we'll be adding load-time overhead to every page for the few who need SVG detection. Core doesn't need SVG detection.

With those guidelines in mind, does this still make sense? Can you provide specific line references to the core code that would be improved?

comment:6 Changed 9 years ago by musicisair

Status: pendingnew

I was just looking at the 1.4.4 release. It looks like the http://code.jquery.com/jquery-git.js version is the same (in regards to the scroll parts).

I'm just assuming the following sections can be DRYed with the $.support check. I haven't done any testing whatsoever. They first check if the documentElement has a scroll position and then check if the document.body has a scroll position.

  • Lines 2336-2337
  • Lines 7883-7884
  • Lines 7958-7959

(http://code.jquery.com/jquery-git.js)

Also, finding the document's main scrolling element is not a trivial task. You must:

  • insert a block element to the document with a height greater than the document window (so we can scroll it).
  • cache the initial scrollTop position
  • scroll with ++scrollElem.scrollTop where scrollElem is either body or documentElement
  • check if scrollElem.scrollTop value has changed
  • reset the element.scrollTop to the original value
  • then finally remove the block element.

Since jQuery is already performing the 2 most expensive operations (adding and removing a div) for the other support tests it should be relatively easy to add this check.

FYI: the reason we can't just do $("body,html").animate({"scrollTop",500},1000); is because Opera scrolls BOTH elements (or it used to, its been a while since I've tested it). It is also not all that efficient to try to animate both elements unnecessarily. :-)

Last edited 9 years ago by musicisair (previous) (diff)

comment:7 Changed 9 years ago by addyosmani

Cc: dmethvin danheberden added

I'm with danheberden on this one - although there are occasionally features and enhancements that might be trivial to add to core, we have to weigh up just how widely those changes might be used. I think this is an ideal candidate for a plugin.

dmethvin, what do you think about the line refs provided by musicisair?

comment:8 Changed 9 years ago by snover

Resolution: patchwelcome
Status: newclosed

Nobody seems particularly interested in taking charge on this one so I am going to close it as patchwelcome.

As far as “we can’t animate both because Opera scrolls both”—if this was the case then there would be a lot of broken positional code in jQuery, since, as has been pointed out in this ticket, we only use one or the other, not both. (I’m not saying it’s not true that this is the case, just that if it is, exclusive checks are insufficient.)

Last edited 9 years ago by snover (previous) (diff)

comment:9 Changed 9 years ago by musicisair

More info (http://msdn.microsoft.com/en-us/library/ms534393(v=vs.85).aspx):

With Microsoft Internet Explorer 6 and later, when you use the !DOCTYPE declaration to specify standards-compliant mode, this attribute applies to the HTML element. When standards-compliant mode is not specified, as with earlier versions of Windows Internet Explorer, this attribute applies to the BODY element, not the HTML element.

comment:10 Changed 9 years ago by musicisair

Also (http://www.greywyvern.com/code/opera/bugs/htmlbodyOverflow):

From the CSS2 specifications, section 11.1.1: Visual effects, Overflow and clipping, Overflow: (emphasis added)

UAs must apply the 'overflow' property set on the root element to the viewport. When the root element is an HTML "HTML" element or an XHTML "html" element, and that element has an HTML "BODY" element or an XHTML "body" element as a child, user agents must instead apply the 'overflow' property from the first such child element to the viewport, if the value on the root element is 'visible'. The 'visible' value when used for the viewport must be interpreted as 'auto'. The element from which the value is propagated must have a used value for 'overflow' of 'visible'.

Opera [< 11] treats a root element overflow value of auto the same as visible and allows the value applied to the <body> element (hidden) to apply to the viewport [...]

Note: See TracTickets for help on using tickets.