Skip to content
This repository has been archived by the owner on Nov 11, 2017. It is now read-only.

Scrolling momentum + scroll-bar layering broken on WebKit + Solution #48

Closed
minusfive opened this issue May 26, 2013 · 23 comments
Closed
Assignees

Comments

@minusfive
Copy link

Problem

On WebKit, when the mouse remains over the rows' contents as it scrolls, momentum is broken. Also, the scroll-bar is being rendered behind the list's content. Here's a fiddle which illustrates this behaviour:

As you can see, scrolling momentum is broken since the rows' contents extend their full width (.row-wrapper); and since the rows extend the full width of the container, the scroll-bar is being rendered behind the content (I made them semi-transparent so you can see the scroll-bar behind them).

When the rows' contents don't extend the full width of the row, scrolling momentum remains normal as long as your mouse remains over an empty area of the row, as illustrated in this next fiddle (try scrolling on the gray area, then on the blue area, and compare momentum):

Solution

I've implemented a temporary solution on my app:

  • Applying z-index: 1; to .ember-list-view fixes the scroll-bar layering issue.
  • Momentarily applying position: relative; to .ember-list-scrolling-view while scrolling fixes the momentum problem, since it will be momentarily positioned on top of the rows, blocking mouse events on them.

Here are the same 2 fiddles, with the solutions applied (notice the scrollYChanged hook, and the _scrollingDidStart and _scrollingDidEnd methods under App.ListView, as well as the z-index: 1; on .ember-list-view in the CSS file:

Any suggestions on the most appropriate place to patch list-view.js directly to solve these problems? I'd be happy to patch and submit a pull request if you point me in the right direction.

@twokul
Copy link
Member

twokul commented May 26, 2013

@minusfive thanks for catching it! I wonder if we should just update list-view required css with:

.ember-list-view {
  overflow: auto;
  position: relative;
  z-index: 1;
}
.ember-list-item-view {
  position: absolute;
}
.ember-list-scrolling-view {
  position: relative;
}

instead of using scrolling events for that. /cc @stefanpenner

@minusfive
Copy link
Author

I tried this, and unfortunately won't work because it would cause .ember-list-scrolling-view to be constantly positioned on top of the .ember-list-item-views. It could work if we also added pointer-events: none; to .ember-list-scrolling-view, but Internet Explorer doesn't support that property (yet?). Perhaps with some Internet Explorer specific hack on top of it?

@minusfive
Copy link
Author

Heres one solution for IE http://www.vinylfox.com/forwarding-mouse-events-through-layers/

@minusfive
Copy link
Author

Also, my pleasure @twokul ;)

@minusfive
Copy link
Author

Sorry, so the pointer-events: none; trick won't work either, precisely because it simply transfers all mouse events transparently to the rows, which leaves us back where we started (just tested on this fiddle: http://jsfiddle.net/minusfive/Fs4fY/).

I forgot I had already tested this :P

@stefanpenner
Copy link
Member

something to keep in mind, is that testing this is quite annoying, what can we do to prevent future regressions. While keeping out time investment to a minimum.

@minusfive
Copy link
Author

Yeah, this one is tough to test. I'm spending some time this week familiarizing myself more with all the pieces, seeing whether I can come up with a good solution for this.

@minusfive
Copy link
Author

I think it may be related to this https://code.google.com/p/chromium/issues/detail?id=241476

@stefanpenner
Copy link
Member

@minusfive we might be forced to have a manual aspect to the testing process. We are trying really hard to have solid executable test coverage. But something like ^ is tricky.

mixonic added a commit to mixonic/list-view that referenced this issue Jun 4, 2013
@mixonic
Copy link
Sponsor Member

mixonic commented Jun 4, 2013

I've got a clean fix for this, I believe: mixonic@adfc03c

But I am having issues getting the tests updated for rc5. That patch requires rc5 for backburner.debounce. I'm also not explicitly removing the scrollYChanged listener, but I don't believe it is required.

/cc @kselden from our chat earlier.

@krisselden
Copy link
Contributor

fixed scrollbar layering with c55ae13

minusfive added a commit to minusfive/list-view that referenced this issue Jun 4, 2013
* 'webkit-scrolling' of git://github.com/mixonic/list-view:
  When scrolling add a class to the list-view. Fixes emberjs#48
  Update Ember and Handlebars
@minusfive
Copy link
Author

@mixonic thanks for looking into this!

I was going through your code and feel you're definitely going in the right direction—I feel that adding the pointer-events: none; to the list-view-items is definitely a more semantic solution than the one I came up with (position: relative; to the .ember-list-scrolling-view), since it's stating the effect we aim to accomplish, on the elements we want to affect. And we only need it to work on browsers which support this CSS property, so it's perfect.

However, I wonder if the mixin is the right place for it, considering it is also used by VirtualListView, and we don't really need it there. I have a feeling this would work better on the ListView itself. What do you think? I'm going to give it a try now, see if it works.

