Fix bug 1584579: Exclude placeables from access key candidates if key…

… not translated (#1398)
mozilla · Sep 30, 2019 · a1eeb18 · a1eeb18
1 parent c434db7
commit a1eeb18
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 24 deletions.
diff --git a/frontend/src/core/utils/fluent/extractAccessKeyCandidates.js b/frontend/src/core/utils/fluent/extractAccessKeyCandidates.js
@@ -2,9 +2,6 @@
 
 import flattenDeep from 'lodash.flattendeep';
 
-import parser from './parser';
-import serializer from './serializer';
-
 import type { FluentMessage, PatternElement } from './types';
 
 
@@ -39,19 +36,10 @@ function getTextElementsRecursivelly(elements: Array<PatternElement>) {
  * and then generates a list of unique characters from either the attribute with an ID
  * 'label' or the message value.
  *
- * @param {FluentMessage} translation A Fluent message to extract access key candidates from.
+ * @param {FluentMessage} message A (flat) Fluent message to extract access key candidates from.
  * @returns {?Array<string>} A list of access key candidates.
  */
-export default function extractAccessKeyCandidates(translation: FluentMessage): ?Array<string> {
-    // The input message is flat, so we must re-parse it to be able to remove non-TextElements
-    let message = parser.parseEntry(serializer.serializeEntry(translation));
-
-    // Unless it's Junk, which often happens while typing:
-    // then it's better to still show some candidates then to show nothing
-    if (message.type === 'Junk') {
-        message = translation;
-    }
-
+export default function extractAccessKeyCandidates(message: FluentMessage): ?Array<string> {
     // If message has no attributes, return null
     if (!message.attributes) {
         return null;
@@ -77,14 +65,20 @@ export default function extractAccessKeyCandidates(translation: FluentMessage):
         return null;
     }
 
-    // Only take TextElements, see bug 1447103 for detals (that's why flat Message is no good)
+    // Only take TextElements
     const textElements = getTextElementsRecursivelly(source.value.elements);
 
     // Collect values of TextElements
     const values = textElements.map(element => {
         let value = '';
         if (element && typeof(element.value) === 'string') {
-            value = element.value.replace(/\s/g, '') // Also: Remove whitespace
+            value = (
+                element.value
+                // Exclude placeables (message is flat). See bug 1447103 for details.
+                .replace(/{[^}]*}/g, '')
+                // Exclude whitespace
+                .replace(/\s/g, '')
+            )
         }
         return value;
     });

diff --git a/frontend/src/core/utils/fluent/extractAccessKeyCandidates.test.js b/frontend/src/core/utils/fluent/extractAccessKeyCandidates.test.js
@@ -1,10 +1,11 @@
 import extractAccessKeyCandidates from './extractAccessKeyCandidates';
+import flattenMessage from './flattenMessage';
 import parser from './parser';
 
 
 describe('extractAccessKeyCandidates', () => {
     it('returns null if the message has no attributes', () => {
-        const message = parser.parseEntry('title = Title');
+        const message = flattenMessage(parser.parseEntry('title = Title'));
         const res = extractAccessKeyCandidates(message);
 
         expect(res).toEqual(null);
@@ -13,7 +14,7 @@ describe('extractAccessKeyCandidates', () => {
     it('returns null if the message has no accesskey attribute', () => {
         const input = 'title =' +
             '\n    .foo = Bar';
-        const message = parser.parseEntry(input);
+        const message = flattenMessage(parser.parseEntry(input));
         const res = extractAccessKeyCandidates(message);
 
         expect(res).toEqual(null);
@@ -23,7 +24,7 @@ describe('extractAccessKeyCandidates', () => {
         const input = 'title =' +
             '\n    .foo = Bar' +
             '\n    .accesskey = B';
-        const message = parser.parseEntry(input);
+        const message = flattenMessage(parser.parseEntry(input));
         const res = extractAccessKeyCandidates(message);
 
         expect(res).toEqual(null);
@@ -32,7 +33,7 @@ describe('extractAccessKeyCandidates', () => {
     it('returns a list of access keys from the message value', () => {
         const input = 'title = Candidates' +
             '\n    .accesskey = B';
-        const message = parser.parseEntry(input);
+        const message = flattenMessage(parser.parseEntry(input));
         const res = extractAccessKeyCandidates(message);
 
         expect(res).toEqual(['C', 'a', 'n', 'd', 'i', 't', 'e', 's']);
@@ -42,17 +43,17 @@ describe('extractAccessKeyCandidates', () => {
         const input = 'title = Title' +
             '\n    .label = Candidates' +
             '\n    .accesskey = B';
-        const message = parser.parseEntry(input);
+        const message = flattenMessage(parser.parseEntry(input));
         const res = extractAccessKeyCandidates(message);
 
         expect(res).toEqual(['C', 'a', 'n', 'd', 'i', 't', 'e', 's']);
     });
 
-    it('Only takes TextElements into account when generating candidates', () => {
+    it('Does not take Placeables into account when generating candidates', () => {
         const input = 'title = Title' +
             '\n    .label = Candidates { brand }' +
             '\n    .accesskey = B';
-        const message = parser.parseEntry(input);
+        const message = flattenMessage(parser.parseEntry(input));
         const res = extractAccessKeyCandidates(message);
 
         expect(res).toEqual(['C', 'a', 'n', 'd', 'i', 't', 'e', 's']);
@@ -66,7 +67,7 @@ describe('extractAccessKeyCandidates', () => {
             '\n           *[other] Cmd' +
             '\n        }' +
             '\n    .accesskey = C';
-        const message = parser.parseEntry(input);
+        const message = flattenMessage(parser.parseEntry(input));
         const res = extractAccessKeyCandidates(message);
 
         expect(res).toEqual(['C', 't', 'r', 'l', 'm', 'd']);