Bug 52790 – REGRESSION (r75555): Safari Reader does not scroll far enough in response to page down and other scrolling actions

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

<rdar://problem/8891256>

* STEPS TO REPRODUCE
1. Navigate to <http://www.nytimes.com/2011/01/20/us/20trials.html?_r=1&hp>
2. Activate Safari Reader
3. Press page down, end, space, page up, home

* RESULTS
Reader scrolls by just a few pixels in response to each of the above, instead of scrolling by a page or more.

Caused by r75555 <http://trac.webkit.org/changeset/75555>.

[Darin Fisher (:fishd, Google)]

Perhaps Safari Reader is buggy.  Has anyone looked to see how it is using scroll events?  It looks like Safari Reader is a closed source feature, and it doesn't seem to be the case that I can see what it is doing via the Web Inspector.

[Mihai Parparita]

This only happens when smooth scrolling is enabled. It looks like Reader tries to do its own smooth scrolling in that case. I've uploaded a copy of Reader's JavaScript (visible in the inspector when pausing JS execution) at http://persistent.info/webkit/test-cases/webkit-52790/reader.js (hope that's OK).

In the smoothScroll function, at line 437, there's a ReaderJSController.useSmoothScrolling() check (presumably ReaderJSController is an object exposed by Safari that queries the smooth scrolling system preference), and we abort if it's not enabled.

Later, at line 447 there's an animation handler that gets invoked periodically, to simulate the smooth scroll:
        scrollEventIsSmoothScroll = true;
        element.scrollTop = current;
        scrollEventIsSmoothScroll = false;

Meanwhile, the page has this scroll event handler:

function articleScrolled() {
    updateFindInPageMarkers();
    if (!scrollEventIsSmoothScroll && smoothScrollingAnimator) abortSmoothScroll();
    updateArticlePaddingAndShadows();
}

However, because the scroll event is now async, by the time articleScrolled runs, scrollEventIsSmoothScroll has been set back to false, so we abort the smooth scroll

For better or worse, this is as expected. The assumption was that since scroll is mostly async in other browsers (https://bugs.webkit.org/show_bug.cgi?id=45631#c8), this would not lead to any compatibility issues.

A workaround would be to wrap scrollEventIsSmoothScroll = false; in a setTimeout 0 call. Due to clamping of setTimeout values, it should always execute after the scroll handler.

[Darin Fisher (:fishd, Google)]

I think this is not a valid webkit bug.  Hopefully this'll be easy enough to patch in Safari Reader per Mihai's suggestion.

[Darin Adler]

Typically when a WebKit change breaks a shipping application we consider ways to keep that application working. I don’t think the fact that the application in question is not open source means this is not a bug worth fixing.

[Darin Adler]

Further, Safari Reader may not be the only application or website affected.

[Mihai Parparita]

In this particular case, since we were matching the behavior of other browsers, it seems like a low risk change, compatibility-wise. The change did just go out on the Chrome dev channel, so we'll be on the lookout for issues.

Additionally, since Reader is bundled in Safari, updating it to handle this change seems more feasible than other WebKit clients (I realize in the past we've added workarounds for Mail.app, AIM, and others). Also, the setTimeout workaround (see end of comment 2) is "backwards compatible" (in that it'll work whether scroll is sync or async), so it does not seem like a risky change.

[Darin Adler]

(In reply to comment #2)
> I've uploaded a copy of Reader's JavaScript (visible in the inspector when pausing JS execution) at http://persistent.info/webkit/test-cases/webkit-52790/reader.js (hope that's OK).

Please remove this. It’s not OK to post that code.

[Darin Adler]

(In reply to comment #6)
> Additionally, since Reader is bundled in Safari, updating it to handle this change seems more feasible than other WebKit clients (I realize in the past we've added workarounds for Mail.app, AIM, and others).

I understand. But this relies on some assumptions about Safari releases that may not be true in the future.

> Also, the setTimeout workaround (see end of comment 2) is "backwards compatible" (in that it'll work whether scroll is sync or async)

That’s good to know.

[Mihai Parparita]

(In reply to comment #7)
> Please remove this. It’s not OK to post that code.

Removed.

See also bug 52988.