Bug 145637 - REGRESSION (r181879): Scrolling order on pages with focused iframe is broken.
Summary: REGRESSION (r181879): Scrolling order on pages with focused iframe is broken.
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: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 145649
  Show dependency treegraph
 
Reported: 2015-06-03 23:36 PDT by Brent Fulgham
Modified: 2015-06-04 11:38 PDT (History)
8 users (show)

See Also:


Attachments
Patch (16.43 KB, patch)
2015-06-03 23:46 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (20.29 KB, patch)
2015-06-04 10:25 PDT, Brent Fulgham
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2015-06-03 23:36:52 PDT
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
Comment 1 Brent Fulgham 2015-06-03 23:37:17 PDT
<rdar://problem/20635581>
Comment 2 Brent Fulgham 2015-06-03 23:38:17 PDT
This bug seemed to be introduced by my recent refactoring in Bug 142789.
Comment 3 Brent Fulgham 2015-06-03 23:46:45 PDT
Created attachment 254254 [details]
Patch
Comment 4 Simon Fraser (smfr) 2015-06-04 08:51:38 PDT
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?
Comment 5 zalan 2015-06-04 09:04:48 PDT
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.
Comment 6 Simon Fraser (smfr) 2015-06-04 09:14:30 PDT
(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?
Comment 7 zalan 2015-06-04 09:19:16 PDT
(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.
Comment 8 Brent Fulgham 2015-06-04 09:48:11 PDT
(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).
Comment 9 zalan 2015-06-04 09:55:18 PDT
(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.
Comment 10 Brent Fulgham 2015-06-04 10:09:16 PDT
(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.
Comment 11 Brent Fulgham 2015-06-04 10:25:55 PDT
Created attachment 254269 [details]
Patch
Comment 12 Simon Fraser (smfr) 2015-06-04 10:33:02 PDT
Can we have a test that dumps clientWidth/clientHeight for various subpixel document sizes?
Comment 13 Brent Fulgham 2015-06-04 10:53:48 PDT
Committed r185201: <http://trac.webkit.org/changeset/185201>