2011-01-13 Ojan Vafai <ojan@chromium.org>

        Reviewed by Adam Barth.

        simplify keyboard handling in code review tool
        https://bugs.webkit.org/show_bug.cgi?id=52407

        Now that we have DiffBlock containers, the only things that are
        focusable are previousComment nodes and DiffBlock containers
        that contain add/remove lines.

        Also, this means we show the focus border around the entire diff
        instead of just the first line.

        * code-review.js:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@75754 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Websites/bugs.webkit.org/ChangeLog b/Websites/bugs.webkit.org/ChangeLog
index 310f782e43de8326d42e18933e7c658fe4dd0b32..23e8e8027e45849f22d1bc6cbe89b416b8987148 100644 (file)
--- a/Websites/bugs.webkit.org/ChangeLog
+++ b/Websites/bugs.webkit.org/ChangeLog
@@ -1,3 +1,19 @@
+2011-01-13  Ojan Vafai  <ojan@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        simplify keyboard handling in code review tool
+        https://bugs.webkit.org/show_bug.cgi?id=52407
+
+        Now that we have DiffBlock containers, the only things that are
+        focusable are previousComment nodes and DiffBlock containers
+        that contain add/remove lines.
+
+        Also, this means we show the focus border around the entire diff
+        instead of just the first line.
+
+        * code-review.js:
+
 2011-01-13  Ojan Vafai  <ojan@chromium.org>
 
         Reviewed by Adam Barth.

diff --git a/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb b/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb
index 880b8a72bfa205383c3462517ef4bd75c5b0c0e6..a0529d992df4529631cfaae45b48b30f127f1f50 100644 (file)
--- a/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb
+++ b/Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb
@@ -395,7 +395,7 @@ body {
 }
 </style>
 <script src="https://ajax.googleapis.com/ajax/libs/jquery/1.4.2/jquery.min.js"></script> 
-<script src="code-review.js?version=22"></script>
+<script src="code-review.js?version=23"></script>
 EOF
 
     def self.revisionOrDescription(string)

diff --git a/Websites/bugs.webkit.org/code-review.js b/Websites/bugs.webkit.org/code-review.js
index f2cf8a908164b2ce9df3961ce48a773f559154b1..a8bde40524acfeab06d2c299802e17d70a56b361 100644 (file)
--- a/Websites/bugs.webkit.org/code-review.js
+++ b/Websites/bugs.webkit.org/code-review.js
@@ -946,64 +946,20 @@
     $(document).scrollTop(node.addClass('focused').position().top - window.innerHeight / 2);
   }
 
-  function diffBlockEndPoint(focusable_nodes, line, line_offset, is_backward) {
-    if (!line_offset && is_backward)
-      return line;
-
-    var offset = is_backward ? -1 : 1;
-
-    // If we're at a comment, get a Line in the diff block that contains the comment.
-    if (line.hasClass('previousComment')) {
-      var line_for_comment = $('#' + line.attr('data-comment-for'));
-      line_offset = focusable_nodes.index(line_for_comment);
-
-      // If the comment is not inside a diff block, return the comment node.
-      if (line_offset == -1)
-        return line;
-
-      line = line_for_comment;
-    }
-
-    // Find the Line at the beginning/end of this contiguous block of lines.
-    // Contiguous blocks of lines have contiguous IDs and are contained in the same FileDiff.
-    // Skip over comment nodes.
-    var id = numberFrom(line.attr('id'));
-    var next_node = $(focusable_nodes[line_offset + offset]);
-    while (next_node.size()
-           && (!next_node.hasClass('Line') || next_node.attr('id') == 'line' + (id + offset))
-           && fileDiffFor(line)[0] == fileDiffFor(next_node)[0]) {
-      if (next_node.hasClass('Line')) {
-        line = next_node;
-        id += offset;
-      }
-
-      line_offset += offset;
-      next_node = $(focusable_nodes[line_offset + offset]);
-    }
-    return line;
-  }
-
   function focusNext(className, is_backward) {
-    var focusable_nodes = $('.previousComment,.Line.add,.Line.remove');
-    var focused_node = $('.focused');
-    var index = focusable_nodes.index(focused_node);
-    if (is_backward && (!index || index == -1))
-      return;
+    var focusable_nodes = $('.previousComment,.DiffBlock').filter(function() {
+      return ($(this).hasClass('previousComment') || $('.add,.remove', this).size());
+    });
 
-    if (focused_node.size() && className == 'Line') {
-      focused_node = diffBlockEndPoint(focusable_nodes, focused_node, index, is_backward);
-      index = focusable_nodes.index(focused_node);
-    }
+    var index = focusable_nodes.index($('.focused'));
+    if (index == -1 && is_backward)
+      index = focusable_nodes.length;
 
-    var end = is_backward ? 0 : focusable_nodes.size();
     var offset = is_backward ? -1 : 1;
+    var end = is_backward ? -1 : focusable_nodes.size();
     for (var i = index + offset; i != end; i = i + offset) {
       var node = $(focusable_nodes[i]);
       if (node.hasClass(className)) {
-        if (className == 'Line') {
-          // Pass in true for is_backward because we always want to focus the start of the diff block.
-          node = diffBlockEndPoint(focusable_nodes, node, i, true);
-        }
         focusOn(node);
         return;
       }
@@ -1031,11 +987,11 @@
       break;
 
     case kCharCodeForJ:
-      focusNext('Line', false);
+      focusNext('DiffBlock', false);
       break;
 
     case kCharCodeForK:
-      focusNext('Line', true);
+      focusNext('DiffBlock', true);
       break;
     }
   });