Launchpad itself

Merge lp:~adiroiban/launchpad/bug-570899 into lp:launchpad

  1. bug-570899
  2. Merge into devel
Proposed by Adi Roiban
Status: Merged
Approved by: Eleanor Berger
Approved revision: no longer in the source branch.
Merged at revision: 10912
Proposed branch: lp:~adiroiban/launchpad/bug-570899
Merge into: lp:launchpad
Diff against target: 327 lines (+114/-82)
1 file modified
lib/canonical/launchpad/javascript/translations/pofile.js (+114/-82)
To merge this branch: bzr merge lp:~adiroiban/launchpad/bug-570899
Reviewer Review Type Date Requested Status
Eleanor Berger (community) Approve
Review via email: mp+25443@code.launchpad.net

Commit message

Don't autoselect new translations when pressing the navigation keys.

Description of the change

= Bug 570899 =

When I click on suggested translation, then click to text field to move cursor as I read original string (or whatever reason), horizontal movement (left and right cursor keys) in text field will mark suggested translation as New translation. Even though I did not made any changes.
There is a risk that I overlook it, hit Save & Continue and translation I intended to only approve is hereby translated and reviewed by me. This isn't fair to original translator regarding Karma and morally.

== Proposed fix ==

Add an exception so that key navigation will not autoselect the translations.

== Pre-implementation notes ==

No pre-implementation chat yet.

== Implementation details ==

After a first feedback from intellectronica, I have added key code constants and refactored the existent/previous code to avoid jslint warnings.

== Tests ==

Unfortunately I was not able to simulate/trigger LEFT/RIGHT/UP/DOWN and other key events.
I have also reported this problem and talked with the Windmill developer but we could not find a way for triggering such events in Firefox.

== Demo and Q/A ==
Login as admin using Firefox.
Go to https://translations.launchpad.dev/evolution/trunk/+pots/evolution-2.2/es/5/+translate

Click on "bang bang in evo hoary" suggestion.

Now click on the "New translation:" text input and press the LEFT and RIGHT, UP and DOWN keys.
The new translation should not be selected and the "bang bang in evo hoary" suggestion should be still selected.

The same result should be observed with SHIFT+ALT+J and SHIFT+ALT+K

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/javascript/translations/pofile.js

== JSLint notices ==
jslint: No problem found in '/home/adi/launchpad/lp-branches/bug-570899/lib/canonical/launchpad/javascript/translations/pofile.js'.

jslint: 1 file to lint.

To post a comment you must log in.
Revision history for this message
Eleanor Berger (intellectronica) wrote :

Thanks for perfecting this branch. Very nice work.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
2--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-23 16:09:20 +0000
3+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-05-18 14:14:35 +0000
4@@ -9,6 +9,26 @@
5 Y.log('loading lp.pofile');
6 var self = Y.namespace('lp.pofile');
7
8+var KEY_CODE_TAB = 9;
9+var KEY_CODE_ENTER = 13;
10+var KEY_CODE_LEFT = 37;
11+var KEY_CODE_UP = 38;
12+var KEY_CODE_RIGHT = 39;
13+var KEY_CODE_DOWN = 40;
14+var KEY_CODE_0 = 48;
15+var KEY_CODE_A = 65;
16+var KEY_CODE_B = 66;
17+var KEY_CODE_C = 67;
18+var KEY_CODE_D = 68;
19+var KEY_CODE_F = 70;
20+var KEY_CODE_J = 74;
21+var KEY_CODE_K = 75;
22+var KEY_CODE_L = 76;
23+var KEY_CODE_N = 78;
24+var KEY_CODE_P = 80;
25+var KEY_CODE_R = 82;
26+var KEY_CODE_S = 83;
27+
28 /**
29 * Function to disable/enable all suggestions as they are marked/unmarked
30 * for dismission.
31@@ -187,11 +207,12 @@
32 var selectTranslation = function(e, field) {
33 // Don't select when tabbing, navigating and simply pressing
34 // enter to submit the form.
35+ // Also, don't select when using keyboard shortcuts (ie Alt+Shift+KEY)
36 // Looks like this is not needed for Epiphany and Chromium
37- if (e.keyCode == 9 || e.keyCode == 13 ||
38- e.keyCode == 38 || e.keyCode == 40 ||
39- (e.shiftKey && e.altKey && e.keyCode == 74) ||
40- (e.shiftKey && e.altKey && e.keyCode == 75)) {
41+ if (e.keyCode == KEY_CODE_TAB || e.keyCode == KEY_CODE_ENTER ||
42+ e.keyCode == KEY_CODE_LEFT || e.keyCode == KEY_CODE_UP ||
43+ e.keyCode == KEY_CODE_RIGHT || e.keyCode == KEY_CODE_DOWN ||
44+ (e.shiftKey && e.altKey)) {
45 return;
46 }
47
48@@ -207,41 +228,41 @@
49
50 Y.get('document').on("keyup", function(e) {
51 var link;
52- // Shift+Alt+s - Save form
53- if (e.shiftKey && e.altKey && e.keyCode == 83) {
54+ // Shift+Alt+S - Save form
55+ if (e.shiftKey && e.altKey && e.keyCode == KEY_CODE_S) {
56 Y.one('#save_and_continue_button').invoke('click');
57 }
58- // Shift+Alt+f - Go to search field
59- if (e.shiftKey && e.altKey && e.keyCode == 70) {
60+ // Shift+Alt+F - Go to search field
61+ if (e.shiftKey && e.altKey && e.keyCode == KEY_CODE_F) {
62 setFocus('search_box');
63 }
64- // Shift+Alt+b - Go to first translation field
65- if (e.shiftKey && e.altKey && e.keyCode == 66) {
66+ // Shift+Alt+B - Go to first translation field
67+ if (e.shiftKey && e.altKey && e.keyCode == KEY_CODE_B) {
68 setFocus(fields[0]);
69 }
70- // Shift+Alt+n - Go to next page in batch
71- if (e.shiftKey && e.altKey && e.keyCode == 78) {
72+ // Shift+Alt+N - Go to next page in batch
73+ if (e.shiftKey && e.altKey && e.keyCode == KEY_CODE_N) {
74 link = Y.one('#batchnav_next');
75 if (link !== null){
76 window.location.assign(link.get('href'));
77 }
78 }
79- // Shift+Alt+p - Go to previous page in batch
80- if (e.shiftKey && e.altKey && e.keyCode == 80) {
81+ // Shift+Alt+P - Go to previous page in batch
82+ if (e.shiftKey && e.altKey && e.keyCode == KEY_CODE_P) {
83 link = Y.one('#batchnav_previous');
84 if (link !== null){
85 window.location.assign(link.get('href'));
86 }
87 }
88- // Shift+Alt+a - Go to first page in batch
89- if (e.shiftKey && e.altKey && e.keyCode == 65) {
90+ // Shift+Alt+A - Go to first page in batch
91+ if (e.shiftKey && e.altKey && e.keyCode == KEY_CODE_A) {
92 link = Y.one('#batchnav_first');
93 if (link !== null){
94 window.location.assign(link.get('href'));
95 }
96 }
97- // Shift+Alt+l - Go to last page in batch
98- if (e.shiftKey && e.altKey && e.keyCode == 76) {
99+ // Shift+Alt+L - Go to last page in batch
100+ if (e.shiftKey && e.altKey && e.keyCode == KEY_CODE_L) {
101 link = Y.one('#batchnav_last');
102 if (link !== null){
103 window.location.assign(link.get('href'));
104@@ -268,6 +289,14 @@
105 });
106 };
107
108+/*
109+ * Adapter for calling functions from Y.on().
110+ * It is used for ignoring the `event` parameter that is passed to all
111+ * functions called by Y.on().
112+ */
113+var on_event_adapter = function(event, method, argument) {
114+ method(argument);
115+};
116
117 var initializeFieldsKeyBindings = function (fields) {
118 for (var key = 0; key < fields.length; key++) {
119@@ -296,38 +325,39 @@
120 // Set next field and copy text for all but last field
121 // (last is Save & Continue button)
122 if (key < fields.length - 1) {
123- // Shift+Alt+j - Go to next translation
124+ // Shift+Alt+J - Go to next translation
125 Y.on(
126 'key', setNextFocus, '#' + fields[key],
127- 'down:74+shift+alt', Y, fields[next]);
128+ 'down:' + KEY_CODE_J + '+shift+alt', Y, fields[next]);
129 // Shift+Alt+KEY_DOWN - Go to next translation
130 Y.on(
131 'key', setNextFocus, '#' + fields[key],
132- 'down:40+shift+alt', Y, fields[next]);
133- // Shift+Alt+c - Copy original text
134+ 'down:' + KEY_CODE_DOWN + '+shift+alt', Y, fields[next]);
135+ // Shift+Alt+C - Copy original text
136 Y.on(
137 'key', copyOriginalTextAll, '#' + fields[key],
138- 'down:67+shift+alt', Y, msgset_id, translation_stem);
139-
140- // Shift+Alt+r - Toggle someone should review
141- Y.on(
142- 'key',
143- function(e, stem) {
144- toggleWidget(stem + '_force_suggestion');
145- },
146- '#' + fields[key], 'down:82+shift+alt', Y, msgset_id);
147-
148- // Shift+Alt+d - Toggle dismiss all translations
149- Y.on(
150- 'key', function(e, stem) {
151- toggleWidget(stem + '_dismiss');
152- }, '#' + fields[key], 'down:68+shift+alt', Y, msgset_id);
153+ 'down:' + KEY_CODE_C + '+shift+alt',
154+ Y, msgset_id, translation_stem);
155+
156+ // Shift+Alt+R - Toggle someone should review
157+ Y.on(
158+ 'key', on_event_adapter,
159+ '#' + fields[key], 'down:' + KEY_CODE_R + '+shift+alt', Y,
160+ toggleWidget, msgset_id + '_force_suggestion');
161+
162+ // Shift+Alt+D - Toggle dismiss all translations
163+ Y.on(
164+ 'key', on_event_adapter,
165+ '#' + fields[key], 'down:' + KEY_CODE_D + '+shift+alt', Y,
166+ toggleWidget, msgset_id + '_dismiss');
167
168 // Shift+Alt+0 - Mark current translation
169 Y.on(
170- 'key', function(e, key) {
171- selectWidgetByID(key.replace(/_new/, "_radiobutton"));
172- }, '#' + fields[key], 'down:48+shift+alt', Y, fields[key]);
173+ 'key', on_event_adapter,
174+ '#' + fields[key], 'down:' + KEY_CODE_0 + '+shift+alt', Y,
175+ selectWidgetByID,
176+ fields[key].replace(/_new/, "_radiobutton"));
177+
178
179 initializeSuggestionsKeyBindings(fields[key]);
180 }
181@@ -337,15 +367,15 @@
182 var parts = fields[previous].split('_');
183 var singular_copy_text = (
184 parts[0] + '_' + parts[1] + '_singular_copy_text');
185- // Shift+Alt+k - Go to previous translation
186+ // Shift+Alt+K - Go to previous translation
187 Y.on(
188 'key', setPreviousFocus, '#' + fields[key],
189- 'down:75+shift+alt', Y, fields[previous],
190+ 'down:' + KEY_CODE_K + '+shift+alt', Y, fields[previous],
191 singular_copy_text);
192 // Shift+Alt+KEY_UP - Go to previous translation
193 Y.on(
194 'key', setPreviousFocus, '#' + fields[key],
195- 'down:38+shift+alt', Y, fields[previous],
196+ 'down:' + KEY_CODE_UP + '+shift+alt', Y, fields[previous],
197 singular_copy_text);
198 }
199 }
200@@ -426,6 +456,34 @@
201 initializeFieldsKeyBindings(fields);
202 };
203
204+/*
205+ * Controls the behavior for reseting current translations
206+ */
207+var resetTranslation = function (event, translation_id) {
208+ if (this === null) {
209+ // Don't do nothing if we don't have a context object.
210+ return;
211+ }
212+ if (this.get('checked') === true) {
213+ var new_translation_select = Y.one(
214+ '#' + translation_id + '_select');
215+ if (new_translation_select !== null) {
216+ new_translation_select.set('checked', true);
217+ }
218+ } else {
219+ var new_translation_field = Y.one('#' + translation_id);
220+ if (new_translation_field !== null &&
221+ new_translation_field.get('value') === '') {
222+ var current_select_id = translation_id.replace(
223+ /_new$/, '_radiobutton');
224+ var current_select = Y.one('#' + current_select_id);
225+ if (current_select !== null) {
226+ current_select.set('checked', true);
227+ }
228+ }
229+ }
230+};
231+
232
233 /**
234 * Translations can be reset by submitting an empty translations and ticking
235@@ -443,33 +501,7 @@
236 // this field, just continue to the next field.
237 break;
238 }
239- Y.on(
240- 'click',
241- function (e, translation_id) {
242- if (this === null) {
243- // Don't do nothing if we don't have a context object.
244- return;
245- }
246- if (this.get('checked') == true) {
247- var new_translation_select = Y.one(
248- '#' + translation_id + '_select');
249- if (new_translation_select !== null) {
250- new_translation_select.set('checked', true);
251- }
252- } else {
253- var new_translation_field = Y.one('#' + translation_id);
254- if (new_translation_field !== null &&
255- new_translation_field.get('value') == '') {
256- var current_select_id = translation_id.replace(
257- /_new$/, '_radiobutton');
258- var current_select = Y.one('#' + current_select_id);
259- if (current_select !== null) {
260- current_select.set('checked', true);
261- }
262- }
263- }
264- },
265- node , node, fields[key]
266+ Y.on('click', resetTranslation, node , node, fields[key]
267 );
268 }
269 };
270@@ -487,29 +519,29 @@
271 var initializeBaseTranslate = function () {
272 try {
273 setupSuggestionDismissal();
274- } catch (e) {
275- Y.log(e, "error");
276+ } catch (setup_suggestion_dismissal_error) {
277+ Y.log(setup_suggestion_dismissal_error, "error");
278 }
279
280 try {
281 initializeKeyBindings();
282- } catch (e) {
283- Y.log(e, "error");
284+ } catch (initialize_key_bindings_error) {
285+ Y.log(initialize_key_bindings_error, "error");
286 }
287
288 try {
289 var fields = translations_order.split(' ');
290 initializeResetBehavior(fields);
291- } catch (e) {
292- Y.log(e, "error");
293+ } catch (initialize_reset_behavior_error) {
294+ Y.log(initialize_reset_behavior_error, "error");
295 }
296
297 try {
298 setFocus(autofocus_field);
299- } catch (e) {
300- Y.log(e, "error");
301+ } catch (set_focus_error) {
302+ Y.log(set_focus_error, "error");
303 }
304-}
305+};
306
307
308 /*
309@@ -518,8 +550,8 @@
310 self.initializePOFile = function(e) {
311 try {
312 updateNotificationBox();
313- } catch (e) {
314- Y.log(e, "error");
315+ } catch (update_notification_box_error) {
316+ Y.log(update_notification_box_error, "error");
317 }
318 initializeBaseTranslate();
319 };
320@@ -533,7 +565,7 @@
321 try {
322 var fields = translations_order.split(' ');
323 initializeReviewDivergeMutualExclusion(fields);
324- } catch (e) {
325+ } catch (initialize_review_diverge_mutual_exclusion_error) {
326 Y.log(e, "error");
327 }
328