Bug 145637 – REGRESSION (r181879): Scrolling order on pages with focused iframe is broken.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brent Fulgham]

Failure with certain types of nested iframe:

1. go to http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_iframe_scrolling
2. click into iframce
3. scroll down with gesture

[Brent Fulgham]

<rdar://problem/20635581>

[Brent Fulgham]

This bug seemed to be introduced by my recent refactoring in Bug 142789.

[Brent Fulgham]

Created attachment 254254 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 254254 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.h:481
> +    bool hasScrollableOverflowX() const { return scrollsOverflowX() && scrollWidth() != roundToInt(clientWidth()); }
> +    bool hasScrollableOverflowY() const { return scrollsOverflowY() && scrollHeight() != roundToInt(clientHeight()); }

Is rounding correct here? What does we do when the document has a sub pixel height?

[zalan]

Comment on attachment 254254 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        This page revealed a bug in our RenderBox code where the client height (and width),
> +        which are LayoutUnits, were being compared with scrollHeight (and scrollWidth), which
> +        are simple integer values.

The issue here is not the type difference (LayoutUnit vs integer) as LayoutUnit can also hold integrally snapped values, but the fact that scrolling is all integrally snapped while client height is not snapped at all. When comparing those values, they either need to be snapped the same way or not snapped at all.

> Source/WebCore/ChangeLog:20
> +        (WebCore::RenderBox::hasScrollableOverflowX): Need to properly round LayoutUnits to

Need to properly round LayoutUnits -> Need to properly round clientWidth/clientHeight to

>> Source/WebCore/rendering/RenderBox.h:481
>> +    bool hasScrollableOverflowY() const { return scrollsOverflowY() && scrollHeight() != roundToInt(clientHeight()); }
> 
> Is rounding correct here? What does we do when the document has a sub pixel height?

Since scrollWidth/Height and all other scrolling related arithmetics are still integral based we don't really have any other option. I guess we'd lose the subpixel part of the document through integer truncation.

[Simon Fraser (smfr)]

(In reply to comment #5)

> > Is rounding correct here? What does we do when the document has a sub pixel height?
> 
> Since scrollWidth/Height and all other scrolling related arithmetics are
> still integral based we don't really have any other option. I guess we'd
> lose the subpixel part of the document through integer truncation.

Right, but does this rounding match rounding we do in adjustViewSize() or whatever?

[zalan]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > > Is rounding correct here? What does we do when the document has a sub pixel height?
> > 
> > Since scrollWidth/Height and all other scrolling related arithmetics are
> > still integral based we don't really have any other option. I guess we'd
> > lose the subpixel part of the document through integer truncation.
> 
> Right, but does this rounding match rounding we do in adjustViewSize() or
> whatever?

This rounding matches RenderBox::scrollWidth()/scrollHeight(). Whether that matches adjustViewSize() etc I guess that's something Brent needs to figure out. -I hope it does otherwise we need to adjust all integral roundings for scrolling accordingly.

[Brent Fulgham]

(In reply to comment #7)
> > Right, but does this rounding match rounding we do in adjustViewSize() or
> > whatever?
> 
> This rounding matches RenderBox::scrollWidth()/scrollHeight(). Whether that
> matches adjustViewSize() etc I guess that's something Brent needs to figure
> out. -I hope it does otherwise we need to adjust all integral roundings for
> scrolling accordingly.

It does look like RenderBox::canBeScrolledAndHasScrollableArea() has the same comparison issue (scrollHeight versus clientHeight), and should probably get the same rounding correction.

Now that I know what I'm looking for, I see that other code that deals with scrollHeight/scrollWidth seem to use "pixelSnappedClientWidth" and "pixelSnappedClientHeight", so I'll switch to that instead (they are 100% identical, but probably gives clearer meaning).

[zalan]

(In reply to comment #8)
> (In reply to comment #7)
> > > Right, but does this rounding match rounding we do in adjustViewSize() or
> > > whatever?
> > 
> > This rounding matches RenderBox::scrollWidth()/scrollHeight(). Whether that
> > matches adjustViewSize() etc I guess that's something Brent needs to figure
> > out. -I hope it does otherwise we need to adjust all integral roundings for
> > scrolling accordingly.
> 
> It does look like RenderBox::canBeScrolledAndHasScrollableArea() has the
> same comparison issue (scrollHeight versus clientHeight), and should
> probably get the same rounding correction.
> 
> Now that I know what I'm looking for, I see that other code that deals with
> scrollHeight/scrollWidth seem to use "pixelSnappedClientWidth" and
> "pixelSnappedClientHeight", so I'll switch to that instead (they are 100%
> identical, but probably gives clearer meaning).
Please don't switch to them. Their names are highly misleading (pixelsnapping is so overloaded at this point) and I am going to remove them at some point.
Using roundToInt is more explicit and it clearly matches RenderBox::scrollWidth()/scrollHeight snapping behavior.

[Brent Fulgham]

(In reply to comment #6)
> (In reply to comment #5)
> 
> > > Is rounding correct here? What does we do when the document has a sub pixel height?
> > 
> > Since scrollWidth/Height and all other scrolling related arithmetics are
> > still integral based we don't really have any other option. I guess we'd
> > lose the subpixel part of the document through integer truncation.
> 
> Right, but does this rounding match rounding we do in adjustViewSize() or
> whatever?

Yes, I think we are now matching what other code was already doing.

For example, hasHorizontalOverflow/hasVerticalOverflow was using pixel-snapped units while these other tests were not.

[Brent Fulgham]

Created attachment 254269 [details]
Patch

[Simon Fraser (smfr)]

Can we have a test that dumps clientWidth/clientHeight for various subpixel document sizes?

[Brent Fulgham]

Committed r185201: <http://trac.webkit.org/changeset/185201>