Bug 91461 - Incorrect offset used for scrollWidth/Height calculation
Summary: Incorrect offset used for scrollWidth/Height calculation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-16 18:43 PDT by Emil A Eklund
Modified: 2012-07-18 17:27 PDT (History)
4 users (show)

See Also:


Attachments
Patch (14.96 KB, patch)
2012-07-16 18:52 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-07-16 18:43:25 PDT
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
Comment 1 Emil A Eklund 2012-07-16 18:52:47 PDT
Created attachment 152678 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-07-17 02:14:23 PDT
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?
Comment 3 Emil A Eklund 2012-07-17 09:01:10 PDT
(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.
Comment 4 Eric Seidel (no email) 2012-07-17 10:20:58 PDT
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.
Comment 5 Emil A Eklund 2012-07-17 10:22:02 PDT
(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.
Comment 6 WebKit Review Bot 2012-07-17 11:22:22 PDT
Comment on attachment 152678 [details]
Patch

Clearing flags on attachment: 152678

Committed r122861: <http://trac.webkit.org/changeset/122861>
Comment 7 WebKit Review Bot 2012-07-17 11:22:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Tony Chang 2012-07-17 13:25:25 PDT
Additional baselines for chromium mac (SL and Lion) landed in http://trac.webkit.org/changeset/122871.
Comment 9 Jer Noble 2012-07-18 17:06:36 PDT
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
Comment 10 Jer Noble 2012-07-18 17:23:39 PDT
Looks like fast/sub-pixel/block-with-margin-overflow.html will be broken on any platform which does not support SUBPIXEL_LAYOUT.
Comment 11 Jer Noble 2012-07-18 17:27:48 PDT
Fixed (for mac) in http://trac.webkit.org/changeset/123051.