LAZR Javascript Library

Merge lp:~jtv/lazr-js/bug-480986 into lp:lazr-js

  1. bug-480986
  2. Merge into toolchain
Proposed by Jeroen T. Vermeulen
Status: Merged
Approved by: Barry Warsaw
Approved revision: 135
Merged at revision: not available
Proposed branch: lp:~jtv/lazr-js/bug-480986
Merge into: lp:lazr-js
Diff against target: 110 lines (+17/-21)
2 files modified
src-js/lazrjs/overlay/overlay.js (+3/-4)
src-js/lazrjs/overlay/tests/overlay.js (+14/-17)
To merge this branch: bzr merge lp:~jtv/lazr-js/bug-480986
Reviewer Review Type Date Requested Status
Barry Warsaw (community) code Approve
Review via email: mp+14801@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

= Bug 480986 =

The Picker's escape handling wasn't working in Chrome. The reason is this Chrome issue: http://www.google.ru/support/forum/p/Chrome/thread?tid=2c74b15190723cf6&hl=en

In a nutshell, Chrome doesn't do keypress events for the escape key. This was not affecting the autocomplete widget or the inline editor since they listen for "key" events (and obey the escape right when you press it rather than when you release it). But it did affect the other overlay-based widgets since it was in the overlay that the escape key was handled. To reproduce, open any of these widgets (choiceedit, formoverlay, overlay, or picker) in Chrome or Chromium and press escape. It should close the widget, but with the bug still present it will do nothing.

In this branch, the overlay's escape handling is replaced with the inline editor's approach. I discussed with the author of the original escape handling in the overlay and we found that this was the best approach after all.

You'll also find a few drive-by cleanups prompted by "make lint."

Jeroen

Revision history for this message
Barry Warsaw (barry) wrote :

As discussed:

* reformat line 13-14
* add comment for down:27

review: Approve (code)
lp:~jtv/lazr-js/bug-480986 updated
136. By Jeroen T. Vermeulen

