launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/bug-813627-preview-diff-overlay into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #813627 in Launchpad itself: "Preview diff overlay blocks bug page"
  https://bugs.launchpad.net/launchpad/+bug/813627

For more details, see:
https://code.launchpad.net/~henninge/launchpad/bug-813627-preview-diff-overlay/+merge/68820

= Summary =

When an overlay for preview diff is closed, it fails to remove the
blocking diff that is used to handle clicking outside of the
overlay. The reason for this is that the widget overides the
"hide" and "show" method of YUI.Widget and thus fails to toggle the
"visible" attribute of the widget. Toggeling that attribute triggers
a custom events to which the blocking diff removal and restoration
is wired.

== Proposed fix ==

The methods must also call the superclass methods they are overriding
to get a behaviour consistent with the Widget infrastructure.

== Pre-implementation notes ==

Deryck mentioned that really using a blocking diff is an old way of
achieving this but I choose not to go into that.

== Tests ==

I extended the open-and-close test to also check for the "visible"
attribute of the widget. For that I needed to expose the widget in
the module

lib/lp/code/javascript/tests/test_branchdiff.html


== Demo and Q/A ==

Click on any line like thise "Diff: 5 lines (+1/-0) 1 file modified".
You will see the pop up. Clicking outside of the overlay, clicking on
the X or pressing the Escape key should close the overlay. Outside
clicking on links etc. should be blocked while the overlay is open.
After the overlay was closed, all clicking on the page should be
possible again, until it is opened again.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/javascript/tests/test_branchdiff.js
  lib/lp/code/javascript/branchmergeproposal.diff.js

./lib/lp/code/javascript/branchmergeproposal.diff.js
      23: 'DiffOverlay' has not been fully defined yet.
-- 
https://code.launchpad.net/~henninge/launchpad/bug-813627-preview-diff-overlay/+merge/68820
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/bug-813627-preview-diff-overlay into lp:launchpad.

=== modified file 'lib/lp/code/javascript/branchmergeproposal.diff.js'
--- lib/lp/code/javascript/branchmergeproposal.diff.js	2011-05-05 20:22:13 +0000
+++ lib/lp/code/javascript/branchmergeproposal.diff.js	2011-07-22 11:22:48 +0000
@@ -35,10 +35,12 @@
          * doesn't provide CSS to handle .visible objects.
          */
         hide: function() {
+            this.constructor.superclass.hide.call(this);
             this.get('boundingBox').setStyle('display', 'none');
         },
 
         show: function() {
+            this.constructor.superclass.show.call(this);
             this.get('boundingBox').setStyle('display', 'block');
         }
     });
@@ -103,6 +105,10 @@
     lp_client.get(api_url, config);
 }
 
+/*
+ * Export rendered widgets.
+ */
+namespace.rendered_overlays = rendered_overlays;
 
 /*
  * Link up the onclick handler for the a.diff-link in the node to the function

=== modified file 'lib/lp/code/javascript/tests/test_branchdiff.js'
--- lib/lp/code/javascript/tests/test_branchdiff.js	2011-06-07 16:42:11 +0000
+++ lib/lp/code/javascript/tests/test_branchdiff.js	2011-07-22 11:22:48 +0000
@@ -29,24 +29,30 @@
         name: 'Test branch diff functions',
 
         /*
-         * Diff overlays should reopen with multiple clicks.
+         * Diff overlays should reopen with multiple clicks. The widget's
+         * visible attribute must be toggled, too.
          */
         test_diff_overlay_multiple_opens: function() {
             // Setup mock client and initialize the link click handler.
             var mock_client = new MockClient();
             var link_node = Y.one('#test-diff-popup');
+            var api_url = link_node.one('a.api-ref').getAttribute('href');
             module.link_popup_diff_onclick(link_node, mock_client);
             // Open the overlay once.
             link_node.one('a.diff-link').simulate('click');
-            var overlay = Y.one('.yui3-pretty-overlay');
+            var widget = module.rendered_overlays[api_url];
+            var overlay = widget.get('boundingBox');
             Y.Assert.isNotNull(overlay);
             Y.Assert.areEqual(overlay.getStyle('display'), 'block');
+            Y.Assert.isTrue(widget.get('visible'));
             // Close the overlay.
             overlay.one('.close a').simulate('click');
             Y.Assert.areEqual(overlay.getStyle('display'), 'none');
+            Y.Assert.isFalse(widget.get('visible'));
             // Open it again.
             link_node.one('a.diff-link').simulate('click');
             Y.Assert.areEqual(overlay.getStyle('display'), 'block');
+            Y.Assert.isTrue(widget.get('visible'));
         }
 
         }));