2011-02-17 Ojan Vafai <ojan@chromium.org>
authorojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 21 Feb 2011 02:29:11 +0000 (02:29 +0000)
committerojan@chromium.org <ojan@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 21 Feb 2011 02:29:11 +0000 (02:29 +0000)
        Reviewed by Adam Barth.

        use actual browser focus in the code review tool
        https://bugs.webkit.org/show_bug.cgi?id=54726

        This makes keyboard handling play better with focusable element
        (i.e. links/textareas/buttons). Also, in theory, this makes
        the review tool more amenable to screen readers (I think).

        * PrettyPatch/PrettyPatch.rb:
        * code-review.js:

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

Websites/bugs.webkit.org/ChangeLog
Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb
Websites/bugs.webkit.org/code-review.js

index c8f8929..e8ec48e 100644 (file)
@@ -1,5 +1,19 @@
 2011-02-17  Ojan Vafai  <ojan@chromium.org>
 
+        Reviewed by Adam Barth.
+
+        use actual browser focus in the code review tool
+        https://bugs.webkit.org/show_bug.cgi?id=54726
+
+        This makes keyboard handling play better with focusable element
+        (i.e. links/textareas/buttons). Also, in theory, this makes
+        the review tool more amenable to screen readers (I think).
+
+        * PrettyPatch/PrettyPatch.rb:
+        * code-review.js:
+
+2011-02-17  Ojan Vafai  <ojan@chromium.org>
+
         Reviewed by Antonio Gomes.
 
         make event handling work in Gecko
index c33054e..1718268 100644 (file)
@@ -399,7 +399,7 @@ body {
   width: 6em;
 }
 
-.focused {
+div:focus {
   outline: 1px solid blue;
   outline-offset: -1px;
 }
index 9386493..b92000b 100644 (file)
@@ -1074,9 +1074,6 @@ var CODE_REVIEW_UNITTEST;
 
     updateToolbarAnchorState();
     loadDiffState();
-
-    // Ensure the body has focus so that it receives key events.
-    document.body.focus();
   });
 
   function handleReviewFormLoad() {
@@ -1337,20 +1334,23 @@ var CODE_REVIEW_UNITTEST;
   }
 
   function focusOn(node) {
-    // FIXME: We should probably give propery browser focus here.
-    $('.focused').removeClass('focused');
     if (node.length == 0)
       return;
-    $(document).scrollTop(node.addClass('focused').position().top - window.innerHeight / 2);
+
+    // Give a tabindex so the element can receive actual browser focus.
+    // -1 makes the element focusable without actually putting in in the tab order.
+    node.attr('tabindex', -1);
+    node.focus();
+    $(document).scrollTop(node.position().top - window.innerHeight / 2);
   }
 
   function focusNext(filter, direction) {
-    var focusable_nodes = $('.frozenComment,.previousComment,.DiffBlock,.overallComments').filter(function() {
+    var focusable_nodes = $('a,.frozenComment,.previousComment,.DiffBlock,.overallComments').filter(function() {
       return !$(this).hasClass('DiffBlock') || $('.add,.remove', this).size();
     });
 
     var is_backward = direction == DIRECTION.BACKWARD;
-    var index = focusable_nodes.index($('.focused'));
+    var index = focusable_nodes.index($(document.activeElement));
     if (index == -1 && is_backward)
       index = focusable_nodes.length;
 
@@ -1432,9 +1432,10 @@ var CODE_REVIEW_UNITTEST;
   }
   
   function handleEnterKeyPress() {
-    var focused = $('.focused');
-    if (!focused.size())
-      return false;
+    if (document.activeElement.nodeName == 'BODY')
+      return;
+
+    var focused = $(document.activeElement);
 
     if (focused.hasClass('frozenComment')) {
       unfreezeComment(focused);