Also, and this may be a minor thing, but the .scrolling class could probably use better namespacing, perhaps something like .ember-list-view-is-scrolling would be better?

@minusfive
Copy link
Author

Also, perhaps instead of requiring another piece of external CSS, considering this (and other properties) are required anyway, we should probably put them inline, along with the others already in use. Thoughts?

@mixonic
Copy link
Sponsor Member

mixonic commented Jun 5, 2013

@minusfive I know Safari and Chrome exhibited the bad behavior. This code looks good in Safari, Chrome, FireFox on OSX and IE9 with click testing. @kselden's fix for the scrolling element is part of why my change is clean- I suspect mouse events on the scrolling element would have needed to be blocked with the old dom layout.

Lingering issues:

  • Tests. I'm kind of blocked on getting tests to run with rc5. Stefan offered to pair on it.
  • I believe Kris has an alternative strategy for stopping mousemove events. And legitimate concerns about this blocking click events and such while scrolling (I'm willing to take that compromise).
  • Virtual should be tested for the same bug. I don't expect to find it there, and if it is not there the fix should only be applied to list-view.

Personally I'm happy to keep this fix in CSS. Let's not go crazy adding a third thing (inlining more styles) to this ticket :-)

@minusfive
Copy link
Author

I just tested and can confirm both the bug and the solution also apply to VirtualListView... on desktop browsers. On touch devices it doesn't, since for inertial scrolling you must remove your finger from the screen, and there is no cursor in that case so there are no "mouseover" events to be triggered.

The question, then, is whether it is still necessary on Virtual, considering its main use case is touch devices.

On my apps I prefer to test for whether the client is a touch device or not, and use the right class in each case; but I can see why someone wouldn't do that, and would prefer to simply use Virtual all the time. I guess in those cases it's better to have the solution present in both implementations.

Not sure I agree with you on the external CSS vs. inline, though—which is more ironic than you think, considering I'm usually on the other side of this argument. I think that in cases in which the CSS controls some variable behaviour (like turning animations on/of, manipulating their timing/direction, x/y position, dimensions, etc.), external CSS is definitely the way to go. But in cases where the CSS is fixed and required, it should be part of the implementation so the possibility of user-error is reduced. Just my 2¢, of course.

But if everyone agrees on keeping the CSS external, what do you think about better naming of the class?

@minusfive
Copy link
Author

Thinking more about the external vs. inline CSS thing, perhaps your solution is better since it's only applying the class to one element, instead of adding the inline property to every item-view while scrolling, which [seems like it would be?] slower.

@minusfive
Copy link
Author

I split the z-index issue into its own: #54

minusfive referenced this issue Jun 5, 2013
delegate applyTransform from list item to parent
and always use top left in ListView to prevent
scroll bar from being occluded.
minusfive added a commit to minusfive/list-view that referenced this issue Jun 5, 2013
…orm3d positioning

- Moved applyTransform from ListView to ListViewMixin since VirtualListView
  also uses it.
- Brought transform3d based positioning back into applyTransform
- Added z-index:1 to ListView in order to fix scroll-bar layering on webkit
- Added position:absolute to ListItemViewMixin to remove CSS requirements
- Removed ListViewHelper (no longer needed)
- All tests seem to pass

Note: This does not fix emberjs#48 (inertial scrolling conflict with mouse events
      on webkit).
@ghost ghost assigned krisselden Sep 14, 2013
@kemenaran
Copy link

Is this issue really fixed? I ran into this problem today, using the latest ember-list-view version.

I eventually subclassed the ListView and disabled pointer-events during the scroll; is this expected?

@krisselden
Copy link
Contributor

@kemenaran the underlying cause of the initial report of this bug was fixed, but there maybe something you are doing inside of the list item view or css that causes the same symptom as this bug. Can you make a jsbin or fiddle demonstrating the problem with the latest version?

@kemenaran
Copy link

Here is a JS Bin demonstrating the issue in Webkit browsers (tested on Chrome 30 and Safari 7).

  1. Put the mouse cursor over an item of the list (see the hover effet)
  2. Scroll the list quickly and repeatedly using an input method that trigger inertial scrolling (like a trackpad)
  3. Half of the time, instead of decelerating slowly, the scrolling stops, and the whole page bounces instead

The issue doesn't appear if the cursor is over a section of the list without text (for instance the right side of the sample).

@stefanpenner
Copy link
Member

@kemenaran oh I believe the layering is fixed, but the issue you brought up I believe is not yet.

@kemenaran
Copy link

@stefanpenner ok, so I'm not just misusing the ListView :) Should this issue be re-opened, then ? Otherwise I can create a new one.

@stefanpenner
Copy link
Member

@kemenaran lets create a new one, as 1/2 of this issue has been resolved. Feel free to cross link

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants