Exclusions; use querySelector to find sub-elements.

As @philc pointed out in #1366, this is less brittle.

3 files changed, 27 insertions, 22 deletions

diff --git a/background_scripts/exclusions.coffee b/background_scripts/exclusions.coffee
index 62647bd8..55ced3ef 100644
--- a/background_scripts/exclusions.coffee
+++ b/background_scripts/exclusions.coffee
@@ -23,15 +23,17 @@ root.Exclusions = Exclusions =
 
   rules: Settings.get("exclusionRules")
 
-  # Merge the matching rules for URL, or null. If rules are provided, match against those.
+  # Merge the matching rules for URL, or null.  In the normal case, we use the configured @rules; hence, this
+  # is the default.  However, when called from the page popup, we are testing what effect candidate new rules
+  # would have on the current tab.  In this case, the candidate rules are provided by the caller.
   getRule: (url, rules=@rules) ->
     matches = (rule for rule in rules when rule.pattern and 0 <= url.search(RegexpCache.get(rule.pattern)))
     # An absolute exclusion rule (with no passKeys) takes priority.
     for rule in matches
       return rule unless rule.passKeys
-    if matches.length
+    if 0 < matches.length
       pattern: (rule.pattern for rule in matches).join " | " # Not used; for debugging only.
-      passKeys: Utils.uniqueCharacters (rule.passKeys for rule in matches).join ""
+      passKeys: Utils.distinctCharacters (rule.passKeys for rule in matches).join ""
     else
       null
 
diff --git a/lib/utils.coffee b/lib/utils.coffee
index 81b0fb49..05ebd071 100644
--- a/lib/utils.coffee
+++ b/lib/utils.coffee
@@ -111,7 +111,7 @@ Utils =
   isString: (obj) -> typeof obj == 'string' or obj instanceof String
 
   # Transform "zjkjkabz" into "abjkz".
-  uniqueCharacters: (str) ->
+  distinctCharacters: (str) ->
     unique = ""
     for char in str.split("").sort()
       unique += char unless 0 <= unique.indexOf char
diff --git a/pages/options.coffee b/pages/options.coffee
index 9aea9fd9..15f4c16c 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -86,11 +86,8 @@ class ExclusionRulesOption extends Option
   # options page, there is no current URL, so there is no initial pattern.  This is the default.  On the popup
   # page (see ExclusionRulesOnPopupOption), the pattern is pre-populated based on the current tab's URL.
   addRule: (pattern="") ->
-      @appendRule { pattern: pattern, passKeys: "" }
-      # Focus the pattern within the new rule.
-      element = @element.children[@element.children.length-1]
-      element.children[0].children[0].focus()
-      # Scroll the new rule into view.
+      element = @appendRule { pattern: pattern, passKeys: "" }
+      @getPattern(element).focus()
       exclusionScrollBox = $("exclusionScrollBox")
       exclusionScrollBox.scrollTop = exclusionScrollBox.scrollHeight
       @onUpdated()
@@ -100,7 +97,7 @@ class ExclusionRulesOption extends Option
     for rule in rules
       @appendRule rule
 
-  # Append a row for a new rule.
+  # Append a row for a new rule.  Return the newly-added element.
   appendRule: (rule) ->
     content = document.querySelector('#exclusionRuleTemplate').content
     row = document.importNode content, true
@@ -111,20 +108,19 @@ class ExclusionRulesOption extends Option
       for event in [ "input", "change" ]
         element.addEventListener event, @onUpdated
 
-    remove = row.querySelector ".exclusionRemoveButton"
-    remove.addEventListener "click", (event) =>
-      row = event.target.parentNode.parentNode
-      row.parentNode.removeChild row
+    @getRemoveButton(row).addEventListener "click", (event) =>
+      rule = event.target.parentNode.parentNode
+      rule.parentNode.removeChild rule
       @onUpdated()
 
     @element.appendChild row
+    @element.children[@element.children.length-1]
 
   readValueFromElement: ->
     rules =
       for element in @element.getElementsByClassName "exclusionRuleTemplateInstance"
-        pattern = element.children[0].firstChild.value.split(/\s+/).join ""
-        passKeys = element.children[1].firstChild.value.split(/\s+/).join ""
-        { pattern: pattern, passKeys: passKeys }
+        pattern: @getPattern(element).value.split(/\s+/).join ""
+        passKeys: @getPassKeys(element).value.split(/\s+/).join ""
     rules.filter (rule) -> rule.pattern
 
   areEqual: (a,b) ->
@@ -133,6 +129,11 @@ class ExclusionRulesOption extends Option
     flatten = (rule) -> if rule and rule.pattern then rule.pattern + "\n" + rule.passKeys else ""
     a.map(flatten).join("\n") == b.map(flatten).join("\n")
 
+  # Accessors for the three main sub-elements of an "exclusionRuleTemplateInstance".
+  getPattern: (element) -> element.querySelector(".pattern")
+  getPassKeys: (element) -> element.querySelector(".passKeys")
+  getRemoveButton: (element) -> element.querySelector(".exclusionRemoveButtonButton")
+
 # ExclusionRulesOnPopupOption is ExclusionRulesOption, extended with some UI tweeks suitable for use in the
 # page popup.  This also differs from ExclusionRulesOption in that, on the page popup, there is always a URL
 # (@url) associated with the current tab.
@@ -144,8 +145,10 @@ class ExclusionRulesOnPopupOption extends ExclusionRulesOption
     element = super @generateDefaultPattern()
     @activatePatternWatcher element
     # ExclusionRulesOption.addRule()/super() has focused the pattern.  Here, focus the passKeys instead;
-    # because, in the popup, we already have a pattern.
-    element.children[1].children[0].focus()
+    # because, in the popup, we already have a pattern, so the user is more likely to edit the passKeys.
+    @getPassKeys(element).focus()
+    # Return element (for consistency with ExclusionRulesOption.addRule()).
+    element
 
   populateElement: (rules) ->
     super(rules)
@@ -157,7 +160,7 @@ class ExclusionRulesOnPopupOption extends ExclusionRulesOption
       pattern = element.children[0].firstChild.value.trim()
       if 0 <= @url.search bgExclusions.RegexpCache.get pattern
         haveMatch = true
-        element.children[1].firstChild.focus()
+        @getPassKeys(element).focus()
       else
         element.style.display = 'none'
     @addRule() unless haveMatch
@@ -172,8 +175,8 @@ class ExclusionRulesOnPopupOption extends ExclusionRulesOption
         patternElement.style.color = "red"
         patternElement.title = "Red text means that the pattern does not\nmatch the current URL."
 
-  # Generate a default exclusion-rule pattern from a URL.  This is used to pre-populate the pattern on the
-  # page popup.
+  # Generate a default exclusion-rule pattern from a URL.  This is then used to pre-populate the pattern on
+  # the page popup.
   generateDefaultPattern: ->
     if /^https?:\/\/./.test @url
       # The common use case is to disable Vimium at the domain level.