launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/autocompete-decrufted into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #376864 in Launchpad itself: "No auto-complete when editing tags in Opera or IE"
  https://bugs.launchpad.net/launchpad/+bug/376864

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/autocompete-decrufted/+merge/106479

Pre-implementation: no one

The bug tag autocomplete is disabled for all msie and opera versions without
regard to browser versions and YUI versions. Lp could test if the feature
is broken rather than assuming that it is broken.

--------------------------------------------------------------------

RULES

    * Replace the the cursor position hack for msie. We care about the
      supported features, not the browser.
    * Remove the hack that disables autocomplete for opera and msie.
      * Test both browsers and if one fails, add a version test so that
        the next release will be enabled for users.
    ADDENDUM
    * Opera does work! The guard could have been removed years ago.


QA

    Using Chromium, firefox, Opera, and MSIE
    * Visit https://bugs.qastaging.launchpad.net/pocket-lint/+bug/605911
    * Verify autocomplete suggests tag and that you can complete one.


LINT

    lib/lp/app/javascript/autocomplete/autocomplete.js
    ^ Fixed more lines of lint than lines of code.

TEST

    ./bin/test -vvc --layer=YUITest lp.app


IMPLEMENTATION

Removed the guards that prevent Opera and MSIE from completing.
Revised the getCaretPos() to favour selectionEnd, but fallback to
document.selection. The MSIE implementation did not work, so I rewrote
it. Both Opera and IE are fast.
   lib/lp/app/javascript/autocomplete/autocomplete.js
-- 
https://code.launchpad.net/~sinzui/launchpad/autocompete-decrufted/+merge/106479
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/autocompete-decrufted into lp:launchpad.

=== modified file 'lib/lp/app/javascript/autocomplete/autocomplete.js'
--- lib/lp/app/javascript/autocomplete/autocomplete.js	2012-04-26 19:11:37 +0000
+++ lib/lp/app/javascript/autocomplete/autocomplete.js	2012-05-18 23:04:20 +0000
@@ -36,7 +36,8 @@
 
 // We need a base class on which to build our autocomplete widget, so we will
 // make that class capable of positioning itself, too.
-var AutoCompleteBase = Y.Base.build("AutoCompleteBase", Y.Widget, [Y.WidgetStack]);
+var AutoCompleteBase = Y.Base.build(
+    "AutoCompleteBase", Y.Widget, [Y.WidgetStack]);
 
 
 /**
@@ -53,7 +54,8 @@
 
 AutoComplete.LIST_TEMPLATE = '<ul></ul>';
 AutoComplete.ITEM_TEMPLATE = '<li class="item yui3-menuitem"></li>';
-AutoComplete.ITEM_CONTENT_TEMPLATE = '<a href="#" class="yui3-menuitem-content"></a>';
+AutoComplete.ITEM_CONTENT_TEMPLATE = (
+    '<a href="#" class="yui3-menuitem-content"></a>');
 
 AutoComplete.ATTRS = {
     /**
@@ -152,16 +154,6 @@
     initializer: function() {
         // The widget starts out hidden.
         this.hide();
-
-        // XXX mars 2009-03-30 bug=352022
-        // Disable the widget in Opera, as the NodeMenuNav plugin we use for
-        // the autocomplete is so slow as to be broken.
-        // As for IE, well, it's the usual story.
-        if (Y.UA.opera || Y.UA.ie) {
-            this.disable();
-            // Also disable rendering using a NOP function.
-            this.render = function() {};
-        }
     },
 
     /**
@@ -230,7 +222,8 @@
         var result;
         var item;
         var match;
-        for (var idx = 0; idx < matches.length; idx++) {
+        var idx;
+        for (idx = 0; idx < matches.length; idx++) {
             match  = matches[idx];
             result = this.formatResult(match.text, query, match.offset);
             item   = this._renderCompletion(result, idx);
@@ -350,12 +343,12 @@
         var start = input.lastIndexOf(delimiter, caret_pos - 1);
         var end = input.indexOf(delimiter, caret_pos - 1);
 
-        if ((start == end) && (start != -1)) {
+        if ((start === end) && (start !== -1)) {
             // The caret was on the delimiter itself.
             return null;
         }
 
-        if (start == -1) {
+        if (start === -1) {
             // There wasn't a delimiter between the caret and the start of the
             // string.
             start = 0;
@@ -364,19 +357,19 @@
             start++;
         }
 
-        if (end == -1) {
+        if (end === -1) {
             // There wasn't a delimiter between the caret and the end of the
             // string.
             end = input.length;
         }
 
         // Strip any leading whitespace.
-        while ((input[start] == ' ' || input[start] == '\t') && (start <= end))
-        {
+        while ((input[start] === ' ' || input[start] === '\t')
+                && (start <= end)) {
             start++;
         }
 
-        if (start == end) {
+        if (start === end) {
             // The whitespace stripping took us to the end of the input.
             return null;
         }
@@ -455,15 +448,13 @@
         // of the user query have a higher priority, and come first in the
         // list of matches.  Matches farther toward the end coming later.
         var matches = [];
-        var match_set;
-        for (var index = 0; index < start_indicies.length; index++) {
-            match_set = start_indicies[index];
+        Y.Array.each(start_indicies, function(match_set, index) {
             if (match_set) {
                 Y.Array.each(match_set, function(match) {
                     matches.push({text: match, offset: index});
                 });
             }
-        }
+        });
 
         return matches;
     },
@@ -566,7 +557,7 @@
         var input_tail = input_txt.substring(query_end, input_txt.length);
         var tail_delimiter = delimiter;
         // Add the delimiter only if it's needed.
-        if (input_tail.charAt(input_tail.length - 1) == delimiter) {
+        if (input_tail.charAt(input_tail.length - 1) === delimiter) {
             tail_delimiter = '';
         }
 
@@ -617,7 +608,8 @@
             return null;
         }
 
-        for (var idx = 0; idx < index; idx++) {
+        var idx;
+        for (idx = 0; idx < index; idx++) {
             item = item.next();
             if (!item) {
                 return null;
@@ -716,10 +708,12 @@
     _onInputKeydown: function(e) {
         // Is this one of our completion keys; Tab, or Enter?
         if (e.keyCode === TAB || e.keyCode === RETURN) {
-            /* Check that the last string was not completed and that there are  
-               matching queries (we don't want to try and complete the input if 
+            /* Check that the last string was not completed and that there are
+               matching queries (we don't want to try and complete the input if
                there are no matches). */
-            if (this.get(QUERY) !== null && !this._last_input_was_completed && this.findMatches(this.get(QUERY).text).length !== 0) {
+            if (this.get(QUERY) !== null
+                && !this._last_input_was_completed
+                && this.findMatches(this.get(QUERY).text).length !== 0) {
                 // The user has an active query in the input box.
                 this.completeInput();
                 // Keep the tab key from switching focus away from the input
@@ -772,14 +766,15 @@
  */
 Y.lazr.NodeCaretPos.prototype.getCaretPos = function() {
     var elem = Y.Node.getDOMNode(this);
-    if (Y.UA.ie) {
-        if (document.selection) {
-            var range = document.selection.createRange();
-            range.moveToElementText(elem);
-            return range.text.length;
-        }
-    } else if (typeof elem.selectionEnd != "undefined") {
+    if (elem.selectionEnd) {
         return elem.selectionEnd;
+    } else if (document.selection) {
+        var range = document.selection.createRange();
+        if (range.parentElement() === elem) {
+            var end_range = range.duplicate();
+            end_range.moveStart("character", -elem.value.length);
+            return end_range.text.length;
+        }
     }
     return null;
 };