From 616976a45c83dbb675623562328a8f324a855f0c Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 7 Mar 2015 11:47:55 -0800 Subject: [PATCH] Make inline reticle code more general and robust Summary: Ref T2009. Currently, the code which draws the reticle is sort of implicitly hard-coded with some of the rules for the 2up view. Instead, use general rules: - Start selection at the next ``. - End selection at the rightmost adjacent ``. These rules work in all cases. Test Plan: - Activated reticle in 1up and 2up views by clicking line numbers and hovering over comments. It now draws correctly. - Dragged over line ranges in 1up and 2up views, saw accurate reticle. Reviewers: btrahan, chad Reviewed By: chad Subscribers: epriestley Maniphest Tasks: T2009 Differential Revision: https://secure.phabricator.com/D12009 --- resources/celerity/map.php | 28 ++++++++--------- .../differential/changeset-view.css | 1 + .../behavior-edit-inline-comments.js | 30 ++++++++++++++----- 3 files changed, 38 insertions(+), 21 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index b6a870abd9..c09dd3e94b 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,8 +10,8 @@ return array( 'core.pkg.css' => '6408f2d3', 'core.pkg.js' => '5a1c336d', 'darkconsole.pkg.js' => '8ab24e01', - 'differential.pkg.css' => '6641cdd5', - 'differential.pkg.js' => '3fab5259', + 'differential.pkg.css' => 'df94a9e2', + 'differential.pkg.js' => 'f458a7dc', 'diffusion.pkg.css' => '591664fa', 'diffusion.pkg.js' => 'bfc0737b', 'maniphest.pkg.css' => '68d4dd3d', @@ -55,7 +55,7 @@ return array( 'rsrc/css/application/dashboard/dashboard.css' => '17937d22', 'rsrc/css/application/diff/inline-comment-summary.css' => 'eb5f8e8c', 'rsrc/css/application/differential/add-comment.css' => 'c478bcaa', - 'rsrc/css/application/differential/changeset-view.css' => 'bad09138', + 'rsrc/css/application/differential/changeset-view.css' => '88100552', 'rsrc/css/application/differential/core.css' => '7ac3cabc', 'rsrc/css/application/differential/results-table.css' => '181aa9d9', 'rsrc/css/application/differential/revision-comment.css' => '48186045', @@ -367,7 +367,7 @@ return array( 'rsrc/js/application/differential/behavior-comment-preview.js' => '6932def3', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', 'rsrc/js/application/differential/behavior-dropdown-menus.js' => '2035b9cb', - 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'ae8e9b44', + 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '33d4c5e2', 'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492', 'rsrc/js/application/differential/behavior-populate.js' => '8694b1df', 'rsrc/js/application/differential/behavior-show-field-details.js' => 'bba9eedf', @@ -518,7 +518,7 @@ return array( 'conpherence-notification-css' => '04a6e10a', 'conpherence-update-css' => '1099a660', 'conpherence-widget-pane-css' => '3d575438', - 'differential-changeset-view-css' => 'bad09138', + 'differential-changeset-view-css' => '88100552', 'differential-core-view-css' => '7ac3cabc', 'differential-inline-comment-editor' => '41060c54', 'differential-results-table-css' => '181aa9d9', @@ -569,7 +569,7 @@ return array( 'javelin-behavior-differential-comment-jump' => '4fdb476d', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-dropdown-menus' => '2035b9cb', - 'javelin-behavior-differential-edit-inline-comments' => 'ae8e9b44', + 'javelin-behavior-differential-edit-inline-comments' => '33d4c5e2', 'javelin-behavior-differential-feedback-preview' => '6932def3', 'javelin-behavior-differential-keyboard-navigation' => '2c426492', 'javelin-behavior-differential-populate' => '8694b1df', @@ -1027,6 +1027,14 @@ return array( '331b1611' => array( 'javelin-install', ), + '33d4c5e2' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-util', + 'javelin-vector', + 'differential-inline-comment-editor', + ), '3ab51e2c' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -1645,14 +1653,6 @@ return array( 'javelin-util', 'phabricator-prefab', ), - 'ae8e9b44' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-util', - 'javelin-vector', - 'differential-inline-comment-editor', - ), 'b1f0ccee' => array( 'javelin-install', 'javelin-dom', diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index 400f88e578..4cdf7e09ea 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -268,6 +268,7 @@ td.cov-I { opacity: 0.5; top: 0px; left: 0px; + box-sizing: border-box; } .differential-inline-comment, 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 20b5e74785..9a3a783a61 100644 --- a/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js +++ b/webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js @@ -31,16 +31,32 @@ JX.behavior('differential-edit-inline-comments', function(config) { top = bot; bot = tmp; } - var code = target.nextSibling; - var pos = JX.$V(top) - .add(JX.Vector.getAggregateScrollForNode(top)) - .add(1 + JX.Vector.getDim(target).x, 0); - var dim = JX.Vector.getDim(code).add(-4, 0); - if (isOnRight(target)) { - dim.x += JX.Vector.getDim(code.nextSibling).x; + // Find the leftmost cell that we're going to highlight: this is the next + // in the row. In 2up views, it should be directly adjacent. In + // 1up views, we may have to skip over the other line number column. + var l = top; + while (JX.DOM.isType(l, 'th')) { + l = l.nextSibling; } + // Find the rightmost cell that we're going to highlight: this is the + // farthest consecutive, adjacent in the row. Sometimes the left + // and right nodes are the same (left side of 2up view); sometimes we're + // going to highlight several nodes (copy + code + coverage). + var r = l; + while (r.nextSibling && JX.DOM.isType(r.nextSibling, 'td')) { + r = r.nextSibling; + } + + var pos = JX.$V(l) + .add(JX.Vector.getAggregateScrollForNode(l)); + + var dim = JX.$V(r) + .add(JX.Vector.getAggregateScrollForNode(r)) + .add(-pos.x, -pos.y) + .add(JX.Vector.getDim(r)); + var bpos = JX.$V(bot) .add(JX.Vector.getAggregateScrollForNode(bot)); dim.y = (bpos.y - pos.y) + JX.Vector.getDim(bot).y;