revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-01-15 17:21:10 +01:00
Code Issues Releases Wiki Activity

Don't mangle inline comments with tables in them in Differential

Browse source 
Summary:
Fixes T3814. Broadly, remarkup tables in inline comments did not work properly. I ran into several messes here and cleaned up some of them:

  - Some of this code is doing `JX.$N('div', {}, JX.$H(response.markup))`, to turn an HTML response into a node, passing that around, and then doing junk with it. This is super old and gross.
    - The slightly more modern pattern is `JX.$H(response.markup).getFragment().firstChild`, but this is kind of yuck too and not as safe as it could be.
    - Introduce `JX.$H(response.markup).getNode()`, which actually expresses intent here. We have a bunch of `getFragment().firstChild` callsites which should switch to this, but I didn't clean those up yet because I don't want to test them all.
    - Switch the `JX.$N('div', {}, JX.$H(response.markup))`-style callsites to `JX.$H(response.markup).getNode()`.
  - `copyRows()` is too aggressive in finding `<tr />` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `<tr />` tags which belong to some deeper table.
  - Once this is fixed, there's another bug with mousing over the cells in tables in inline comments. We select the nearest `<td />`, but that's the cell in the remarkup table. Instead, select the correct `<td />`.
  - At this point, these last two callsites were looking ugly. I provided `findAbove()` to clean them up.

Test Plan: Created, edited, deleted, moused over, and reloaded a revision with inline comments including remarkup tables. Used "Show more context" links.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3814

Differential Revision: https://secure.phabricator.com/D6924
This commit is contained in:
epriestley 2013-09-10 15:31:32 -07:00
parent 51eb8a301a
commit e8126fa958
5 changed files with 135 additions and 55 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

94
src/__celerity_resource_map__.php
View file

