revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-28 00:10:57 +01:00
Code Issues Releases Wiki Activity

Don't mutate DOM on touch-originated cursor events in Differential

Browse source 
Summary:
Fixes T10229. Broadly:

  - When the user hovers over a line number or inline comment, we update the yellow reticle to highlight the relevant lines. Specifically, this is in response to a `mouseover` event.
  - On touch devices, touches fire `mouseover` and if you mutate the DOM inside the event, the device aborts the touch.

To remedy this:

  - Distingiush between mouse-originated and touch-originated cursor events.
    - We do this, roughly, by setting a flag when we see "touchstart", and clearing it when we see the second copy of any unique cursor event.
    - This method is complex, but should be robust to any implementation differences between devices (for example, it will work no matter which order the events are fired in).
    - This method should also produce the correct results on weird devices that have both mouse-devices and touch-devices available for cursor input.
  - When we see a touch-originated `mouseover` or `mouseout`, don't mutate the DOM.
  - Put an extra DOM mutation into the `click` event to improve highlighting behavior on touch devices.

Test Plan:
  - In iOS Simulator (4s, iOS 9.2), clicked various inline actions ("Reply", "Hide", "Done", "Cancel", line numbers, etc). Got responses after a single touch.
  - Verified hover + click behavior on a desktop.
  - Logged and examined a bunch of events as a general sanity check.

Reviewers: chad

Reviewed By: chad

Subscribers: aljungberg

Maniphest Tasks: T10229

Differential Revision: https://secure.phabricator.com/D15136
This commit is contained in:
epriestley 2016-01-29 05:21:41 -08:00
parent dbf1d0d721
commit bf43d4cf2a
5 changed files with 134 additions and 52 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

68
resources/celerity/map.php
View file

