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

Commit

Permalink
fix z order issue with scrollbar
Browse files Browse the repository at this point in the history 
delegate applyTransform from list item to parent
and always use top left in ListView to prevent
scroll bar from being occluded.
  • Loading branch information
@krisselden
krisselden committed Jun 4, 2013
1 parent 2dd59fb commit c55ae13
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 5 deletions.
7 changes: 2 additions & 5 deletions packages/list-view/lib/list_item_view_mixin.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
require('list-view/list_view_helper');

var get = Ember.get, set = Ember.set;

function samePosition(a, b) {
Expand All @@ -19,7 +17,7 @@ function positionElement() {
// TODO: avoid needing this by avoiding unnecessary
// calls to this method in the first place
if (samePosition(position, _position)) { return; }
this.applyTransform(element, position);
this._parentView.applyTransform(element, position);

this._position = position;
}, this);
Expand All @@ -33,6 +31,5 @@ Ember.ListItemViewMixin = Ember.Mixin.create({
classNames: ['ember-list-item-view'],
_position: null,
_positionDidChange: Ember.observer(positionElement, 'position'),
_positionElement: positionElement,
applyTransform: Ember.ListViewHelper.applyTransform
_positionElement: positionElement
});
8 changes: 8 additions & 0 deletions packages/list-view/lib/list_view.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,14 @@ Ember.ListView = Ember.ContainerView.extend(Ember.ListViewMixin, {
'overflow-scrolling': 'touch'
},

applyTransform: function(element, position){
var x = position.x,
y = position.y;

element.style.top = y + 'px';
element.style.left = x + 'px';
},

_scrollTo: function(scrollTop) {
var element = get(this, 'element');

Expand Down

5 comments on commit c55ae13

@minusfive
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After testing I realized that we lost transform3d-based positioning with this commit, which affects performance. This does make it look like the scroll-bar z-index issue was fixed but in reality it isn't. I'm working on a new commit which should bring back 3d transforms and fix the z-index issue. I've also opened an issue specifically for this problem as issue #54 (separating it from the inertial scroll problem of issue #48).

@minusfive
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See PR #55

@krisselden
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"This does make it look like the scroll-bar z-index issue was fixed but in reality it isn't." What was broken after this commit? Do you have a demo of this problem? I have many doubts about this statement.

@minusfive
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the original issue was that when using translate3d-based positioning the z-index would get screwed. Regular positioning (absolute top/left) always gave the right z-index (or I should say "never had that problem").

When you moved the applyTransform from ListViewHelper to ListView you only moved the code that handled regular positioning. Since translate3d-based positioning was no longer in play, the scroll-bar z-index looked like it had been fixed, but in reality what you removed was the thing that created the problem in the first place.

Normally this would've been great. Unfortunately, in this case the translate3d positioning was there for a reason: performance (as you can see here http://jsperf.com/translate3d-vs-positioning). By removing it you accidentally introduced a regression.

When I added the translate3ds back to the applyTransform, the z-index problem came back. The only valid solution I've found is to apply z-index: 1; to the listView element itself, as stated on the original issue.

@krisselden
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the jsperf link, one thing I noticed is the 2d css transforms perform just as well, and they also don't mess up the scroll bar z order.

Please sign in to comment.