launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/long-text-truncation-on-edit into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #249848 in Launchpad itself: "Watermark/title displays poorly with very long name"
  https://bugs.launchpad.net/launchpad/+bug/249848

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/long-text-truncation-on-edit/+merge/119295

== Implementation ==

There were some issues found during QA with regard to using the 3rd party Javascript module for providing multi-line truncation of long text.

The main issue was that when invoking the inline text editor, the truncated text rather than the original text was used. There was also an issue in the 3rd party code whereby an Opera work around caused issues in Firefox. Also, the code to call into the 3rd party module was attached to the wrong widget in editor.js

== Tests ==

I added a new test suite to the test_inline_edit yui tests to check the ellipsis truncation behaviour.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/ellipsis.js
  lib/lp/app/javascript/formwidgets/resizing_textarea.js
  lib/lp/app/javascript/inlineedit/editor.js
  lib/lp/app/javascript/inlineedit/tests/test_inline_edit.html
  lib/lp/app/javascript/inlineedit/tests/test_inline_edit.js

The newly added code is lint free but there's a fair bit of lint in the 3rd party code which has been left as is. 
-- 
https://code.launchpad.net/~wallyworld/launchpad/long-text-truncation-on-edit/+merge/119295
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/long-text-truncation-on-edit into lp:launchpad.

=== modified file 'lib/lp/app/javascript/ellipsis.js'
--- lib/lp/app/javascript/ellipsis.js	2012-08-10 05:00:39 +0000
+++ lib/lp/app/javascript/ellipsis.js	2012-08-13 06:21:18 +0000
@@ -134,7 +134,10 @@
             // apply the styles
             yEl.setStyles(nativeStyles);
             // this is needed to trigger the overflow in some browser (*cough* Opera *cough*)
-            yEl.setStyle('height', lineHeight + 'px');
+            // XXX - In Firefox, getComputedStyle('height') sometimes returns the the height
+            // for a double line even when a single line is all that is needed, so we resort
+            // to using offsetHeight instead.
+            yEl.setStyle('height', node.offsetHeight + 'px');
             // exit early and clean-up
             clone.remove();
             return;

=== modified file 'lib/lp/app/javascript/formwidgets/resizing_textarea.js'
--- lib/lp/app/javascript/formwidgets/resizing_textarea.js	2012-01-21 00:37:14 +0000
+++ lib/lp/app/javascript/formwidgets/resizing_textarea.js	2012-08-13 06:21:18 +0000
@@ -264,7 +264,8 @@
         var scroll_height = this.clone.get('scrollHeight');
 
         if (this.get('single_line') &&
-            this._single_line_height >= scroll_height) {
+            this._single_line_height >= scroll_height &&
+            scroll_height > 0) {
             // Force height if we're only one line and the single_line attr
             // is set.
             this.t_area.setStyles({
@@ -273,7 +274,7 @@
             });
         } else if (this._prev_scroll_height !== scroll_height) {
             // Only update the height if we've changed.
-            new_height = Math.max(
+            var new_height = Math.max(
                 this.get('min_height'),
                 Math.min(scroll_height, this.get('max_height')));
             this.t_area.setStyle('height', new_height);

=== modified file 'lib/lp/app/javascript/inlineedit/editor.js'
--- lib/lp/app/javascript/inlineedit/editor.js	2012-08-13 00:07:14 +0000
+++ lib/lp/app/javascript/inlineedit/editor.js	2012-08-13 06:21:18 +0000
@@ -35,6 +35,7 @@
     MULTILINE = 'multiline',
     MULTILINE_MIN_SIZE = 60,
     TRUNCATE_LINES = 'truncate_lines',
+    ORIGINAL_ELLIPSIS_TEXT = 'ellipsis-original-text',
 
     TOP_BUTTONS = 'top_buttons',
     BOTTOM_BUTTONS = 'bottom_buttons',
@@ -276,20 +277,6 @@
     },
 
     /**
-     * Determines the maximum number of lines to display before the text is
-     * truncated with an ellipsis. 0 means no truncation.
-     *
-     * @attribute truncate_lines
-     * @default 0
-     */
-    truncate_lines: {
-        value: 0,
-        validator: function(value) {
-            return Y.Lang.isNumber(value) && value >= 0;
-        }
-    },
-
-    /**
      * Determines which sets of buttons should be shown in multi-line
      * mode: "top", "bottom", or "both".
      *
@@ -990,7 +977,13 @@
                 // Remove trailing whitespace.
                 return content.replace(/\s+$/,'');
             } else {
-                return Y.Lang.trim(this.get(TEXT).get('text'));
+                // Long lines may have been truncated with an ellipsis so we
+                // may need to get the original untruncated text.
+                var text = text_node.getData(ORIGINAL_ELLIPSIS_TEXT);
+                if (!Y.Lang.isString(text)) {
+                    text = text_node.get('text');
+                }
+                return Y.Lang.trim(text);
             }
         },
         readOnly: true
@@ -1010,6 +1003,20 @@
                 return this.editor.get(ACCEPT_EMPTY);
             }
         }
+    },
+
+    /**
+     * Determines the maximum number of lines to display before the text is
+     * truncated with an ellipsis. 0 means no truncation.
+     *
+     * @attribute truncate_lines
+     * @default 0
+     */
+    truncate_lines: {
+        value: 0,
+        validator: function(value) {
+            return Y.Lang.isNumber(value) && value >= 0;
+        }
     }
 };
 
@@ -1143,6 +1150,19 @@
     },
 
     /**
+     * If truncate_lines has been set, show an ellipsis for long text which
+     * overflows the allowed number of lines.
+     * @private
+     */
+    _showEllipsis: function() {
+        var truncate_lines = this.get(TRUNCATE_LINES);
+        if (truncate_lines > 0) {
+            var text = this.get(TEXT);
+            text.ellipsis({lines: truncate_lines});
+        }
+    },
+
+    /**
      * Initialize the widget.  If an InlineEditor widget hasn't been
      * supplied, it will construct one first, and configure the editor
      * to appear in the appropriate DOM position.  Renders the editor
@@ -1169,6 +1189,15 @@
         // We might want to cancel the render event, depending on the
         // user's browser.
         this.on('render', this._onRender);
+
+        // Set up the callback to truncate the displayed text if necessary.
+        var that = this;
+        Y.on('domready', function() {
+            that._showEllipsis();
+            });
+        Y.on('windowresize', function () {
+            that._showEllipsis();
+        });
     },
 
     /**
@@ -1308,6 +1337,7 @@
         if (this.get(RENDERED)) {
             var text = this.get(TEXT),
                 val  = this.editor.get(VALUE);
+            text.setData(ORIGINAL_ELLIPSIS_TEXT, val);
             text.set('innerHTML', '');
             if (this.editor.get(MULTILINE)) {
                 text.set('innerHTML', val);
@@ -1348,6 +1378,7 @@
         this.editor.hideLoadingSpinner();
         this.syncUI();
         this.hide_editor();
+        this._showEllipsis();
         this._uiAnimateSave();
         this.editor.set(INITIAL_VALUE_OVERRIDE, null);
     },

=== modified file 'lib/lp/app/javascript/inlineedit/tests/test_inline_edit.html'
--- lib/lp/app/javascript/inlineedit/tests/test_inline_edit.html	2012-03-14 04:41:36 +0000
+++ lib/lp/app/javascript/inlineedit/tests/test_inline_edit.html	2012-08-13 06:21:18 +0000
@@ -28,6 +28,8 @@
       <script type="text/javascript"
           src="../../../../../../build/js/lp/app/anim/anim.js"></script>
       <script type="text/javascript"
+          src="../../../../../../build/js/lp/app/ellipsis.js"></script>
+      <script type="text/javascript"
           src="../../../../../../build/js/lp/app/errors.js"></script>
       <script type="text/javascript"
           src="../../../../../../build/js/lp/app/formoverlay/formoverlay.js"></script>

=== modified file 'lib/lp/app/javascript/inlineedit/tests/test_inline_edit.js'
--- lib/lp/app/javascript/inlineedit/tests/test_inline_edit.js	2012-04-06 17:28:25 +0000
+++ lib/lp/app/javascript/inlineedit/tests/test_inline_edit.js	2012-08-13 06:21:18 +0000
@@ -8,7 +8,7 @@
 
     var SAMPLE_HTML = [
         '<h1>Single-line editing</h1>',
-        ' <div id="editable_single_text">',
+        ' <div id="editable_single_text" style="width: 40em;">',
         ' <span id="single_text"',
         '  class="yui3-editable_text-text">Some editable inline text.</span>',
         ' <button id="single_edit" class="yui3-editable_text-trigger">',
@@ -454,6 +454,107 @@
     }));
 
     tests.suite.add(new Y.Test.Case({
+        name: 'Ellipsis',
+
+        setUp: function() {
+            setup_sample_html();
+            this.long_text = 'Blah';
+            var x;
+            for (x=0; x<100; x++) {
+                this.long_text += ' Blah';
+            }
+            Y.one('#single_text').set('text', this.long_text);
+            this.single = make_editable_text({
+                contentBox: '#editable_single_text',
+                truncate_lines: 2,
+                multiline: false
+            });
+            this.single.render();
+            this.single._showEllipsis();
+        },
+
+        tearDown: function() {
+            cleanup_widget(this.single);
+        },
+
+        // Long text is truncated in the text display.
+        _assert_displayed_value_truncated: function() {
+            var ellipsis = '\u2026';
+            var display_text = Y.one('#single_text').get('text');
+            Y.Assert.isTrue(
+                display_text.indexOf(
+                    ellipsis, display_text.length - ellipsis.length) >= 0,
+                'Long text should be truncated when displayed.');
+        },
+
+        // Long text is truncated when first displayed.
+        test_initial_value_truncated: function() {
+            this._assert_displayed_value_truncated();
+        },
+
+        // When the text is edited, the untruncated text is displayed in the
+        // input field.
+        test_edit_value_not_truncated: function() {
+            simulate('#single_edit', 'click');
+            Y.Assert.areEqual(
+                this.long_text,
+                this.single.editor.get('value'),
+                'Untruncated text should be displayed in the edit ' +
+                'field.');
+        },
+
+        // When the text is edited and a short value is entered,  the
+        // untruncated text is displayed in the text field as well as being
+        // saved.
+        test_short_saved_value_not_truncated: function() {
+            simulate('#single_edit', 'click');
+            this.single.editor
+                .get('input_field')
+                .set('value', 'Short');
+
+            this.single.editor.save();
+
+            Assert.areEqual(
+                'Short',
+                this.single.editor.get('value'),
+                "Sanity check: the editor's value should have been " +
+                "saved.");
+
+            Assert.areEqual(
+                'Short',
+                this.single.get('value'),
+                "The editable text's current value should be updated " +
+                "after saving some new text in the editor.");
+        },
+
+        // When the text is edited and a long value is entered, the truncated
+        // text is displayed in the text field but the long value is saved.
+        test_long_saved_value_truncated: function() {
+            simulate('#single_edit', 'click');
+            var long_saved_text = this.long_text + 'foo';
+            this.single.editor
+                .get('input_field')
+                .set('value', long_saved_text);
+
+            this.single.editor.save();
+
+            Assert.areEqual(
+                long_saved_text,
+                this.single.editor.get('value'),
+                "Sanity check: the editor's value should have been " +
+                "saved.");
+
+            Assert.areEqual(
+                long_saved_text,
+                this.single.get('value'),
+                "The editable text's current value should be updated " +
+                "after saving some new text in the editor.");
+            this._assert_displayed_value_truncated();
+        }
+
+    }));
+
+    tests.suite.add(new Y.Test.Case({
         name: 'Editable text initial values',
 
         setUp: function() {
@@ -1019,6 +1120,6 @@
     }));
 
 }, '0.1', {'requires': ['test', 'console', 'lazr.editor', 'node',
-    'lp.app.formwidgets.resizing_textarea', 'event',
-    'event-simulate', 'plugin']
+    'lp.app.formwidgets.resizing_textarea', 'lp.app.ellipsis',
+    'event', 'event-simulate', 'plugin']
 });