launchpad-reviewers team mailing list archive

[Benji York <benji@xxxxxxxxxxxxx>]

Benji York has proposed merging lp:~benji/launchpad/bug-779538 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #779538 in Launchpad itself: "notifications disappear when you click *any* link in the text"
  https://bugs.launchpad.net/launchpad/+bug/779538

For more details, see:
https://code.launchpad.net/~benji/launchpad/bug-779538/+merge/60650

Bug 779538 describes a regression caused by my recent change to add a
"Hide X" link to notification boxes so the user can dismiss them when
they're no longer needed (especially useful on AJAX-heavy pages).
Unfortunately that behavior extended to links within the notification
box.

When a user would click on a link in a notification box the box would
disappear.  This is primarily a problem when control- or shift-clicking
on a link to open it in another browser tab or window.

This branch rectifies that problem by using a different approach to
implement the click-to-close behavior.  We now use setInterval to
periodically add real (as opposed to the previous approach of using CSS)
hide links and attaching click handlers to them.  This preserves all the
expected behavior of message boxes (control-/shift-click on links
doesn't close them, highlighting text works correctly, etc.).

The setInterval period is so high (500 ms), and the function does so
little that it should introduce only negligible CPU load.  Memory leaks
are also a concern with approaches like this: I can't see that any
objects are allocated in the "idle" case, so I believe there are no
leaks.

The make lint report is clean.

-- 
https://code.launchpad.net/~benji/launchpad/bug-779538/+merge/60650
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-779538 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css	2011-05-04 14:33:48 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css	2011-05-11 15:48:43 +0000
@@ -1017,23 +1017,13 @@
 .warning.message::before {
     content: url(/@@/warning-large);
     }
-.error.message::after, .warning.message::after,
-.informational.message::after {
-    /* Add an affordance that suggests the ability to hide the message. */
-    /* The NBSP (\00A0) is needed because of the margin trick below. */
-    content: 'Hide\00A0\2715';
+.error.message .hider, .warning.message .hider,
+.informational.message .hider {
+    color: #333;
     float: right;
     display: block;
     margin-left: 999em; /* Assure the message is always "bumped" down. */
     }
-.error.message:hover::after, .warning.message:hover::after,
-.informational.message:hover::after {
-    text-decoration: underline;
-    }
-.error.message:hover, .warning.message:hover,
-.informational.message:hover {
-    cursor: pointer;
-}
 .informational {
     /* Informational messages are blue-to-grey, alerts have an info icon. */
 

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt	2011-05-03 17:53:50 +0000
+++ lib/lp/app/templates/base-layout-macros.pt	2011-05-11 15:48:43 +0000
@@ -125,11 +125,29 @@
         // anywhere outside of it.
         Y.on('click', handleClickOnPage, window);
 
-        // Hook up an event hanlder that will close any message boxes when they
-        // are clicked.
-        Y.delegate('click', function(e) {
-            this.setStyle('display', 'none');
-        }, 'body', '.message');
+        // Add a "Hide" link to a message box.
+        var add_hide_to_message = function(message) {
+            if (Y.Lang.isValue(message.one('.hider'))) {
+                // We've already done this one.
+                return;
+            }
+            var hider = message.appendChild(Y.Node.create('<a/>')
+                .set('href', '#')
+                .set('text', 'Hide\u00A0\u2715')
+                .addClass('hider'));
+
+            Y.on('click', function(e) {
+                message.setStyle('display', 'none');
+            }, hider);
+        }
+
+        // Periodically check to see if there are any new message boxes on the
+        // page that need a hide link.
+        setInterval(function() {
+            Y.all('.warning.message').each(add_hide_to_message);
+            Y.all('.informational.message').each(add_hide_to_message);
+            Y.all('.error.message').each(add_hide_to_message);
+        }, 500);
 
         Y.on('lp:context:web_link:changed', function(e) {
             window.location = e.new_value;