Skip to content

Commit

Permalink
fixes for Firefox (#223)
Browse files Browse the repository at this point in the history
- SVGs don't have height in Firefox, so we have to fallback to the
parentElement's height in order to calculate scale correctly. When the
`drag` handler was used on a `.handle` element that was an SVG, the
value of `scale` was always zero. Now it is the correct value (usually
1).
- Use the `transitionend` event to wait for events to finish. In chrome,
these animations were scheduled in such a way that the dom elements were
technically in the right order but visually inaccurate if we slowed the
transitions down. In Firefox, the smoke tests failed due to the elements
being out of order. In addition to using the `transitionend` event
(which is supported by all browsers we
support(https://developer.mozilla.org/en-US/docs/Web/Events/transitionend#Browser_compatibility),
when `DEBUG` mode is active (e.g. development or test builds, not
production builds`, we emit a custom event on the document and use a
custom waiter to prevent Ember from advancing the test suite too fast
before the transitions are done (the transforms need to finish so that
the elements are in the correct order in the dom).
- Add Firefox to our .travis.yml and testem.js files so that we test
against Firefox in CI.
  • Loading branch information
fivetanley authored and jgwhite committed Dec 19, 2018
1 parent 859947e commit 3d85276
Show file tree
Hide file tree
Showing 8 changed files with 72 additions and 19 deletions.
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ sudo: false
dist: trusty

addons:
firefox: latest
chrome: stable

cache:
Expand Down
13 changes: 10 additions & 3 deletions addon/helpers/drag.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,18 @@ export function drag(app, mode, itemSelector, offsetFn, callbacks = {}) {
let offset = offsetFn();
let itemElement = item.get(0);
let rect = itemElement.getBoundingClientRect();
let scale = itemElement.clientHeight / (rect.bottom - rect.top);
// firefox gives some elements, like <svg>, a clientHeight of 0.
// we can try to grab it off the parent instead to have a better
// guess at what the scale is.
// https://bugzilla.mozilla.org/show_bug.cgi?id=874811#c14
// https://stackoverflow.com/a/13647345
// https://stackoverflow.com/a/5042051
let clientHeight = (itemElement.clientHeight || item.offsetHeight) || itemElement.parentNode.offsetHeight;
let scale = clientHeight / (rect.bottom - rect.top);
let halfwayX = itemOffset.left + (offset.dx * scale) / 2;
let halfwayY = itemOffset.top + (offset.dy * scale) / 2;
let targetX = itemOffset.left + offset.dx * scale;
let targetY = itemOffset.top + offset.dy * scale;
let targetX = itemOffset.left + (offset.dx * scale);
let targetY = itemOffset.top + (offset.dy * scale);

andThen(() => {
triggerEvent(itemElement, start, {
Expand Down
18 changes: 18 additions & 0 deletions addon/helpers/waiters.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { registerWaiter } from '@ember/test';

let dropStarts = 0;
/**
* Watch for transitions to start and end before allowing ember to
* continue the test suite. Since we can't use transitionstart reliably in
* all browsers, but we can use transitionend, we emit our own custom
* event that is only used in tests.
*/
registerWaiter(() => {
return dropStarts === 0;
});
document.addEventListener('ember-sortable-drop-start', () => {
dropStarts++;
});
document.addEventListener('ember-sortable-drop-stop', () => {
dropStarts--;
});
40 changes: 28 additions & 12 deletions addon/mixins/sortable-item.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Promise } from 'rsvp';
import Mixin from '@ember/object/mixin';
import { DEBUG } from '@glimmer/env';
import { Promise, defer } from 'rsvp';
import { run } from '@ember/runloop';
import Ember from 'ember';
import { computed } from '@ember/object';
Expand Down Expand Up @@ -612,15 +613,15 @@ export default Mixin.create({
_drop() {
if (!this.element) { return; }

let transitionPromise = this._waitForTransition();

this._preventClick();

this.set('isDragging', false);
this.set('isDropping', true);

this._tellGroup('update');

this._waitForTransition()
.then(run.bind(this, '_complete'));
transitionPromise.then(() => this._complete())
},

/**
Expand Down Expand Up @@ -652,17 +653,32 @@ export default Mixin.create({
@return Promise
*/
_waitForTransition() {
return new Promise(resolve => {
run.next(() => {
let duration = 0;
if (DEBUG) {
// emit event for tests to start waiting for the transition to end
document.dispatchEvent(new Event('ember-sortable-drop-start'));
}

if (this.get('isAnimated')) {
duration = this.get('transitionDuration');
}
let transitionPromise;

if (this.get('isAnimated')) {
const deferred = defer();
this.element.addEventListener('transitionend', deferred.resolve);
transitionPromise = deferred.promise.finally(() => {
this.element.removeEventListener('transitionend', deferred.resolve);
});
} else {
const duration = this.get('isAnimated') ? this.get('transitionDuration') : 200;
transitionPromise = new Promise((resolve) => run.later(resolve, duration));
}

run.later(this, resolve, duration);
if (DEBUG) {
transitionPromise = transitionPromise.finally(() => {
// emit event for tests to stop waiting
document.dispatchEvent(new Event('ember-sortable-drop-stop'));
});
});
}

return transitionPromise;
},

/**
Expand Down
1 change: 1 addition & 0 deletions test-support/helpers/ember-sortable/test-helpers.js
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
import 'ember-sortable/helpers/drag';
import 'ember-sortable/helpers/reorder';
import 'ember-sortable/helpers/waiters';
10 changes: 9 additions & 1 deletion testem.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ module.exports = {
test_page: 'tests/index.html?hidepassed',
disable_watching: true,
launch_in_ci: [
'Chrome'
'Chrome',
'Firefox'
],
launch_in_dev: [
'Chrome'
Expand All @@ -17,6 +18,13 @@ module.exports = {
'--remote-debugging-port=0',
'--window-size=1440,900'
]
},
Firefox: {
mode: 'ci',
args: [
'--headless',
'--window-size=1440,900'
]
}
}
};
7 changes: 4 additions & 3 deletions tests/acceptance/smoke-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,9 @@ test('reordering with mouse events', function(assert) {
});

let itemHeight = () => {
let item = findWithAssert('.scrollable-demo .sortable-item');
return item.outerHeight() + parseInt(item.css('margin-top'));
let item = findWithAssert('.scrollable-demo .sortable-item')[0];
const itemStyle = item.currentStyle || window.getComputedStyle(item);
return item.offsetHeight + parseInt(itemStyle.marginTop);
};

let itemWidth = () => {
Expand All @@ -87,7 +88,7 @@ test('reordering with mouse events', function(assert) {
'mouse',
'.scrollable-demo .handle[data-item=Uno]',
() => {
return { dy: itemHeight() + 1, dx: itemWidth() + 1 };
return { dy: itemHeight() + 1, dx: itemWidth() };
},
{
dragend: function() {
Expand Down
1 change: 1 addition & 0 deletions tests/test-helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { setApplication } from '@ember/test-helpers';
import { start } from 'ember-qunit';
import 'ember-sortable/helpers/drag';
import 'ember-sortable/helpers/reorder';
import 'ember-sortable/helpers/waiters';

setApplication(Application.create(config.APP));

Expand Down

0 comments on commit 3d85276

Please sign in to comment.