Skip to content
This repository
Browse code

Refactor handlerStack. Closes #657.

Previously, handlerStack was designed only for removal of the handler
right at the top of the stack. However, some handlers sought to remove
themselves when they were not at the top of the stack, creating
confusion. The new handlerStack ensures that such removal can always be
done safely.
  • Loading branch information...
commit ea73be17468b1bd02171c82e1fb21b1bc16ccd0e 1 parent 2ed217b
Jez Ng authored October 20, 2012
1  .gitignore
@@ -3,3 +3,4 @@
3 3
 *.swp
4 4
 *.crx
5 5
 *.js
  6
+node_modules/*
6  content_scripts/link_hints.coffee
@@ -63,7 +63,7 @@ LinkHints =
@@ -163,7 +163,7 @@ LinkHints =
@@ -231,7 +231,7 @@ LinkHints =
45  content_scripts/vimium_frontend.coffee
@@ -10,7 +10,7 @@ findModeQuery = { rawQuery: "" }
10 10
 findModeQueryHasResults = false
11 11
 findModeAnchorNode = null
12 12
 isShowingHelpDialog = false
13  
-handlerStack = []
  13
+handlerStack = new HandlerStack
14 14
 keyPort = null
15 15
 # Users can disable Vimium on URL patterns via the settings page.
16 16
 isEnabledForUrl = true
@@ -355,7 +355,9 @@ extend window,
355 355
         visibleInputs[selectedInputIndex].element.focus()
356 356
       else unless event.keyCode == KeyboardUtils.keyCodes.shiftKey
357 357
         DomUtils.removeElement hintContainingDiv
358  
-        handlerStack.pop()
  358
+        @remove()
  359
+
  360
+      false
359 361
 
360 362
 #
361 363
 # Sends everything except i & ESC to the handler in background_page. i & ESC are special because they control
@@ -365,7 +367,7 @@ extend window,
365 367
 # Note that some keys will only register keydown events and not keystroke events, e.g. ESC.
366 368
 #
367 369
 onKeypress = (event) ->
368  
-  return unless bubbleEvent('keypress', event)
  370
+  return unless handlerStack.bubbleEvent('keypress', event)
369 371
 
370 372
   keyChar = ""
371 373
 
@@ -381,32 +383,15 @@ onKeypress = (event) ->
381 383
     if (keyChar)
382 384
       if (findMode)
383 385
         handleKeyCharForFindMode(keyChar)
384  
-        suppressEvent(event)
  386
+        DomUtils.suppressEvent(event)
385 387
       else if (!isInsertMode() && !findMode)
386 388
         if (currentCompletionKeys.indexOf(keyChar) != -1)
387  
-          suppressEvent(event)
  389
+          DomUtils.suppressEvent(event)
388 390
 
389 391
         keyPort.postMessage({ keyChar:keyChar, frameId:frameId })
390 392
 
391  
-#
392  
-# Called whenever we receive a key event.  Each individual handler has the option to stop the event's
393  
-# propagation by returning a falsy value.
394  
-#
395  
-bubbleEvent = (type, event) ->
396  
-  for i in [(handlerStack.length - 1)..0]
397  
-    # We need to check for existence of handler because the last function call may have caused the release of
398  
-    # more than one handler.
399  
-    if (handlerStack[i] && handlerStack[i][type] && !handlerStack[i][type](event))
400  
-      suppressEvent(event)
401  
-      return false
402  
-  true
403  
-
404  
-suppressEvent = (event) ->
405  
-  event.preventDefault()
406  
-  event.stopPropagation()
407  
-
408 393
 onKeydown = (event) ->
409  
-  return unless bubbleEvent('keydown', event)
  394
+  return unless handlerStack.bubbleEvent('keydown', event)
410 395
 
411 396
   keyChar = ""
412 397
 
@@ -442,20 +427,20 @@ onKeydown = (event) ->
442 427
       if (isEditable(event.srcElement))
443 428
         event.srcElement.blur()
444 429
       exitInsertMode()
445  
-      suppressEvent(event)
  430
+      DomUtils.suppressEvent(event)
446 431
 
447 432
   else if (findMode)
448 433
     if (KeyboardUtils.isEscape(event))
449 434
       handleEscapeForFindMode()
450  
-      suppressEvent(event)
  435
+      DomUtils.suppressEvent(event)
451 436
 
452 437
     else if (event.keyCode == keyCodes.backspace || event.keyCode == keyCodes.deleteKey)
453 438
       handleDeleteForFindMode()
454  
-      suppressEvent(event)
  439
+      DomUtils.suppressEvent(event)
455 440
 
456 441
     else if (event.keyCode == keyCodes.enter)
457 442
       handleEnterForFindMode()
458  
-      suppressEvent(event)
  443
+      DomUtils.suppressEvent(event)
459 444
 
460 445
     else if (!modifiers)
461 446
       event.stopPropagation()
@@ -466,7 +451,7 @@ onKeydown = (event) ->
466 451
   else if (!isInsertMode() && !findMode)
467 452
     if (keyChar)
468 453
       if (currentCompletionKeys.indexOf(keyChar) != -1)
469  
-        suppressEvent(event)
  454
+        DomUtils.suppressEvent(event)
470 455
 
471 456
       keyPort.postMessage({ keyChar:keyChar, frameId:frameId })
472 457
 
@@ -485,7 +470,7 @@ onKeydown = (event) ->
485 470
       isValidFirstKey(KeyboardUtils.getKeyChar(event))))
486 471
     event.stopPropagation()
487 472
 
488  
-onKeyup = () -> return unless bubbleEvent('keyup', event)
  473
+onKeyup = (event) -> return unless handlerStack.bubbleEvent('keyup', event)
489 474
 
490 475
 checkIfEnabledForUrl = ->
491 476
   url = window.location.toString()
@@ -750,7 +735,7 @@ findAndFocus = (backwards) ->
750 735
   if (elementCanTakeInput)
751 736
     handlerStack.push({
752 737
       keydown: (event) ->
753  
-        handlerStack.pop()
  738
+        @remove()
754 739
         if (KeyboardUtils.isEscape(event))
755 740
           DomUtils.simulateSelect(document.activeElement)
756 741
           enterInsertModeWithoutShowingIndicator(document.activeElement)
4  content_scripts/vomnibar.coffee
@@ -52,13 +52,13 @@ class VomnibarUI
52 52
   show: ->
53 53
     @box.style.display = "block"
54 54
     @input.focus()
55  
-    handlerStack.push({ keydown: @onKeydown.bind(this) })
  55
+    @handlerId = handlerStack.push keydown: @onKeydown.bind @
56 56
 
57 57
   hide: ->
58 58
     @box.style.display = "none"
59 59
     @completionList.style.display = "none"
60 60
     @input.blur()
61  
-    handlerStack.pop()
  61
+    handlerStack.remove @handlerId
62 62
 
63 63
   reset: ->
64 64
     @input.value = ""
4  lib/dom_utils.coffee
@@ -131,5 +131,9 @@ DomUtils =
131 131
     document.documentElement.appendChild(flashEl)
132 132
     setTimeout((-> DomUtils.removeElement flashEl), 400)
133 133
 
  134
+  suppressEvent: (event) ->
  135
+    event.preventDefault()
  136
+    event.stopPropagation()
  137
+
134 138
 root = exports ? window
135 139
 root.DomUtils = DomUtils
37  lib/handler_stack.coffee
... ...
@@ -0,0 +1,37 @@
  1
+root = exports ? window
  2
+
  3
+class root.HandlerStack
  4
+
  5
+  constructor: ->
  6
+    @stack = []
  7
+    @counter = 0
  8
+
  9
+  genId: -> @counter = ++@counter & 0xffff
  10
+
  11
+  # Adds a handler to the stack. Returns a unique ID for that handler that can be used to remove it later.
  12
+  push: (handler) ->
  13
+    handler.id = @genId()
  14
+    @stack.push handler
  15
+    handler.id
  16
+
  17
+  # Called whenever we receive a key event. Each individual handler has the option to stop the event's
  18
+  # propagation by returning a falsy value.
  19
+  bubbleEvent: (type, event) ->
  20
+    for i in [(@stack.length - 1)..0] by -1
  21
+      handler = @stack[i]
  22
+      # We need to check for existence of handler because the last function call may have caused the release
  23
+      # of more than one handler.
  24
+      if handler && handler[type]
  25
+        @currentId = handler.id
  26
+        passThrough = handler[type].call(@, event)
  27
+        if not passThrough
  28
+          DomUtils.suppressEvent(event)
  29
+          return false
  30
+    true
  31
+
  32
+  remove: (id = @currentId) ->
  33
+    for i in [(@stack.length - 1)..0] by -1
  34
+      handler = @stack[i]
  35
+      if handler.id == id
  36
+        @stack.splice(i, 1)
  37
+        break
1  manifest.json
@@ -30,6 +30,7 @@
30 30
       "js": ["lib/utils.js",
31 31
              "lib/keyboard_utils.js",
32 32
              "lib/dom_utils.js",
  33
+             "lib/handler_stack.js",
33 34
              "lib/clipboard.js",
34 35
              "content_scripts/link_hints.js",
35 36
              "content_scripts/vomnibar.js",
2  test_harnesses/vomnibar.html
@@ -15,7 +15,7 @@
15 15
   <link rel="stylesheet" type="text/css" href="../vimium.css" />
16 16
   <script>
17 17
     function setup() {
18  
-      window.handlerStack = [];
  18
+      window.handlerStack = new HandlerStack();
19 19
       // This itemHtml was obtained just by copying and pasting what was generated when using it in practice.
20 20
       var itemHtml = '<span class="source">history</span> http://<span class="fuzzyMatch">n</span><span class="fuzzyMatch">i</span><span class="fuzzyMatch">n</span><span class="fuzzyMatch">j</span><span class="fuzzyMatch">a</span>words.com/info/about <span class="title">Ninjawords - a really fast dictionary</span>';
21 21
       var results = [{ html: itemHtml }, { html: itemHtml }];
4  tests/dom_tests/dom_tests.coffee
@@ -177,11 +177,11 @@ context "Input focus",
177 177
     focusInput 1
178 178
     assert.equal "first", document.activeElement.id
179 179
     # deactivate the tabbing mode and its overlays
180  
-    handlerStack[handlerStack.length - 1].keydown mockKeyboardEvent("A")
  180
+    handlerStack.bubbleEvent 'keydown', mockKeyboardEvent("A")
181 181
 
182 182
     focusInput 100
183 183
     assert.equal "third", document.activeElement.id
184  
-    handlerStack[handlerStack.length - 1].keydown mockKeyboardEvent("A")
  184
+    handlerStack.bubbleEvent 'keydown', mockKeyboardEvent("A")
185 185
 
186 186
 Tests.outputMethod = (args...) ->
187 187
   newOutput = args.join "\n"
1  tests/dom_tests/dom_tests.html
@@ -32,6 +32,7 @@
32 32
     <script type="text/javascript" src="../../lib/utils.js"></script>
33 33
     <script type="text/javascript" src="../../lib/keyboard_utils.js"></script>
34 34
     <script type="text/javascript" src="../../lib/dom_utils.js"></script>
  35
+    <script type="text/javascript" src="../../lib/handler_stack.js"></script>
35 36
     <script type="text/javascript" src="../../lib/clipboard.js"></script>
36 37
     <script type="text/javascript" src="../../content_scripts/link_hints.js"></script>
37 38
     <script type="text/javascript" src="../../content_scripts/vomnibar.js"></script>
52  tests/unit_tests/handler_stack_test.coffee
... ...
@@ -0,0 +1,52 @@
  1
+require "./test_helper.js"
  2
+extend(global, require "../../lib/handler_stack.js")
  3
+
  4
+context "handlerStack",
  5
+  setup ->
  6
+    stub global, "DomUtils", {}
  7
+    stub DomUtils, "suppressEvent", ->
  8
+    @handlerStack = new HandlerStack
  9
+    @handler1Called = false
  10
+    @handler2Called = false
  11
+
  12
+  should "bubble events", ->
  13
+    @handlerStack.push { keydown: => @handler1Called = true }
  14
+    @handlerStack.push { keydown: => @handler2Called = true }
  15
+    @handlerStack.bubbleEvent 'keydown', {}
  16
+    assert.isTrue @handler2Called
  17
+    assert.isTrue @handler1Called
  18
+
  19
+  should "terminate bubbling on falsy return value", ->
  20
+    @handlerStack.push { keydown: => @handler1Called = true }
  21
+    @handlerStack.push { keydown: => @handler2Called = true; false }
  22
+    @handlerStack.bubbleEvent 'keydown', {}
  23
+    assert.isTrue @handler2Called
  24
+    assert.isFalse @handler1Called
  25
+
  26
+  should "remove handlers correctly", ->
  27
+    @handlerStack.push { keydown: => @handler1Called = true }
  28
+    handlerId = @handlerStack.push { keydown: => @handler2Called = true }
  29
+    @handlerStack.remove handlerId
  30
+    @handlerStack.bubbleEvent 'keydown', {}
  31
+    assert.isFalse @handler2Called
  32
+    assert.isTrue @handler1Called
  33
+
  34
+  should "remove handlers correctly", ->
  35
+    handlerId = @handlerStack.push { keydown: => @handler1Called = true }
  36
+    @handlerStack.push { keydown: => @handler2Called = true }
  37
+    @handlerStack.remove handlerId
  38
+    @handlerStack.bubbleEvent 'keydown', {}
  39
+    assert.isTrue @handler2Called
  40
+    assert.isFalse @handler1Called
  41
+
  42
+  should "handle self-removing handlers correctly", ->
  43
+    ctx = @
  44
+    @handlerStack.push { keydown: => @handler1Called = true }
  45
+    @handlerStack.push { keydown: ->
  46
+      ctx.handler2Called = true
  47
+      @remove()
  48
+    }
  49
+    @handlerStack.bubbleEvent 'keydown', {}
  50
+    assert.isTrue @handler2Called
  51
+    assert.isTrue @handler1Called
  52
+    assert.equal @handlerStack.stack.length, 1

1 note on commit ea73be1

Phil Crosby
Owner

Nice tests.

Please sign in to comment.
Something went wrong with that request. Please try again.