Bug 91461 – Incorrect offset used for scrollWidth/Height calculation

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Emil A Eklund]

Due to a different offset being used to calculate the scrollWidth/Height and pixelSnappedClientWidth/Height the scroll value can be off by one in same cases. This can causes scrollbars to appear even when there is no overflow.

Downstream chromium bug: http://code.google.com/p/chromium/issues/detail?id=137068

[Emil A Eklund]

Created attachment 152678 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 152678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152678&action=review

I'm confused by this.  This is more than "off by one".  It seems you're adjusting these values to include the accumulated paretn offset?

> Source/WebCore/rendering/RenderBox.cpp:388
> +        return snapSizeToPixel(max(clientWidth(), layoutOverflowRect().maxX() - borderLeft()), x() + clientLeft());

I'm surprised there isnt' an accessor to get x + clientLeft?

[Emil A Eklund]

(In reply to comment #2)
> (From update of attachment 152678 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152678&action=review
> 
> I'm confused by this.  This is more than "off by one".  It seems you're adjusting these values to include the accumulated paretn offset?

Well, almost. I'm not adjusting the value to include the accumulated offset, it is only used to determine how the width/height values should be rounded. Thus at most it was off by one before.

> 
> > Source/WebCore/rendering/RenderBox.cpp:388
> > +        return snapSizeToPixel(max(clientWidth(), layoutOverflowRect().maxX() - borderLeft()), x() + clientLeft());
> 
> I'm surprised there isnt' an accessor to get x + clientLeft?

Yeah, as was I. I considered adding one but couldn't come up with a good name. As it is only used in a handful of places it might not be worth the cost of adding one.

[Eric Seidel (no email)]

Comment on attachment 152678 [details]
Patch

I see.  That's tricky.  Ideally we would get these as part of larger structs, instead of directly in these accessors.  Like scrollRect(), etc.  But that's another change for another time.

[Emil A Eklund]

(In reply to comment #4)
> (From update of attachment 152678 [details])
> I see.  That's tricky.  Ideally we would get these as part of larger structs, instead of directly in these accessors.  Like scrollRect(), etc.  But that's another change for another time.

Thanks. I'll see about making that change down the road.

[WebKit Review Bot]

Comment on attachment 152678 [details]
Patch

Clearing flags on attachment: 152678

Committed r122861: <http://trac.webkit.org/changeset/122861>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Additional baselines for chromium mac (SL and Lion) landed in http://trac.webkit.org/changeset/122871.

[Jer Noble]

This changeset included a new test fast/sub-pixel/block-with-margin-overflow.html which has been broken on the Apple Lion bots since its addition, as well as new expected results for platform/mac/fast/multicol/shrink-to-column-height-for-pagination-expected.txt which have also been broken.

See:
http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r122861%20(1220)/results.html
http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r122861%20(1220)/fast/multicol/shrink-to-column-height-for-pagination-pretty-diff.html
http://build.webkit.org/results/Apple%20Lion%20Release%20WK1%20(Tests)/r122861%20(1220)/fast/sub-pixel/block-with-margin-overflow-pretty-diff.html

[Jer Noble]

Looks like fast/sub-pixel/block-with-margin-overflow.html will be broken on any platform which does not support SUBPIXEL_LAYOUT.

[Jer Noble]

Fixed (for mac) in http://trac.webkit.org/changeset/123051.