Review change.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src-js/lazrjs/overlay/overlay.js'
--- src-js/lazrjs/overlay/overlay.js 2009-10-23 18:24:21 +0000
+++ src-js/lazrjs/overlay/overlay.js 2009-11-12 20:53:09 +0000
@@ -209,11 +209,10 @@
209 var visible = this.get('visible');209 var visible = this.get('visible');
210 if (visible) {210 if (visible) {
211 Y.get('body').appendChild(this._blocking_div);211 Y.get('body').appendChild(this._blocking_div);
212 this._doc_kp_handler = Y.on('keypress', function(e) {212 // Handle Escape (code 27) on keydown.
213 if (e.keyCode == ESCAPE) {213 this._doc_kp_handler = Y.on('key', function() {
214 self.fire(CANCEL);214 self.fire(CANCEL);
215 }215 }, document, 'down:27');
216 }, document);
217 } else {216 } else {
218 this._removeBlockingDiv();217 this._removeBlockingDiv();
219 }218 }
220219
=== modified file 'src-js/lazrjs/overlay/tests/overlay.js'
--- src-js/lazrjs/overlay/tests/overlay.js 2009-10-22 18:37:47 +0000
+++ src-js/lazrjs/overlay/tests/overlay.js 2009-11-12 20:53:09 +0000
@@ -51,6 +51,12 @@
51 cleanup_widget(this.overlay);51 cleanup_widget(this.overlay);
52 },52 },
5353
54 hitEscape: function() {
55 simulate(this.overlay.get('boundingBox'),
56 '.close .close-button',
57 'keydown', { keyCode: ESCAPE });
58 },
59
54 test_picker_can_be_instantiated: function() {60 test_picker_can_be_instantiated: function() {
55 this.overlay = new Y.lazr.PrettyOverlay();61 this.overlay = new Y.lazr.PrettyOverlay();
56 Assert.isInstanceOf(62 Assert.isInstanceOf(
@@ -146,9 +152,7 @@
146 this.overlay = new Y.lazr.PrettyOverlay();152 this.overlay = new Y.lazr.PrettyOverlay();
147 this.overlay.render();153 this.overlay.render();
148154
149 simulate(this.overlay.get('boundingBox'),155 this.hitEscape();
150 '.close .close-button',
151 'keypress', { keyCode: ESCAPE });
152 Assert.isFalse(this.overlay.get('visible'), "The widget wasn't hidden");156 Assert.isFalse(this.overlay.get('visible'), "The widget wasn't hidden");
153 },157 },
154158
@@ -162,9 +166,7 @@
162 event_was_fired = true;166 event_was_fired = true;
163 });167 });
164 }, this);168 }, this);
165 simulate(this.overlay.get('boundingBox'),169 this.hitEscape();
166 '.close .close-button',
167 'click', { keyCode: ESCAPE });
168 this.wait(function() {170 this.wait(function() {
169 Assert.isTrue(event_was_fired, "cancel event wasn't fired");171 Assert.isTrue(event_was_fired, "cancel event wasn't fired");
170 }, 3000);172 }, 3000);
@@ -177,15 +179,11 @@
177 this.overlay = new Y.lazr.PrettyOverlay();179 this.overlay = new Y.lazr.PrettyOverlay();
178 this.overlay.render();180 this.overlay.render();
179181
180 simulate(this.overlay.get('boundingBox'),182 this.hitEscape();
181 '.close .close-button',
182 'keypress', { keyCode: ESCAPE });
183 Assert.isFalse(this.overlay.get('visible'), "The widget wasn't hidden");183 Assert.isFalse(this.overlay.get('visible'), "The widget wasn't hidden");
184 this.overlay.show();184 this.overlay.show();
185 Assert.isTrue(this.overlay.get('visible'), "The widget wasn't shown again");185 Assert.isTrue(this.overlay.get('visible'), "The widget wasn't shown again");
186 simulate(this.overlay.get('boundingBox'),186 this.hitEscape();
187 '.close .close-button',
188 'keypress', { keyCode: ESCAPE });
189 Assert.isFalse(this.overlay.get('visible'), "The widget wasn't hidden");187 Assert.isFalse(this.overlay.get('visible'), "The widget wasn't hidden");
190 },188 },
191189
@@ -193,7 +191,7 @@
193 this.overlay = new Y.lazr.PrettyOverlay();191 this.overlay = new Y.lazr.PrettyOverlay();
194 function PrettyOverlaySubclass(config) {192 function PrettyOverlaySubclass(config) {
195 PrettyOverlaySubclass.superclass.constructor.apply(this, arguments);193 PrettyOverlaySubclass.superclass.constructor.apply(this, arguments);
196 };194 }
197 PrettyOverlaySubclass.NAME = 'lazr-overlaysubclass';195 PrettyOverlaySubclass.NAME = 'lazr-overlaysubclass';
198 Y.extend(PrettyOverlaySubclass, Y.lazr.PrettyOverlay);196 Y.extend(PrettyOverlaySubclass, Y.lazr.PrettyOverlay);
199197
@@ -206,14 +204,13 @@
206 test_overlay_bodyContent_has_size_1: function() {204 test_overlay_bodyContent_has_size_1: function() {
207 var overlay = new Y.Overlay({205 var overlay = new Y.Overlay({
208 headerContent: 'Form for testing',206 headerContent: 'Form for testing',
209 bodyContent: '<input type="text" name="field1" />',207 bodyContent: '<input type="text" name="field1" />'
210 });208 });
211 overlay.render();209 overlay.render();
212 Assert.areEqual(210 Assert.areEqual(
213 1,211 1,
214 overlay.get("bodyContent").size(),212 overlay.get("bodyContent").size(),
215 "The bodContent should contain only one node."213 "The bodContent should contain only one node.");
216 )
217 },214 },
218215
219 test_set_progress: function() {216 test_set_progress: function() {
@@ -226,7 +223,7 @@
226 Assert.areEqual(223 Assert.areEqual(
227 '23%',224 '23%',
228 this.overlay.get('boundingBox').query('.steps .step-on').getStyle('width')225 this.overlay.get('boundingBox').query('.steps .step-on').getStyle('width')
229 )226 );
230 }227 }
231228
232}));229}));

Subscribers

People subscribed via source and target branches