Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2362673004:
Aligns text to braille in chromevox. (Closed)
|
Created:
4 years, 3 months ago by ultimatedbz Modified:
4 years, 2 months ago Reviewers:
dmazzoni CC:
aboxhall+watch_chromium.org, alemate+watch_chromium.org, arv+watch_chromium.org, chromium-reviews, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, je_julie, nektar+watch_chromium.org, oshima+watch_chromium.org, yuzo+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAligns text to braille in chromevox.
In the chromevox text to speech display where braille and regular text is shown one on top of the other, the texts are not aligned. This causes friction when looking between the braille and regular text.
BUG=651563
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/80b4abd5c8bcd470929a6486dd7047352cfc769a
Cr-Commit-Position: refs/heads/master@{#424340}
Patch Set 1 #Patch Set 2 : Addressed all presubmit issues. #
Total comments: 20
Patch Set 3 : Addressed code reviews. Extracted grouping algorithm into a helper method. #
Total comments: 6
Patch Set 4 : Addressed code reviews and wrote tests. #
Total comments: 8
Patch Set 5 : Addressed test code reviews #Patch Set 6 : fixed test #Messages
Total messages: 27 (12 generated)
Description was changed from ========== Aligns text to braille in chromevox. Text now aligns thanks to html tables. wrote alignment code. However, it doesn't align because of unicode. No matter, because we will align with html. Finished grouping algorithm. Able to pass array of groupings to Panel. Need to clean up code and display groups. BUG= ========== to ========== Aligns text to braille in chromevox. Text now aligns thanks to html tables. wrote alignment code. However, it doesn't align because of unicode. No matter, because we will align with html. Finished grouping algorithm. Able to pass array of groupings to Panel. Need to clean up code and display groups. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Aligns text to braille in chromevox. Text now aligns thanks to html tables. wrote alignment code. However, it doesn't align because of unicode. No matter, because we will align with html. Finished grouping algorithm. Able to pass array of groupings to Panel. Need to clean up code and display groups. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Aligns text to braille in chromevox. Text now aligns thanks to html tables. wrote alignment code. However, it doesn't align because of unicode. No matter, because we will align with html. Finished grouping algorithm. Able to pass array of groupings to Panel. Need to clean up code and display groups. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
ultimatedbz@google.com changed reviewers: + dmazzoni@chromium.org
Please file a bug for this and add the bug number. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:60: * @param {Array} brailleToText Map of Braille letters to the first How about {Array<number>} - it's always good to be as specific as possible https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:62: * corresponding text letter. nit: wrap this https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:74: for (var i = 0; i < byteBuf.length - 1; ++i) { Abstract this concept of breaking into groups into its own helper function so it's ready for unit testing. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:75: brailleChars += String.fromCharCode( We're not using brailleChars anymore, so you can just get rid of it. Alternatively, do one pass to convert to brailleChars, then a second pass to convert the text and brailleChars to text - that might be better so you won't need the String.fromCharCode line twice. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:78: groups.push([text.substr(textIndex, brailleToText[i] - textIndex), Instead of each group being an array [text, braille], make it an object with named fields, like {text: text, braille: braille}. This makes it harder to accidentally get them mixed up elsewhere in the code. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.css (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.css:301: table { nit: blank line before Also make this: table#braille-table so it only applies to this table, not any table https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.css:306: table * { Same here, and below https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:16: * @param {string|{groups:Array}|{text: string, braille: string}=} opt_data Delete {text: string, braille: string}, that's not used anymore since you replaced it with groups.
https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:75: brailleChars += String.fromCharCode( On 2016/09/26 04:53:49, dmazzoni wrote: > We're not using brailleChars anymore, so you can just > get rid of it. > > Alternatively, do one pass to convert to brailleChars, > then a second pass to convert the text and brailleChars > to text - that might be better so you won't need the > String.fromCharCode line twice. Isn't brailleChars used below in line 100 though? I wasn't exactly sure what the extension bridge was so I left it alone. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:16: * @param {string|{groups:Array}|{text: string, braille: string}=} opt_data On 2016/09/26 04:53:49, dmazzoni wrote: > Delete {text: string, braille: string}, that's not used anymore since you > replaced it with groups. Again, I left this here because I wasn't sure what the extension bridge was. Should I modify the extension bridge code to also take groups instead?
https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:75: brailleChars += String.fromCharCode( On 2016/09/27 21:15:49, ultimatedbz wrote: > On 2016/09/26 04:53:49, dmazzoni wrote: > > We're not using brailleChars anymore, so you can just > > get rid of it. > > > > Alternatively, do one pass to convert to brailleChars, > > then a second pass to convert the text and brailleChars > > to text - that might be better so you won't need the > > String.fromCharCode line twice. > > Isn't brailleChars used below in line 100 though? I wasn't exactly sure what the > extension bridge was so I left it alone. You're right, I missed that. That's for a way to display braille captions on Chrome on other non-ChromeOS platforms
https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:16: * @param {string|{groups:Array}|{text: string, braille: string}=} opt_data On 2016/09/27 21:15:49, ultimatedbz wrote: > On 2016/09/26 04:53:49, dmazzoni wrote: > > Delete {text: string, braille: string}, that's not used anymore since you > > replaced it with groups. > > Again, I left this here because I wasn't sure what the extension bridge was. > Should I modify the extension bridge code to also take groups instead? In this case the panel is only going to receive the groups, so it's okay to change this. The compiler should catch it if I'm wrong.
https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:60: * @param {Array} brailleToText Map of Braille letters to the first On 2016/09/26 04:53:49, dmazzoni wrote: > How about {Array<number>} - it's always good to be as specific > as possible Done. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:62: * corresponding text letter. On 2016/09/26 04:53:48, dmazzoni wrote: > nit: wrap this Done. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:74: for (var i = 0; i < byteBuf.length - 1; ++i) { On 2016/09/26 04:53:48, dmazzoni wrote: > Abstract this concept of breaking into groups into its own helper function > so it's ready for unit testing. Done. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:75: brailleChars += String.fromCharCode( On 2016/09/27 21:54:50, dmazzoni wrote: > On 2016/09/27 21:15:49, ultimatedbz wrote: > > On 2016/09/26 04:53:49, dmazzoni wrote: > > > We're not using brailleChars anymore, so you can just > > > get rid of it. > > > > > > Alternatively, do one pass to convert to brailleChars, > > > then a second pass to convert the text and brailleChars > > > to text - that might be better so you won't need the > > > String.fromCharCode line twice. > > > > Isn't brailleChars used below in line 100 though? I wasn't exactly sure what > the > > extension bridge was so I left it alone. > > You're right, I missed that. That's for a way to display braille > captions on Chrome on other non-ChromeOS platforms Done. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.css (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.css:301: table { On 2016/09/26 04:53:49, dmazzoni wrote: > nit: blank line before > > Also make this: table#braille-table so it only applies to this > table, not any table Done. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.css:306: table * { On 2016/09/26 04:53:49, dmazzoni wrote: > Same here, and below Done. https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js (right): https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:16: * @param {string|{groups:Array}|{text: string, braille: string}=} opt_data On 2016/09/27 21:56:03, dmazzoni wrote: > On 2016/09/27 21:15:49, ultimatedbz wrote: > > On 2016/09/26 04:53:49, dmazzoni wrote: > > > Delete {text: string, braille: string}, that's not used anymore since you > > > replaced it with groups. > > > > Again, I left this here because I wasn't sure what the extension bridge was. > > Should I modify the extension bridge code to also take groups instead? > > In this case the panel is only going to receive the groups, > so it's okay to change this. > > The compiler should catch it if I'm wrong. Ohh right! I see what you mean. Thanks! https://codereview.chromium.org/2362673004/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel_command.js:16: * @param {string|{groups:Array}|{text: string, braille: string}=} opt_data On 2016/09/27 21:56:03, dmazzoni wrote: > On 2016/09/27 21:15:49, ultimatedbz wrote: > > On 2016/09/26 04:53:49, dmazzoni wrote: > > > Delete {text: string, braille: string}, that's not used anymore since you > > > replaced it with groups. > > > > Again, I left this here because I wasn't sure what the extension bridge was. > > Should I modify the extension bridge code to also take groups instead? > > In this case the panel is only going to receive the groups, > so it's okay to change this. > > The compiler should catch it if I'm wrong. Done.
lgtm with nits Great job! This means that if you have no objections or questions about the suggestions I made, make the changes, respond, upload, and then check the "commit" box. One last thing - please first update the change description. Get rid of the individual git commits and consolidate to a single subject line (the subject should match the first line of the description) and brief description, and add a link to a bug. https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:60: * index of corresponding text letter. nit: indent this comment by 4 spaces since it's a continuation of the previous line https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:92: * index of corresponding text letter. nit: indent this comment by 4 spaces since it's a continuation of the previous line https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html (right): https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:37: </div> nit: it looks like you have an extra </div> here
Description was changed from ========== Aligns text to braille in chromevox. Text now aligns thanks to html tables. wrote alignment code. However, it doesn't align because of unicode. No matter, because we will align with html. Finished grouping algorithm. Able to pass array of groupings to Panel. Need to clean up code and display groups. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Aligns text to braille in chromevox. Text now aligns thanks to html tables. wrote alignment code. However, it doesn't align because of unicode. No matter, because we will align with html. Finished grouping algorithm. Able to pass array of groupings to Panel. Need to clean up code and display groups. BUG=651563 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js (right): https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:60: * index of corresponding text letter. On 2016/09/29 16:13:12, dmazzoni wrote: > nit: indent this comment by 4 spaces since it's a continuation of the previous > line Done. https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/chromevox/background/braille_captions_background.js:92: * index of corresponding text letter. On 2016/09/29 16:13:13, dmazzoni wrote: > nit: indent this comment by 4 spaces since it's a continuation of the previous > line Done. https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html (right): https://codereview.chromium.org/2362673004/diff/40001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/panel.html:37: </div> On 2016/09/29 16:13:13, dmazzoni wrote: > nit: it looks like you have an extra </div> here Done.
Description was changed from ========== Aligns text to braille in chromevox. Text now aligns thanks to html tables. wrote alignment code. However, it doesn't align because of unicode. No matter, because we will align with html. Finished grouping algorithm. Able to pass array of groupings to Panel. Need to clean up code and display groups. BUG=651563 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Aligns text to braille in chromevox. In the chromevox text to speech display where braille and regular text is shown one on top of the other, the texts are not aligned. This causes friction when looking between the braille and regular text. BUG=651563 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Tests look great, just a few things to improve readability https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs (right): https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:97: * Asserts that the last written display content is an empty buffer of Update this comment https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:101: assertGroupValid: function(groups, expected) { Maybe assertGroupsValid? https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:215: Nit: get rid of extra blank line https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:271: * to one braille letter. Use "braille cell" instead of "braille letter", and "text character" instead of "regular letter" (and in several other places) https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:279: var groups = cvox.BrailleCaptionsBackground.groupBrailleAndText(translated, text, mapping); Wrap to 80 chars, here and in one or two other places
https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs (right): https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:97: * Asserts that the last written display content is an empty buffer of On 2016/10/04 22:34:30, dmazzoni_ooo_until_oct_10 wrote: > Update this comment Done. https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:101: assertGroupValid: function(groups, expected) { On 2016/10/04 22:34:30, dmazzoni_ooo_until_oct_10 wrote: > Maybe assertGroupsValid? Done. https://codereview.chromium.org/2362673004/diff/60001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/braille/braille_display_manager_test.unitjs:271: * to one braille letter. On 2016/10/04 22:34:30, dmazzoni_ooo_until_oct_10 wrote: > Use "braille cell" instead of "braille letter", and "text character" instead of > "regular letter" (and in several other places) > Done.
still looks good, please commit when ready
The CQ bit was checked by ultimatedbz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2362673004/#ps80001 (title: "Addressed test code reviews")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ultimatedbz@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2362673004/#ps100001 (title: "fixed test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Aligns text to braille in chromevox. In the chromevox text to speech display where braille and regular text is shown one on top of the other, the texts are not aligned. This causes friction when looking between the braille and regular text. BUG=651563 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Aligns text to braille in chromevox. In the chromevox text to speech display where braille and regular text is shown one on top of the other, the texts are not aligned. This causes friction when looking between the braille and regular text. BUG=651563 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Aligns text to braille in chromevox. In the chromevox text to speech display where braille and regular text is shown one on top of the other, the texts are not aligned. This causes friction when looking between the braille and regular text. BUG=651563 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Aligns text to braille in chromevox. In the chromevox text to speech display where braille and regular text is shown one on top of the other, the texts are not aligned. This causes friction when looking between the braille and regular text. BUG=651563 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/80b4abd5c8bcd470929a6486dd7047352cfc769a Cr-Commit-Position: refs/heads/master@{#424340} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/80b4abd5c8bcd470929a6486dd7047352cfc769a Cr-Commit-Position: refs/heads/master@{#424340} |
