scrollWidth, scrollHeight different when overflow is hidden versus visible

RESOLVED FIXED in mozilla21

Status

()

Core
DOM: CSS Object Model
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Daniel Trebbien, Assigned: roc)

Tracking

(Depends on: 1 bug, {relnote})

20 Branch
mozilla21
relnote
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
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

Updated

4 years ago
Attachment #705081 - Attachment mime type: text/plain → text/html
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... :(
In this case, I think it's pretty clear we should make overflow:visible match hidden.
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.
Assignee: nobody → roc
Attachment #705705 - Flags: review?(matspal)
Created attachment 705862 [details]
Testcase #2
Created attachment 705868 [details]
Same as #2 but with overflow:visible
Created attachment 705906 [details]
Quirks, body, small
Created attachment 705908 [details]
Quirks, body, large
Created attachment 705909 [details]
Standards, root, small
Created attachment 705912 [details]
Standards, root, large
Comment on attachment 705705 [details] [diff] [review]
fix

You removed the last uses of Element::GetPaddingRectSize, remove it?
Attachment #705705 - Flags: review?(matspal) → review+
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.
Keywords: dev-doc-needed, relnote
OS: Mac OS X → All
Hardware: x86_64 → All
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.
(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.
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.
> 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.
Keywords: dev-doc-needed
(Reporter)

Comment 17

4 years ago
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.
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1f8a3c55ac
https://hg.mozilla.org/mozilla-central/rev/8e1f8a3c55ac
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

4 years ago
Depends on: 841363
(Reporter)

Comment 21

4 years ago
The patch for this bug might also have fixed Bug 678678.
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
(Reporter)

Comment 23

4 years ago
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?
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.
Blocks: 678678
Blocks: 861217
Depends on: 1140412
You need to log in before you can comment on or make changes to this bug.