From e8126fa95858a1e3376b84fd476d001a2600c513 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 10 Sep 2013 15:31:32 -0700 Subject: [PATCH] Don't mangle inline comments with tables in them in Differential 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 `` tags. This actually causes the bug in T3814. We only want to find these tags at top level, not all tags. Don't copy `` 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 ``, but that's the cell in the remarkup table. Instead, select the correct ``. - 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 --- src/__celerity_resource_map__.php | 94 +++++++++---------- webroot/rsrc/externals/javelin/lib/DOM.js | 71 +++++++++++++- .../DifferentialInlineCommentEditor.js | 6 +- .../behavior-edit-inline-comments.js | 8 +- .../differential/behavior-show-more.js | 11 ++- 5 files changed, 135 insertions(+), 55 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 34480f9005..1308c015d5 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -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', diff --git a/webroot/rsrc/externals/javelin/lib/DOM.js b/webroot/rsrc/externals/javelin/lib/DOM.js index 95496f4887..5c94d406bc 100644 --- a/webroot/rsrc/externals/javelin/lib/DOM.js +++ b/webroot/rsrc/externals/javelin/lib/DOM.js @@ -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(, "' + - tagname + '", "' + sigil + '"): '+ 'matched no nodes.'); + JX.$E( + 'JX.DOM.find(, "' + 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(, "' + 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(, "' + 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 diff --git a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js index b0a2ec8e3b..7d24af836f 100644 --- a/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js +++ b/webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js @@ -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 diff --git a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js index 1ab0edae23..1d83eab062 100644 --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js @@ -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'; diff --git a/webroot/rsrc/js/application/differential/behavior-show-more.js b/webroot/rsrc/js/application/differential/behavior-show-more.js index 2c44b798c3..14e0d9feb1 100644 --- a/webroot/rsrc/js/application/differential/behavior-show-more.js +++ b/webroot/rsrc/js/application/differential/behavior-show-more.js @@ -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 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 {