Last Comment Bug 833542 - scrollWidth, scrollHeight different when overflow is hidden versus visible
: scrollWidth, scrollHeight different when overflow is hidden versus visible
Status: RESOLVED FIXED
: relnote
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: 20 Branch
: All All
: -- normal (vote)
: mozilla21
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 1140412 841363
Blocks: 678678 861217
  Show dependency treegraph
 
Reported: 2013-01-22 13:02 PST by Daniel Trebbien
Modified: 2015-03-06 06:59 PST (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case (838 bytes, text/html)
2013-01-22 13:02 PST, Daniel Trebbien
no flags Details
fix (11.49 KB, patch)
2013-01-23 19:33 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Testcase #2 (2.43 KB, text/html)
2013-01-24 08:03 PST, Mats Palmgren (vacation)
no flags Details
Same as #2 but with overflow:visible (2.44 KB, text/html)
2013-01-24 08:19 PST, Mats Palmgren (vacation)
no flags Details
Quirks, body, small (880 bytes, text/html)
2013-01-24 09:16 PST, Mats Palmgren (vacation)
no flags Details
Quirks, body, large (873 bytes, text/html)
2013-01-24 09:16 PST, Mats Palmgren (vacation)
no flags Details
Standards, root, small (911 bytes, text/html)
2013-01-24 09:17 PST, Mats Palmgren (vacation)
no flags Details
Standards, root, large (904 bytes, text/html)
2013-01-24 09:19 PST, Mats Palmgren (vacation)
no flags Details
Testcase #7 (1.34 KB, text/html)
2013-01-25 06:38 PST, Mats Palmgren (vacation)
no flags Details

Description Daniel Trebbien 2013-01-22 13:02:21 PST
Created attachment 705081 [details]
Test case

As the summary says, the scrollWidth and scrollHeight values can change depending on the value of the overflow property.

Attached is a test case. In Firefox 18.0.1 and Firefox Aurora 20.0a2 (2013-01-22), the result is:

  For overflow:hidden
  theBox.scrollWidth = 81
  theBox.scrollHeight = 77

  For overflow:visible
  theBox.scrollWidth = 50
  theBox.scrollHeight = 50


In Safari 6.0.2 and WebKit r140445 built on 22 January 2013, the result is:

  For overflow:hidden
  theBox.scrollWidth = 77
  theBox.scrollHeight = 72

  For overflow:visible
  theBox.scrollWidth = 77
  theBox.scrollHeight = 72

In IE 10.0.9200.16466 the result is:

  For overflow:hidden
  theBox.scrollWidth = 82
  theBox.scrollHeight = 74

  For overflow:visible
  theBox.scrollWidth = 81
  theBox.scrollHeight = 74


The CSSOM View spec on scrollWidth and scrollHeight (http://www.w3.org/TR/cssom-view/#dom-element-scrollwidth and http://www.w3.org/TR/cssom-view/#dom-element-scrollheight ) seems to imply that it should not matter whether the overflow of `theBox' is hidden or visible.
Comment 1 Daniel Trebbien 2013-01-22 13:22:16 PST
Opera agrees with Firefox, but I still think that this is incorrect.  In Opera 12.12, the result is:

  For overflow:hidden
  theBox.scrollWidth = 77
  theBox.scrollHeight = 76

  For overflow:visible
  theBox.scrollWidth = 50
  theBox.scrollHeight = 50
Comment 2 Boris Zbarsky [:bz] 2013-01-23 09:31:12 PST
Note that the CSSOM spec for scroll* is known-buggy in various ways, so I wouldn't put too much stock in what it says... :(
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-01-23 18:50:17 PST
In this case, I think it's pretty clear we should make overflow:visible match hidden.
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-01-23 19:33:07 PST
Created attachment 705705 [details] [diff] [review]
fix

This changes behavior for a couple of tests in test_offsets.html --- tests where an overflow:visible element has overflowing children. I think that's OK. I effectively disable all the scrollWidth/scrollHeight tests where scrollWidth/scrollHeight is > clientWidth/clientHeight and the test was assuming scrollWidth==clientWidth/scrollHeight==clientHeight.
Comment 5 Mats Palmgren (vacation) 2013-01-24 08:03:13 PST
Created attachment 705862 [details]
Testcase #2
Comment 6 Mats Palmgren (vacation) 2013-01-24 08:19:50 PST
Created attachment 705868 [details]
Same as #2 but with overflow:visible
Comment 7 Mats Palmgren (vacation) 2013-01-24 09:16:09 PST
Created attachment 705906 [details]
Quirks, body, small
Comment 8 Mats Palmgren (vacation) 2013-01-24 09:16:52 PST
Created attachment 705908 [details]
Quirks, body, large
Comment 9 Mats Palmgren (vacation) 2013-01-24 09:17:44 PST
Created attachment 705909 [details]
Standards, root, small
Comment 10 Mats Palmgren (vacation) 2013-01-24 09:19:15 PST
Created attachment 705912 [details]
Standards, root, large
Comment 11 Mats Palmgren (vacation) 2013-01-24 09:35:53 PST
Comment on attachment 705705 [details] [diff] [review]
fix

You removed the last uses of Element::GetPaddingRectSize, remove it?
Comment 12 Mats Palmgren (vacation) 2013-01-24 09:49:19 PST
I think this is a scary change in terms of web compat.
Being more compatible with webkit and the spec is good, but I'm worried that
many sites and possibly libraries like jQuery etc have made workarounds for
these differences.  This change needs to be announced and documented widely.
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2013-01-24 10:18:57 PST
Comment on attachment 705705 [details] [diff] [review]
fix

Review of attachment 705705 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +1170,5 @@
> +    if (x1 < 0)
> +      x1 = 0;
> +  } else {
> +    if (x2 > aScrollPortSize.width)
> +      x2 = aScrollPortSize.width;

Please also add {}s for the three ifs that need them.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-01-24 21:08:26 PST
(In reply to Mats Palmgren [:mats] from comment #12)
> I think this is a scary change in terms of web compat.
> Being more compatible with webkit and the spec is good,

... And IE. Although IE and Webkit don't behave the same for overflowing content beyond the left/top of the element. We behave like IE in that case.

> but I'm worried that
> many sites and possibly libraries like jQuery etc have made workarounds for
> these differences.  This change needs to be announced and documented widely.

Hmm, I'm not sure how to announce this.

I suspect using scrollWidth/scrollHeight on overflow:visible element might be quite uncommon. I don't think I've ever thought about it before!

I grepped for scrollWidth/scrollHeight in jquery and couldn't find them at all.
Comment 15 Mats Palmgren (vacation) 2013-01-25 06:38:53 PST
Created attachment 706375 [details]
Testcase #7

> ... And IE.

Well, webkit and IE isn't compatible even for simple inline and block cases.

Results for Testcase #7:
    	scroll	client	offset
------------------------------
Firefox (with or without patch):
A	87,19	0,0	107,19
DIV	94,36	65,36	85,36

IE10:
A	107,37	0,0	107,18
DIV	94,37	65,37	85,37

Chrome (26.0.1386.0 dev):
A	0,0	0,0	107,19
DIV	105,36	65,36	85,36


Note that the scrollWidth spec says that we should add padding-right
to the overflow area of the children (which is what Chrome does).
This is clearly *wrong*, because it violates the basic CSS box model.
In the example, the image (lime) *overflows* the content width *over*
the padding-right area and beyond, and its clipped at the border-edge.
I've included a overflow:auto at the bottom to illustrate -- if you
scroll it right-most you'll see "padding" at the end in Chrome, it
shouldn't be there (IE10 and Firefox is correct).

Both IE10 and Chrome has the wrong result for A.scrollWidth above.
Firefox has correct scrollWidth values.
Comment 16 Mats Palmgren (vacation) 2013-01-25 06:42:22 PST
> I suspect using scrollWidth/scrollHeight on overflow:visible ...

Note that the change here also affects overflow:hidden -- see the TABLE cases
at the end in Testcase #2.

> I grepped for scrollWidth/scrollHeight in jquery and couldn't find them at all.

OK, maybe my worries are exaggerated.

I do think we should make a short release note though,
"element.scrollWidth/Height was changed to implement the CSSOM spec better"
or some such.
Comment 17 Daniel Trebbien 2013-01-25 07:44:35 PST
The only thing that I do know about jQuery's use of CSSOM View extensions is that jQuery.offset() depends on Element.getBoundingClientRect():  http://ejohn.org/blog/getboundingclientrect-is-awesome/

If there is a build of Firefox with Robert's patch applied, I can help test that the results of Element.getBoundingClientRect() do not change.  That's probably all that jQuery cares about, though.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-01-28 17:29:42 PST
(In reply to Daniel Trebbien from comment #17)
> If there is a build of Firefox with Robert's patch applied, I can help test
> that the results of Element.getBoundingClientRect() do not change.  That's
> probably all that jQuery cares about, though.

Thanks. I'm 100% certain this won't alter getBoundingClientRect.
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-01-29 01:18:33 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1f8a3c55ac
Comment 20 Ryan VanderMeulen [:RyanVM] 2013-01-29 06:48:23 PST
https://hg.mozilla.org/mozilla-central/rev/8e1f8a3c55ac
Comment 21 Daniel Trebbien 2013-02-14 15:23:40 PST
The patch for this bug might also have fixed Bug 678678.
Comment 22 Kohei Yoshino [:kohei] 2013-02-23 23:28:59 PST
I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21
Comment 23 Daniel Trebbien 2013-04-12 08:55:04 PDT
Seeing as the patch for this bug has likely fixed Bug 678678 and Bug 861217 as well, can Robert's patch be ported to the next Firefox 20.0.x release?
Comment 24 Boris Zbarsky [:bz] 2013-04-12 09:00:32 PDT
I don't think this change fits the criteria for such a backport (basically "major security problem, major stability regression, or major compatibility regression").

Note that this patch will ship in Firefox 21 no matter what, on May 14.

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