833542 - scrollWidth, scrollHeight different when overflow is hidden versus visible

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Daniel Trebbien]

Attachment: 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]

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 [:bzbarsky]]

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) (email my personal email if necessary)]

In this case, I think it's pretty clear we should make overflow:visible match hidden.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 (inactive)]

Attachment: Testcase #2

Comment 6

[Mats Palmgren (inactive)]

Attachment: Same as #2 but with overflow:visible

Comment 7

[Mats Palmgren (inactive)]

Attachment: Quirks, body, small

Comment 8

[Mats Palmgren (inactive)]

Attachment: Quirks, body, large

Comment 9

[Mats Palmgren (inactive)]

Attachment: Standards, root, small

Comment 10

[Mats Palmgren (inactive)]

Attachment: Standards, root, large

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 705705 [details] [diff] [review]
fix

You removed the last uses of Element::GetPaddingRectSize, remove it?

Comment 12

[Mats Palmgren (inactive)]

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 (he/him; ⌚ UTC+1/+2)]

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) (email my personal email if necessary)]

(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 (inactive)]

Attachment: 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 (inactive)]

> 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]

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) (email my personal email if necessary)]

(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) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1f8a3c55ac

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e1f8a3c55ac

Comment 21

[Daniel Trebbien]

The patch for this bug might also have fixed Bug 678678.

Comment 22

[Kohei Yoshino]

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]

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 [:bzbarsky]]

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.