Bug 145318 - [iOS] scrollIntoViewIfNeeded is not working with scroll-snap points
Summary: [iOS] scrollIntoViewIfNeeded is not working with scroll-snap points
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 172349
  Show dependency treegraph
 
Reported: 2015-05-22 14:00 PDT by Brent Fulgham
Modified: 2017-05-19 01:04 PDT (History)
12 users (show)

See Also:


Attachments
WIP Patch (51.71 KB, patch)
2015-06-18 18:25 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (52.21 KB, patch)
2015-06-18 22:09 PDT, Brent Fulgham
simon.fraser: 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-05-22 14:00:14 PDT
In Bug 145216, I corrected a problem where programmatic scrolls did not update the scroll snap state.

Unfortunately, I did not recognize that these changes were not sufficient to correct the problem in iOS, because many of the ScrollAnimator methods used on Mac are stubbed out on iOS.

This needs to be corrected so that the iOS ScrollAnimator knows about the current scroll snap offset index, so that its default '0' value doesn't keep getting retrieved by WebCore code and resetting the offset.

It may be that iOS should keep around a ScrollController object so that we can share the axis offset handling code between the two ports.
Comment 1 Brent Fulgham 2015-05-22 14:00:55 PDT
This bug manifests as a scroll area (with snap points), where calling 'scrollIntoViewIfNeeded" causes the right scroll snap region to move into view, only to flash back to the zeroth scroll element.
Comment 2 Radar WebKit Bug Importer 2015-05-22 14:01:34 PDT
<rdar://problem/21081501>
Comment 3 Brent Fulgham 2015-06-18 18:25:21 PDT
Created attachment 255161 [details]
WIP Patch
Comment 4 Brent Fulgham 2015-06-18 22:09:21 PDT
Created attachment 255172 [details]
Patch
Comment 5 Simon Fraser (smfr) 2015-06-19 10:48:38 PDT
Comment on attachment 255172 [details]
Patch

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

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:54
> +        HorizontalSnapOffsetIndex,
> +        VerticalSnapOffsetIndex,

Maybe these should have "Current" in the name.

> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:115
> +    unsigned m_currentHorizontalSnapPointIndex { 0 };
> +    unsigned m_currentVerticalSnapPointIndex { 0 };

Do we need to distinguish between "not set" and 0?

> Source/WebCore/platform/cocoa/ScrollController.mm:134
>      , m_lastMomentumScrollTimestamp(0)
> +#if ENABLE(RUBBER_BANDING)
>      , m_startTime(0)

Could use C++11 initialization to avoid these.
Comment 6 Brent Fulgham 2015-06-19 11:04:08 PDT
Comment on attachment 255172 [details]
Patch

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

>> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:54
>> +        VerticalSnapOffsetIndex,
> 
> Maybe these should have "Current" in the name.

I like that better. I'll change to that.

>> Source/WebCore/page/scrolling/ScrollingStateScrollingNode.h:115
>> +    unsigned m_currentVerticalSnapPointIndex { 0 };
> 
> Do we need to distinguish between "not set" and 0?

No. "non set" is indicated by empty snap offset vectors.

>> Source/WebCore/platform/cocoa/ScrollController.mm:134
>>      , m_startTime(0)
> 
> Could use C++11 initialization to avoid these.

Good idea.
Comment 7 Brent Fulgham 2015-06-19 12:25:39 PDT
Committed r185762: <http://trac.webkit.org/changeset/185762>