Bug 52988 – REGRESSION (r75555): Safari RSS sidebar jiggles when scrolling

• Component: Layout and Rendering
• Status: RESOLVED FIXED

<rdar://problem/8905599>

* STEPS TO REPRODUCE
1. Navigate to <feed://images.apple.com/main/rss/hotnews/hotnews.rss>
2. Drag the vertical scroller down

* RESULTS
The sidebar jiggles vertically as the content scrolls.

Caused by r75555 <http://trac.webkit.org/changeset/75555>, which made scroll events asynchronous. WebKit now lets the sidebar draw after scrolling and before the scroll event handler gets a chance to reposition it.

See also bug 52790.

[Darin Adler]

Getting the scroll event before repainting happens allows this class of user interface implementation. I hope there’s a way we can restore that.

* NOTES
1. Safari RSS is also presenting the “doesn’t scroll far enough” symptoms from bug 52790.
2. The content workaround from bug 52790 is unlikely to help, since handling the scroll event synchronously is vital to avoiding jiggling.

[Mihai Parparita]

This is unfortunate.

Is using position: fixed an alternative here? It looks like the code (slipDiffScroll) tries to be clever if the sidebar is taller than the viewport area, so a scroll event handler will be necessary regardless in those cases (where the sidebar needs to scroll).

Darin: any ideas how to handle this use-case while still keeping the scroll event async?

[Darin Adler]