@ -8,10 +8,10 @@
return array(
'names' => array(
'core.pkg.css' => '8abb1666',
'core.pkg.js' => '573e6664',
'core.pkg.js' => 'a79eed25',
'darkconsole.pkg.js' => 'e7393ebb',
'differential.pkg.css' => '2de124c9',
'differential.pkg.js' => 'f83532f8',
'differential.pkg.js' => '5c2ba922',
'diffusion.pkg.css' => 'f45955ed',
'diffusion.pkg.js' => '3a9a8bfa',
'maniphest.pkg.css' => '4845691a',
@ -192,8 +192,8 @@ return array(
'rsrc/externals/font/lato/lato-regular.ttf' => 'e270165b',
'rsrc/externals/font/lato/lato-regular.woff' => '13d39fe2',
'rsrc/externals/font/lato/lato-regular.woff2' => '57a9f742',
'rsrc/externals/javelin/core/Event.js' => '85ea0626',
'rsrc/externals/javelin/core/Stratcom.js' => '6c53634d',
'rsrc/externals/javelin/core/Event.js' => '2ee659ce',
'rsrc/externals/javelin/core/Stratcom.js' => '6ad39b6f',
'rsrc/externals/javelin/core/__tests__/event-stop-and-kill.js' => '717554e4',
'rsrc/externals/javelin/core/__tests__/install.js' => 'c432ee85',
'rsrc/externals/javelin/core/__tests__/stratcom.js' => '88bf7313',
@ -379,7 +379,7 @@ return array(
'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76',
'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
'rsrc/js/application/differential/behavior-dropdown-menus.js' => '9a6b9324',
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '65ef6074',
'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '4fbbc3e9',
'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492',
'rsrc/js/application/differential/behavior-populate.js' => '8694b1df',
'rsrc/js/application/differential/behavior-toggle-files.js' => 'ca3f91eb',
@ -465,7 +465,7 @@ return array(
'rsrc/js/core/behavior-choose-control.js' => '327a00d1',
'rsrc/js/core/behavior-crop.js' => 'fa0f4fc2',
'rsrc/js/core/behavior-dark-console.js' => 'f411b6ae',
'rsrc/js/core/behavior-device.js' => 'a205cf28',
'rsrc/js/core/behavior-device.js' => 'b5b36110',
'rsrc/js/core/behavior-drag-and-drop-textarea.js' => '4f6a4b4e',
'rsrc/js/core/behavior-error-log.js' => '6882e80a',
'rsrc/js/core/behavior-fancy-datepicker.js' => '8ae55229',
@ -584,12 +584,12 @@ return array(
'javelin-behavior-dashboard-tab-panel' => 'd4eecc63',
'javelin-behavior-day-view' => '5c46cff2',
'javelin-behavior-desktop-notifications-control' => 'edd1ba66',
'javelin-behavior-device' => 'a205cf28',
'javelin-behavior-device' => 'b5b36110',
'javelin-behavior-differential-add-reviewers-and-ccs' => 'e10f8e18',
'javelin-behavior-differential-comment-jump' => '4fdb476d',
'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
'javelin-behavior-differential-dropdown-menus' => '9a6b9324',
'javelin-behavior-differential-edit-inline-comments' => '65ef6074',
'javelin-behavior-differential-edit-inline-comments' => '4fbbc3e9',
'javelin-behavior-differential-feedback-preview' => 'b064af76',
'javelin-behavior-differential-keyboard-navigation' => '2c426492',
'javelin-behavior-differential-populate' => '8694b1df',
@ -683,7 +683,7 @@ return array(
'javelin-diffusion-locate-file-source' => 'b42eddc7',
'javelin-dom' => '805b806a',
'javelin-dynval' => 'f6555212',
'javelin-event' => '85ea0626',
'javelin-event' => '2ee659ce',
'javelin-fx' => '54b612ba',
'javelin-history' => 'd4505101',
'javelin-install' => '05270951',
@ -702,7 +702,7 @@ return array(
'javelin-router' => '29274e2b',
'javelin-scrollbar' => '087e919c',
'javelin-sound' => '949c0fe5',
'javelin-stratcom' => '6c53634d',
'javelin-stratcom' => '6ad39b6f',
'javelin-tokenizer' => '8d3bc1b2',
'javelin-typeahead' => '70baed2f',
'javelin-typeahead-composite-source' => '503e17fd',
@ -1050,6 +1050,9 @@ return array(
'javelin-install',
'javelin-event',
),
'2ee659ce' => array(
'javelin-install',
),
'327a00d1' => array(
'javelin-behavior',
'javelin-stratcom',
@ -1164,6 +1167,14 @@ return array(
'phabricator-drag-and-drop-file-upload',
'phabricator-textareautils',
),
'4fbbc3e9' => array(
'javelin-behavior',
'javelin-stratcom',
'javelin-dom',
'javelin-util',
'javelin-vector',
'differential-inline-comment-editor',
),
'4fdb476d' => array(
'javelin-behavior',
'javelin-stratcom',
@ -1295,14 +1306,6 @@ return array(
'javelin-request',
'javelin-workflow',
),
'65ef6074' => array(
'javelin-behavior',
'javelin-stratcom',
'javelin-dom',
'javelin-util',
'javelin-vector',
'differential-inline-comment-editor',
),
'66dd6e9e' => array(
'javelin-behavior',
'javelin-behavior-device',
@ -1316,6 +1319,12 @@ return array(
'69adf288' => array(
'javelin-install',
),
'6ad39b6f' => array(
'javelin-install',
'javelin-event',
'javelin-util',
'javelin-magical-init',
),
'6b8ef10b' => array(
'javelin-install',
),
@ -1327,12 +1336,6 @@ return array(
'javelin-install',
'javelin-util',
),
'6c53634d' => array(
'javelin-install',
'javelin-event',
'javelin-util',
'javelin-magical-init',
),
'6d3e1947' => array(
'javelin-behavior',
'javelin-diffusion-locate-file-source',
@ -1446,9 +1449,6 @@ return array(
'javelin-dom',
'javelin-stratcom',
),
'85ea0626' => array(
'javelin-install',
),
'85ee8ce6' => array(
'aphront-dialog-view-css',
),
@ -1602,13 +1602,6 @@ return array(
'javelin-vector',
'javelin-magical-init',
),
'a205cf28' => array(
'javelin-behavior',
'javelin-stratcom',
'javelin-dom',
'javelin-vector',
'javelin-install',
),
'a2828756' => array(
'javelin-dom',
'javelin-util',
@ -1739,6 +1732,13 @@ return array(
'javelin-dom',
'phabricator-draggable-list',
),
'b5b36110' => array(
'javelin-behavior',
'javelin-stratcom',
'javelin-dom',
'javelin-vector',
'javelin-install',
),
'b5c256b8' => array(
'javelin-install',
'javelin-dom',

19
webroot/rsrc/externals/javelin/core/Event.js vendored
View file

@ -326,7 +326,16 @@ JX.install('Event', {
/**
* @task info
*/
nodeDistances : {}
nodeDistances : {},
/**
* True if this is a cursor event that was caused by a touch interaction
* rather than a mouse device interaction.
*
* @type bool
* @taks info
*/
isTouchEvent: false
},
/**
@ -339,7 +348,13 @@ JX.install('Event', {
if (__DEV__) {
JX.Event.prototype.toString = function() {
var path = '['+this.getPath().join(', ')+']';
return 'Event<'+this.getType()+', '+path+', '+this.getTarget()+'>';
var type = this.getType();
if (this.getIsTouchEvent()) {
type = type + '/touch';
}
return 'Event<'+type+', '+path+', '+this.getTarget()+'>';
};
}
}

44
webroot/rsrc/externals/javelin/core/Stratcom.js vendored
View file

@ -41,6 +41,7 @@ JX.install('Stratcom', {
_auto : '*',
_data : {},
_execContext : [],
_touchState: null,
/**
* Node metadata is stored in a series of blocks to prevent collisions
@ -330,6 +331,45 @@ JX.install('Stratcom', {
etype = 'blur';
}
// Map of touch events and whether they are unique per touch.
var touch_map = {
touchstart: true,
touchend: true,
mousedown: true,
mouseup: true,
click: true,
// These can conceivably fire multiple times per touch, so if we see
// them more than once that doesn't tell us that we're handling a new
// event.
mouseover: false,
mouseout: false,
mousemove: false,
touchmove: false
};
// If this is a 'touchstart', we're handling touch events.
if (etype == 'touchstart') {
this._touchState = {};
}
// If this is a unique touch event that we haven't seen before, remember
// it as part of the current touch state. On the other hand, if we've
// already seen it, we can deduce that we must be handling a new cursor
// event that is unrelated to the last touch we saw, and conclude that
// we are no longer processing a touch event.
if (touch_map[etype] && this._touchState) {
if (!this._touchState[etype]) {
this._touchState[etype] = true;
} else {
this._touchState = null;
}
}
// This event is a touch event if we're carrying some touch state.
var is_touch = (etype in touch_map) &&
(this._touchState !== null);
var proxy = new JX.Event()
.setRawEvent(event)
.setData(event.customData)
@ -337,9 +377,11 @@ JX.install('Stratcom', {
.setTarget(target)
.setNodes(nodes)
.setNodeDistances(distances)
.setIsTouchEvent(is_touch)
.setPath(path.reverse());
// Don't touch this for debugging purposes
// You can uncomment this to print out all events flowing through
// Stratcom, which tends to make debugging easier.
//JX.log('~> '+proxy.toString());
return this._dispatchProxy(proxy);

50
webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js
View file

@ -22,6 +22,26 @@ JX.behavior('differential-edit-inline-comments', function(config) {
var editor = null;
function updateReticleForComment(e) {
root = e.getNode('differential-changeset');
if (!root) {
return;
}
var data = e.getNodeData('differential-inline-comment');
var change = e.getNodeData('differential-changeset');
var id_part = data.on_right ? change.right : change.left;
var new_part = data.isNewFile ? 'N' : 'O';
var prefix = 'C' + id_part + new_part + 'L';
origin = JX.$(prefix + data.number);
target = JX.$(prefix + (parseInt(data.number, 10) +
parseInt(data.length, 10)));
updateReticle();
}
function updateReticle() {
JX.DOM.getContentFrame().appendChild(reticle);
@ -176,6 +196,10 @@ JX.behavior('differential-edit-inline-comments', function(config) {
['mouseover', 'mouseout'],
['differential-changeset', 'tag:th'],
function(e) {
if (e.getIsTouchEvent()) {
return;
}
if (editor) {
// Don't update the reticle if we're editing a comment, since this
// would be distracting and we want to keep the lines corresponding
@ -274,24 +298,14 @@ JX.behavior('differential-edit-inline-comments', function(config) {
['mouseover', 'mouseout'],
'differential-inline-comment',
function(e) {
if (e.getIsTouchEvent()) {
return;
}
if (e.getType() == 'mouseout') {
hideReticle();
} else {
root = e.getNode('differential-changeset');
if (root) {
var data = e.getNodeData('differential-inline-comment');
var change = e.getNodeData('differential-changeset');
var id_part = data.on_right ? change.right : change.left;
var new_part = data.isNewFile ? 'N' : 'O';
var prefix = 'C' + id_part + new_part + 'L';
origin = JX.$(prefix + data.number);
target = JX.$(prefix + (parseInt(data.number, 10) +
parseInt(data.length, 10)));
updateReticle();
}
updateReticleForComment(e);
}
});
@ -303,6 +317,12 @@ JX.behavior('differential-edit-inline-comments', function(config) {
}
var node = e.getNode('differential-inline-comment');
// If we're on a touch device, we didn't highlight the affected lines
// earlier because we can't use hover events to mutate the document.
// Highlight them now.
updateReticleForComment(e);
handle_inline_action(node, op);
};

5
webroot/rsrc/js/core/behavior-device.js
View file

@ -62,6 +62,11 @@ JX.install('Device', {
JX.Stratcom.invoke('phabricator-device-change', null, device);
},
isDesktop: function() {
var self = JX.Device;
return (self.getDevice() == 'desktop');
},
getDevice : function() {
var self = JX.Device;
if (self._device === null) {