@ -1035,7 +1035,7 @@ celerity_register_resource_map(array(
),
'differential-inline-comment-editor' =>
array(
'uri' => '/res/37e0564f/rsrc/js/application/differential/DifferentialInlineCommentEditor.js',
'uri' => '/res/e952d210/rsrc/js/application/differential/DifferentialInlineCommentEditor.js',
'type' => 'js',
'requires' =>
array(
@ -1519,7 +1519,7 @@ celerity_register_resource_map(array(
),
'javelin-behavior-differential-edit-inline-comments' =>
array(
'uri' => '/res/86f459a4/rsrc/js/application/differential/behavior-edit-inline-comments.js',
'uri' => '/res/935d4012/rsrc/js/application/differential/behavior-edit-inline-comments.js',
'type' => 'js',
'requires' =>
array(
@ -1603,7 +1603,7 @@ celerity_register_resource_map(array(
),
'javelin-behavior-differential-show-more' =>
array(
'uri' => '/res/b9f93090/rsrc/js/application/differential/behavior-show-more.js',
'uri' => '/res/03b7bc9e/rsrc/js/application/differential/behavior-show-more.js',
'type' => 'js',
'requires' =>
array(
@ -2472,7 +2472,7 @@ celerity_register_resource_map(array(
),
'javelin-dom' =>
array(
'uri' => '/res/175211d6/rsrc/externals/javelin/lib/DOM.js',
'uri' => '/res/580c0aeb/rsrc/externals/javelin/lib/DOM.js',
'type' => 'js',
'requires' =>
array(
@ -4300,7 +4300,7 @@ celerity_register_resource_map(array(
'uri' => '/res/pkg/44bfe40c/differential.pkg.css',
'type' => 'css',
),
'd07a3bc2' =>
'5e9e5c4e' =>
array(
'name' => 'differential.pkg.js',
'symbols' =>
@ -4325,7 +4325,7 @@ celerity_register_resource_map(array(
17 => 'javelin-behavior-differential-toggle-files',
18 => 'javelin-behavior-differential-user-select',
),
'uri' => '/res/pkg/d07a3bc2/differential.pkg.js',
'uri' => '/res/pkg/5e9e5c4e/differential.pkg.js',
'type' => 'js',
),
'c8ce2d88' =>
@ -4351,7 +4351,7 @@ celerity_register_resource_map(array(
'uri' => '/res/pkg/96909266/diffusion.pkg.js',
'type' => 'js',
),
'2dbbb7d1' =>
'f32597c9' =>
array(
'name' => 'javelin.pkg.js',
'symbols' =>
@ -4377,7 +4377,7 @@ celerity_register_resource_map(array(
18 => 'javelin-tokenizer',
19 => 'javelin-history',
),
'uri' => '/res/pkg/2dbbb7d1/javelin.pkg.js',
'uri' => '/res/pkg/f32597c9/javelin.pkg.js',
'type' => 'js',
),
'0a9e494f' =>
@ -4420,7 +4420,7 @@ celerity_register_resource_map(array(
'aphront-typeahead-control-css' => '6d1ec88f',
'differential-changeset-view-css' => '44bfe40c',
'differential-core-view-css' => '44bfe40c',
'differential-inline-comment-editor' => 'd07a3bc2',
'differential-inline-comment-editor' => '5e9e5c4e',
'differential-local-commits-view-css' => '44bfe40c',
'differential-results-table-css' => '44bfe40c',
'differential-revision-add-comment-css' => '44bfe40c',
@ -4434,27 +4434,27 @@ celerity_register_resource_map(array(
'global-drag-and-drop-css' => '6d1ec88f',
'inline-comment-summary-css' => '44bfe40c',
'javelin-aphlict' => '8977e356',
'javelin-behavior' => '2dbbb7d1',
'javelin-behavior' => 'f32597c9',
'javelin-behavior-aphlict-dropdown' => '8977e356',
'javelin-behavior-aphlict-listen' => '8977e356',
'javelin-behavior-aphront-basic-tokenizer' => '8977e356',
'javelin-behavior-aphront-drag-and-drop-textarea' => 'd07a3bc2',
'javelin-behavior-aphront-drag-and-drop-textarea' => '5e9e5c4e',
'javelin-behavior-aphront-form-disable-on-submit' => '8977e356',
'javelin-behavior-audit-preview' => '96909266',
'javelin-behavior-dark-console' => '4ccfeb47',
'javelin-behavior-device' => '8977e356',
'javelin-behavior-differential-accept-with-errors' => 'd07a3bc2',
'javelin-behavior-differential-add-reviewers-and-ccs' => 'd07a3bc2',
'javelin-behavior-differential-comment-jump' => 'd07a3bc2',
'javelin-behavior-differential-diff-radios' => 'd07a3bc2',
'javelin-behavior-differential-dropdown-menus' => 'd07a3bc2',
'javelin-behavior-differential-edit-inline-comments' => 'd07a3bc2',
'javelin-behavior-differential-feedback-preview' => 'd07a3bc2',
'javelin-behavior-differential-keyboard-navigation' => 'd07a3bc2',
'javelin-behavior-differential-populate' => 'd07a3bc2',
'javelin-behavior-differential-show-more' => 'd07a3bc2',
'javelin-behavior-differential-toggle-files' => 'd07a3bc2',
'javelin-behavior-differential-user-select' => 'd07a3bc2',
'javelin-behavior-differential-accept-with-errors' => '5e9e5c4e',
'javelin-behavior-differential-add-reviewers-and-ccs' => '5e9e5c4e',
'javelin-behavior-differential-comment-jump' => '5e9e5c4e',
'javelin-behavior-differential-diff-radios' => '5e9e5c4e',
'javelin-behavior-differential-dropdown-menus' => '5e9e5c4e',
'javelin-behavior-differential-edit-inline-comments' => '5e9e5c4e',
'javelin-behavior-differential-feedback-preview' => '5e9e5c4e',
'javelin-behavior-differential-keyboard-navigation' => '5e9e5c4e',
'javelin-behavior-differential-populate' => '5e9e5c4e',
'javelin-behavior-differential-show-more' => '5e9e5c4e',
'javelin-behavior-differential-toggle-files' => '5e9e5c4e',
'javelin-behavior-differential-user-select' => '5e9e5c4e',
'javelin-behavior-diffusion-commit-graph' => '96909266',
'javelin-behavior-diffusion-pull-lastmodified' => '96909266',
'javelin-behavior-error-log' => '4ccfeb47',
@ -4462,7 +4462,7 @@ celerity_register_resource_map(array(
'javelin-behavior-history-install' => '8977e356',
'javelin-behavior-konami' => '8977e356',
'javelin-behavior-lightbox-attachments' => '8977e356',
'javelin-behavior-load-blame' => 'd07a3bc2',
'javelin-behavior-load-blame' => '5e9e5c4e',
'javelin-behavior-maniphest-batch-selector' => '83a3853e',
'javelin-behavior-maniphest-subpriority-editor' => '83a3853e',
'javelin-behavior-maniphest-transaction-controls' => '83a3853e',
@ -4474,7 +4474,7 @@ celerity_register_resource_map(array(
'javelin-behavior-phabricator-hovercards' => '8977e356',
'javelin-behavior-phabricator-keyboard-shortcuts' => '8977e356',
'javelin-behavior-phabricator-nav' => '8977e356',
'javelin-behavior-phabricator-object-selector' => 'd07a3bc2',
'javelin-behavior-phabricator-object-selector' => '5e9e5c4e',
'javelin-behavior-phabricator-oncopy' => '8977e356',
'javelin-behavior-phabricator-remarkup-assist' => '8977e356',
'javelin-behavior-phabricator-reveal-content' => '8977e356',
@ -4482,28 +4482,28 @@ celerity_register_resource_map(array(
'javelin-behavior-phabricator-tooltips' => '8977e356',
'javelin-behavior-phabricator-watch-anchor' => '8977e356',
'javelin-behavior-refresh-csrf' => '8977e356',
'javelin-behavior-repository-crossreference' => 'd07a3bc2',
'javelin-behavior-repository-crossreference' => '5e9e5c4e',
'javelin-behavior-toggle-class' => '8977e356',
'javelin-behavior-workflow' => '8977e356',
'javelin-dom' => '2dbbb7d1',
'javelin-event' => '2dbbb7d1',
'javelin-history' => '2dbbb7d1',
'javelin-install' => '2dbbb7d1',
'javelin-json' => '2dbbb7d1',
'javelin-mask' => '2dbbb7d1',
'javelin-request' => '2dbbb7d1',
'javelin-resource' => '2dbbb7d1',
'javelin-stratcom' => '2dbbb7d1',
'javelin-tokenizer' => '2dbbb7d1',
'javelin-typeahead' => '2dbbb7d1',
'javelin-typeahead-normalizer' => '2dbbb7d1',
'javelin-typeahead-ondemand-source' => '2dbbb7d1',
'javelin-typeahead-preloaded-source' => '2dbbb7d1',
'javelin-typeahead-source' => '2dbbb7d1',
'javelin-uri' => '2dbbb7d1',
'javelin-util' => '2dbbb7d1',
'javelin-vector' => '2dbbb7d1',
'javelin-workflow' => '2dbbb7d1',
'javelin-dom' => 'f32597c9',
'javelin-event' => 'f32597c9',
'javelin-history' => 'f32597c9',
'javelin-install' => 'f32597c9',
'javelin-json' => 'f32597c9',
'javelin-mask' => 'f32597c9',
'javelin-request' => 'f32597c9',
'javelin-resource' => 'f32597c9',
'javelin-stratcom' => 'f32597c9',
'javelin-tokenizer' => 'f32597c9',
'javelin-typeahead' => 'f32597c9',
'javelin-typeahead-normalizer' => 'f32597c9',
'javelin-typeahead-ondemand-source' => 'f32597c9',
'javelin-typeahead-preloaded-source' => 'f32597c9',
'javelin-typeahead-source' => 'f32597c9',
'javelin-uri' => 'f32597c9',
'javelin-util' => 'f32597c9',
'javelin-vector' => 'f32597c9',
'javelin-workflow' => 'f32597c9',
'lightbox-attachment-css' => '6d1ec88f',
'maniphest-task-summary-css' => '0a9e494f',
'maniphest-transaction-detail-css' => '0a9e494f',
@ -4513,7 +4513,7 @@ celerity_register_resource_map(array(
'phabricator-content-source-view-css' => '44bfe40c',
'phabricator-core-css' => '6d1ec88f',
'phabricator-crumbs-view-css' => '6d1ec88f',
'phabricator-drag-and-drop-file-upload' => 'd07a3bc2',
'phabricator-drag-and-drop-file-upload' => '5e9e5c4e',
'phabricator-dropdown-menu' => '8977e356',
'phabricator-file-upload' => '8977e356',
'phabricator-filetree-view-css' => '6d1ec88f',
@ -4535,7 +4535,7 @@ celerity_register_resource_map(array(
'phabricator-project-tag-css' => '0a9e494f',
'phabricator-property-list-view-css' => '6d1ec88f',
'phabricator-remarkup-css' => '6d1ec88f',
'phabricator-shaped-request' => 'd07a3bc2',
'phabricator-shaped-request' => '5e9e5c4e',
'phabricator-side-menu-view-css' => '6d1ec88f',
'phabricator-standard-page-view' => '6d1ec88f',
'phabricator-tag-view-css' => '6d1ec88f',

71
webroot/rsrc/externals/javelin/lib/DOM.js vendored
View file

@ -157,7 +157,29 @@ JX.install('HTML', {
fragment.appendChild(wrapper.removeChild(wrapper.firstChild));
}
return fragment;
},
/**
* Convert the raw HTML string into a single DOM node. This only works
* if the element has a single top-level element. Otherwise, use
* @{method:getFragment} to get a document fragment instead.
*
* @return Node Single node represented by the object.
* @task nodes
*/
getNode : function() {
var fragment = this.getFragment();
if (__DEV__) {
if (fragment.childNodes.length < 1) {
JX.$E('JX.HTML.getNode(): Markup has no root node!');
}
if (fragment.childNodes.length > 1) {
JX.$E('JX.HTML.getNode(): Markup has more than one root node!');
}
}
return fragment.firstChild;
}
}
});
@ -856,14 +878,59 @@ JX.install('DOM', {
}
if (!result.length) {
JX.$E('JX.DOM.find(<node>, "' +
tagname + '", "' + sigil + '"): '+ 'matched no nodes.');
JX.$E(
'JX.DOM.find(<node>, "' + tagname + '", "' + sigil + '"): ' +
'matched no nodes.');
}
return result[0];
},
/**
* Select a node uniquely identified by an anchor, tagname, and sigil. This
* is similar to JX.DOM.find() but walks up the DOM tree instead of down
* it.
*
* @param Node Node to look above.
* @param string Tag name, like 'a' or 'textarea'.
* @param string Optionally, sigil which selected node must have.
* @return Node Matching node.
*
* @task query
*/
findAbove : function(anchor, tagname, sigil) {
if (__DEV__) {
if (!JX.DOM.isNode(anchor)) {
JX.$E(
'JX.DOM.findAbove(<glop>, "' + tagname + '", "' + sigil + '"): ' +
'first argument must be a DOM node.');
}
}
var result = anchor.parentNode;
while (true) {
if (!result) {
break;
}
if (JX.DOM.isType(result, tagname)) {
if (!sigil || JX.Stratcom.hasSigil(result, sigil)) {
break;
}
}
result = result.parentNode;
}
if (!result) {
JX.$E(
'JX.DOM.findAbove(<node>, "' + tagname + '", "' + sigil + '"): ' +
'no matching node.');
}
return result;
},
/**
* Focus a node safely. This is just a convenience wrapper that allows you
* to avoid IE's habit of throwing when nearly any focus operation is

6
webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js
View file

@ -79,7 +79,7 @@ JX.install('DifferentialInlineCommentEditor', {
JX.DOM.alterClass(row, 'differential-inline-loading', is_loading);
},
_didContinueWorkflow : function(response) {
var drawn = this._draw(JX.$N('div', JX.$H(response)));
var drawn = this._draw(JX.$H(response).getNode());
var op = this.getOperation();
if (op == 'edit') {
@ -131,7 +131,7 @@ JX.install('DifferentialInlineCommentEditor', {
// We don't get any markup back if the user deletes a comment, or saves
// an empty comment (which effects a delete).
if (response.markup) {
this._draw(JX.$N('div', JX.$H(response.markup)));
this._draw(JX.$H(response.markup).getNode());
}
// These operations remove the old row (edit adds a new row first).
@ -190,7 +190,7 @@ JX.install('DifferentialInlineCommentEditor', {
var templates = this.getTemplates();
var template = this.getOnRight() ? templates.r : templates.l;
template = JX.$N('div', JX.$H(template));
template = JX.$H(template).getNode();
// NOTE: Operation order matters here; we can't remove anything until
// after we draw the new rows because _draw uses the old rows to figure

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

@ -175,7 +175,13 @@ JX.behavior('differential-edit-inline-comments', function(config) {
var change = e.getNodeData('differential-changeset');
var id_part = data.on_right ? change.right : change.left;
var th = e.getNode('tag:td').previousSibling;
// NOTE: We can't just look for 'tag:td' because the event might be
// inside a table which is inside an inline comment.
var comment = e.getNode('differential-inline-comment');
var td = JX.DOM.findAbove(comment, 'td');
var th = td.previousSibling;
var new_part = isNewFile(th) ? 'N' : 'O';
var prefix = 'C' + id_part + new_part + 'L';

11
webroot/rsrc/js/application/differential/behavior-show-more.js
View file

@ -10,9 +10,9 @@
JX.behavior('differential-show-more', function(config) {
function onresponse(context, response) {
var div = JX.$N('div', {}, JX.$H(response.changeset));
var table = JX.$H(response.changeset).getNode();
var root = context.parentNode;
copyRows(root, div, context);
copyRows(root, table, context);
root.removeChild(context);
}
@ -54,6 +54,13 @@ JX.behavior('differential-show-more', function(config) {
function copyRows(dst, src, before) {
var rows = JX.DOM.scry(src, 'tr');
for (var ii = 0; ii < rows.length; ii++) {
// Find the table this <tr /> belongs to. If it's a sub-table, like a
// table in an inline comment, don't copy it.
if (JX.DOM.findAbove(rows[ii], 'table') !== src) {
continue;
}
if (before) {
dst.insertBefore(rows[ii], before);
} else {