(In reply to comment #3)
> Darin: any ideas how to handle this use-case while still keeping the scroll event async?

I think that as long as the scroll event comes up before any repainting we’d be OK. It all depends what you mean by “async”. I believe the painting is also async, so the timing is what matters.

[Darin Adler]

(In reply to comment #3)
> Is using position: fixed an alternative here?

It’s not.

> It looks like the code (slipDiffScroll) tries to be clever if the sidebar is taller than the viewport area, so a scroll event handler will be necessary regardless in those cases (where the sidebar needs to scroll).

That’s right. That’s why position: fixed is not suitable.

[Darin Fisher (:fishd, Google)]

We should be able to fire the "scroll" event at next animation time, which happens just before painting.  (The "animation time" I'm referring to is the time when requestAnimationFrame callbacks run.)

[James Robinson]

(In reply to comment #5)
> (In reply to comment #3)
> > Is using position: fixed an alternative here?
> 
> It’s not.
> 
> > It looks like the code (slipDiffScroll) tries to be clever if the sidebar is taller than the viewport area, so a scroll event handler will be necessary regardless in those cases (where the sidebar needs to scroll).
> 
> That’s right. That’s why position: fixed is not suitable.

It looks like this layout is bimodal - either the sidebar takes its static position, or it acts as a position:fixed element.  The page would look a _lot_ better if the scroll listener was used to switch the element from being position:absolute to position:fixed when scrolling past the cutoff point, or when resizing the window.  That way there might be a hitch when switching between the two modes (but that looks jumpy anyway) but the scrolling will be synced up outside of that switch.

Longer-term this is a fairly common UI that CSS should be expanded to express declaratively.  It's just not going to work very well in script.

[James Robinson]

See http://code.google.com/p/chromium/issues/detail?id=2 for an example of this.  Sadly the javascript is compressed, but the left-hand bar with id "meta-float" switches between position:relative and position:fixed depending on the scroll offset.  It looks much better than the rss page.

[Darin Adler]

Sure, it would be neat to re-implement Safari RSS with a new technique. But note that this code has been working well since Safari 2. I don’t agree that it is “not going to work very well in script”. It has worked fine. Yes, it might be more efficient and more elegant to do it in some modern way.

Lets separate the “it can be done better” issue from the “we incompatibly changed the semantics of the scroll event” issue. If we can do something that keeps the existing Safari RSS working that would be extremely helpful.

[James Robinson]

(In reply to comment #9)
> Sure, it would be neat to re-implement Safari RSS with a new technique. But note that this code has been working well since Safari 2. I don’t agree that it is “not going to work very well in script”. It has worked fine. Yes, it might be more efficient and more elegant to do it in some modern way.
> 
> Lets separate the “it can be done better” issue from the “we incompatibly changed the semantics of the scroll event” issue. If we can do something that keeps the existing Safari RSS working that would be extremely helpful.

This may have worked in previous versions of Safari, but not in other browsers.

[Darin Adler]

(In reply to comment #10)
> This may have worked in previous versions of Safari, but not in other browsers.

Yes, I understand.

Since it’s been working in WebKit for 8 years there is a chance there is some other content that depends on it.

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> Since it’s been working in WebKit for 8 years there is a chance there is some other content that depends on it.

That's right, but this is part of the calculated risk of trying to fix the scroll event to be more compatible with other browsers (and the spec).  The hope is that the marketshare of other browsers means that we'll have some insulation here.  So far the only reports of regressions have been with WebKit-specific HTML/JS, which makes sense, and fortunately "we" have the option to change that HTML/JS.

[Dave Hyatt]

I think a superior implementation would both be asynchronous and ensure the event fires before a stale repaint.  Allowing stale repaints to happen is just poor behavior, and we shouldn't be using other browsers as an excuse for poor behavior.

Similarly, firing the event synchronously is also poor behavior.

I see no reason why we can't improve the implementation of asyc scrolling to keep these user interfaces working.

Keep in mind that some of the reason you don't see some of these nifty UI systems on the Web is that in other browsers have subpar (albeit async) onscroll implementations.

Let's fix this the right way and get the best of both worlds.

From an API standpint, the “asynchronous” nature of scroll events only really manifests itself in programmatic scrolling, i.e. in whether a the scroll event handler is entered before the script that modified the scroll position returned. r75555 decoupled the scroll event dispatch from the scrolling in order to satisfy this asynchronicity requirement, which introduced some issues. I think we should instead consider something else that will satisfy the requirement, which is to make “scrolling” itself decoupled from setting the scroll position by script. What I’m proposing is that setting scrollX, scrollY etc. (or calling scrollTo, scrollBy) will not scroll synchrnously (and thus not trigger a repaint), but just enqueue the scrolling for after return from script. Of course, the DOM will report the new scroll position when asked. It’s just committing to the ScrollView (or RenderLayer), and the subsequent scroll events, that would be deferred.

[Mihai Parparita]

(In reply to comment #8)
> See http://code.google.com/p/chromium/issues/detail?id=2 for an example of this.  Sadly the javascript is compressed, but the left-hand bar with id "meta-float" switches between position:relative and position:fixed depending on the scroll offset.  It looks much better than the rss page.

I looked into implementing this approach, but I ran into 53049 (on code.google.com the whole document scrolls, on Safari RSS there's only a div with overflow: overlay).

[Mihai Parparita]

(In reply to comment #14)
> I think we should instead consider something else that will satisfy the requirement, which is to make “scrolling” itself decoupled from setting the scroll position by script.

While this would solve this particular problem, it doesn't help in other aspects:
- We would still have to execute scripts before able to paint the results of scrolls, which means that we're more likely to have jerky scrolling.
- We would have divergent behavior from Firefox and IE (the difference in behavior would be visible to pages, WebKit would repaint later when setting scrollTop than when setting other properties).

Perhaps the best compromise for now is to have a mode switch, so that web content gets async events, but other WebKit clients get the legacy behavior (I'm not sure how easy it would be to get Safari RSS on the legacy codepath).

[Peter Kasting]

Comment 13 sounds right to me.

For comment 14, consider the case of smooth scrolling by enabling the code in ScrollAnimatorWin, which breaks scrolls into multiple pieces.  These each trigger a separate onscroll event.  We need to ensure that we don't get any scary re-entrancy if we run script synchronously in response to these.  I don't know if we do or not, but Hyatt's comment 13 makes me feel less nervous.

[Darin Fisher (:fishd, Google)]

(In reply to comment #13)
> I think a superior implementation would both be asynchronous and ensure the event fires before a stale repaint.

Yeah, this is essentially what I meant by comment #6.

However, James pointed out that if we do that then we are necessarily putting script execution on the critical path for scrolling.  This means that it limits our ability to optimize scrolling (e.g., imagine a background thread performing the scroll animation).  Do we really want to give up this kind of optimization?  Maybe so.

Maybe the better answer is to address this use case with CSS as James mentions in comment #7.  If we had that, then we could make the scroll event less useful in exchange for better opportunities to optimize scrolling.

(In reply to comment #11)
> (In reply to comment #10)
> > This may have worked in previous versions of Safari, but not in other browsers.
> 
> Yes, I understand.
> 
> Since it’s been working in WebKit for 8 years there is a chance there is some other content that depends on it.

Here is an example from a major website:

1) Navigate to <http://www.ibm.com/ibm100/us/en/?lnk=ibmhpls1/Corp/events/centennial_launch>
2) Click “A message from Sam Palmisano”
3) Scroll the page

Result: Jiggling

Can we revert r75555 until we come up with a solution that doesn’t break existing content?

[Mihai Parparita]

(In reply to comment #19)
> 1) Navigate to <http://www.ibm.com/ibm100/us/en/?lnk=ibmhpls1/Corp/events/centennial_launch>
> 2) Click “A message from Sam Palmisano”
> 3) Scroll the page
> 
> Result: Jiggling
> 
> Can we revert r75555 until we come up with a solution that doesn’t break existing content?

The page has jiggling in Firefox 3.6 and IE 8, so I think the authors just didn't think of this problem (and if they had, they would have used position: fixed).

[James Robinson]

(In reply to comment #19)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > This may have worked in previous versions of Safari, but not in other browsers.
> > 
> > Yes, I understand.
> > 
> > Since it’s been working in WebKit for 8 years there is a chance there is some other content that depends on it.
> 
> Here is an example from a major website:
> 
> 1) Navigate to <http://www.ibm.com/ibm100/us/en/?lnk=ibmhpls1/Corp/events/centennial_launch>
> 2) Click “A message from Sam Palmisano”
> 3) Scroll the page
> 
> Result: Jiggling
> 
> Can we revert r75555 until we come up with a solution that doesn’t break existing content?

I tried opening that in the latest Firefox build and it jiggles like crazy.  Seems like a site bug to me, not a WebKit one.

[Mihai Parparita]

As far as how to solve this regression with Safari RSS's sidebar, using requestAnimationFrame (http://webstuff.nfshost.com/anim-timing/Overview.html) may be an option. That way you can invoke the custom sidebar sizing/moving logic before painting. In your scroll event handler you'd just set a flag indicating that scrolling is happening, and start requesting callbacks before paintaing (this does mean that the first frame after scrolling starts will not have the new logic, but I doubt that's observable).

Something like:

var isScrolling = false;
var isScrollingTimeout = 0;

// scroller is the div with ID apple-rss-scroller, which currently has slipDiffScroll as its scroll event listener

scroller.onscroll = function() {
  if (!isScrolling) {
    isScrolling = true;
    window.requestAnimationFrame(slipDiffScroll, scroller);
  }
  
  if (isScrollingTimeout) {
    window.clearTimeout(isScrollingTimeout);
  }
  
  // Keep updating for some time after the scroll event was received, in the case of smooth scrolling and other scenarios where we don't fire a scroll event for every frame
  isScrollingTimeout = window.setTimeout(function() {
    isScrolling = false;
  }, 100);
};


function slipDiffScroll() {
  // Current implementation of slipDiffScroll
  
  if (isScrolling) {
    window.requestAnimationFrame(slipDiffScroll, scroller);
  }
}

[Andy Estes]

(In reply to comment #22)
> As far as how to solve this regression with Safari RSS's sidebar, using requestAnimationFrame (http://webstuff.nfshost.com/anim-timing/Overview.html) may be an option. 

Apple's WebKit port doesn't enable REQUEST_ANIMATION_FRAME, and I'm not sure this bug is sufficient justification for taking the risk of enabling it at this point.

The better approach is probably to add a quirk that dispatches ScrollEvent synchronously under certain circumstances.

[Andy Estes]

Created attachment 89544 [details]
Patch

[Maciej Stachowiak]

(In reply to comment #22)
> As far as how to solve this regression with Safari RSS's sidebar, using requestAnimationFrame (http://webstuff.nfshost.com/anim-timing/Overview.html) may be an option. That way you can invoke the custom sidebar sizing/moving logic before painting. 

So here's something I don't get. Why is running a requestAnimationFrame script callback before painting ok, but invoking the scroll event before painting would, per comment #18, be bad? If requestAnimationFrame is an acceptable API, then surely the right long-term fix is Hyatt's suggestion from comment #13.

[James Robinson]

requestAnimationFrame does not provide any assurance that the callback will always run before painting (either before scrolling or after)

[Maciej Stachowiak]

Comment on attachment 89544 [details]
Patch

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

> Source/WebCore/dom/EventQueue.cpp:43
> +    return applicationIsSafari() && (document->url().protocolIs("feed") || document->url().protocolIs("feeds"));

Maybe this would make more sense as a preference setting, then it could be customized per view. That would be a cleaner way to do the check, I think, and it would avoid the problem in applicationIsSafari cited below.

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:58
> +    static bool isSafari = mainBundleIsEqualTo("com.apple.Safari") || mainBundleIsEqualTo("com.apple.WebProcess");

This should have a FIXME, because long-term, WebProcess will not be used only by Safari. Or perhaps there is a better way to check specifically for Safari's WebProcess, in which case maybe it would be wise to future-proof this now.

[Maciej Stachowiak]

(In reply to comment #26)
> requestAnimationFrame does not provide any assurance that the callback will always run before painting (either before scrolling or after)

Then I guess using it wouldn't actually solve the problem.

[Andy Estes]

(In reply to comment #27)
> (From update of attachment 89544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89544&action=review
> 
> > Source/WebCore/dom/EventQueue.cpp:43
> > +    return applicationIsSafari() && (document->url().protocolIs("feed") || document->url().protocolIs("feeds"));
> 
> Maybe this would make more sense as a preference setting, then it could be customized per view. That would be a cleaner way to do the check, I think, and it would avoid the problem in applicationIsSafari cited below.

It looks like Safari RSS doesn't get its own view; the current view is navigated to the RSS page. This means that a Setting flag would have to be toggled on and off per navigation based on the URL scheme, which seems more difficult than the current approach.

> 
> > Source/WebCore/platform/RuntimeApplicationChecks.cpp:58
> > +    static bool isSafari = mainBundleIsEqualTo("com.apple.Safari") || mainBundleIsEqualTo("com.apple.WebProcess");
> 
> This should have a FIXME, because long-term, WebProcess will not be used only by Safari. Or perhaps there is a better way to check specifically for Safari's WebProcess, in which case maybe it would be wise to future-proof this now.

I'd be happy to do this, although I'm not sure I know precisely how to check if it's Safari's WebProcess, at least not via CF API.

[Maciej Stachowiak]

Comment on attachment 89544 [details]
Patch

Since a setting won't work, r=me, but please add the suggested FIXME and follow up on finding a proper way to identify Safari's WebProcess.

[Andy Estes]

Committed r83832

I keep stumbling upon webpages broken by r75555. Here’s another one: <http://www.howtobearetronaut.com/2011/03/stanley-kubricks-chicago-1949/> when you click on a photo and a “lightbox” opens up, it jiggles when you scroll the page behind it.

[James Robinson]

(In reply to comment #32)
> I keep stumbling upon webpages broken by r75555. Here’s another one: <http://www.howtobearetronaut.com/2011/03/stanley-kubricks-chicago-1949/> when you click on a photo and a “lightbox” opens up, it jiggles when you scroll the page behind it.

Are you also testing these pages in firefox and ie?

(In reply to comment #33)
> (In reply to comment #32)
> > I keep stumbling upon webpages broken by r75555. Here’s another one: <http://www.howtobearetronaut.com/2011/03/stanley-kubricks-chicago-1949/> when you click on a photo and a “lightbox” opens up, it jiggles when you scroll the page behind it.
> 
> Are you also testing these pages in firefox and ie?

No, I test them in Safari 5.0.5.