Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2487043002:
Refine braille output (Closed)
|
Created:
4 years, 1 month ago by David Tseng Modified:
4 years, 1 month ago Reviewers:
dmazzoni CC:
chromium-reviews, alemate+watch_chromium.org, oshima+watch_chromium.org, aboxhall+watch_chromium.org, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, arv+watch_chromium.org, dtseng+watch_chromium.org, dmazzoni+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefine braille output
- whitespace: handle exclusion of separator character ' ' for inline nodes
- fix bug where we were adding additional value selection spans when adding text for selected notifications
- general code cleanup: move logic to merge braille spans down into the code sites where they are being used
- add support for character, word navigation (i.e. show the line and move the cursor under the character selected).
BUG=663003, 545487, 523285
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/f9a9eed835172e6ecdb351901f1b5f40266d1ce3
Cr-Commit-Position: refs/heads/master@{#431492}
Patch Set 1 #Patch Set 2 : Refine braille output #Patch Set 3 : Fix test, better comment for tricky selection. #Patch Set 4 : Refine braille output #Patch Set 5 : Refine braille output #
Total comments: 17
Patch Set 6 : Simplify #
Total comments: 6
Patch Set 7 : Refine braille output #Patch Set 8 : Add additional test. #
Messages
Total messages: 39 (25 generated)
Description was changed from ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used BUG= ========== to ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used - add support for character, word navigation (i.e. show the line and move the cursor under the character selected). BUG=663003 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used - add support for character, word navigation (i.e. show the line and move the cursor under the character selected). BUG=663003 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used - add support for character, word navigation (i.e. show the line and move the cursor under the character selected). BUG=663003,545487 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used - add support for character, word navigation (i.e. show the line and move the cursor under the character selected). BUG=663003,545487 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used - add support for character, word navigation (i.e. show the line and move the cursor under the character selected). BUG=663003,545487,523285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 dtseng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
Friendly ping On Tue, Nov 8, 2016 at 3:41 PM, <dtseng@chromium.org> wrote: > Reviewers: > CL: https://codereview.chromium.org/2487043002/ > > Message: > dmazzoni@chromium.org > > Description: > Refine braille output > > - whitespace: handle exclusion of separator character ' ' for inline nodes > - fix bug where we were adding additional value selection spans when > adding text > for selected notifications > - general code cleanup: move logic to merge braille spans down into the > code > sites where they are being used > > BUG= > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation > > Affected files (+145, -84 lines): > M chrome/browser/resources/chromeos/chromevox/cvox2/ > background/background.js > M chrome/browser/resources/chromeos/chromevox/cvox2/background/editing.js > M chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js > M chrome/browser/resources/chromeos/chromevox/cvox2/ > background/output_test.extjs > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
dtseng@chromium.org changed reviewers: + dmazzoni@chromium.org
I probably need to include a reviewer. PTAL.
https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs (right): https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:165: .expectBraille('endof test'); This looks like a bug
On 2016/11/10 01:31:13, dmazzoni wrote: > https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... > File > chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs > (right): > > https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... > chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:165: > .expectBraille('endof test'); > This looks like a bug It does doesn't it? But, it actually isn't I think. <p>end<span>of test</span></p> is the markup and not <p>end <span>of test</span></p> which was the point of making this change in the first place and why I changed the test. If the first snippet visually renders with no space then I then we're right here in observing ony the in line text box data and not inserting spaces. Also, endof test is what I get when I copy/paste the example out of a page. I think for speech, we're doing the right thing by being loose or at least it's acceptable.
https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1578: // Note that there are two sets of indicies here. nit: indicies -> indices (throughout) https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1585: // The second set of indicies is set by the browser and Nit: clarify that the second set of indices you're referring to are |selStart| and |selEnd|, if I understand correctly, like you referred to |startIndex| and |endIndex|, above. How about renaming startIndex -> rangeStart, and endIndex -> rangeEnd, or something along those lines? "Index" is kind of redundant since they're all indices, but if the type is useful then to be consistent you could have rangeStartIndex, selStartIndex, and so on. https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1611: text = range.start.getText(); How about making this an if/else with a comment explaining both the speech case and the braille case. For speech, the spoken text is just the subrange, whereas for braille, it's the whole node with the cursor (or selection, right) underlined. https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1727: var hashInlineSiblings = function(dict, node) { It seems you just want a set, right? How about: addNodesOnSameLineToSet(set, node) that uses a Set()? https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1732: dict[inlineSibling.parent] = inlineSibling.parent; Note that the parent of an inline text box might be partially on the same line but not completely. Consider this html: <pre>One two\n three <span>four</span></pre> The static text object containing "One two\nthree " spans two lines. If |node| is the inline text box for "four", do you want the static text "One two\nthree " to be considered part of that line even if only part of it is on that line? Either way, it'd be helpful to comment the exact purpose of this set so that it's clear both what you expect it to contain, semantically, and how you compute it (you and I both know inline text boxes but a future hacker on this code might not understand what assumptions it's making). https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1768: curNodes.some(lookupInline)) nit: fits on previous line https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1771: separator = Output.SPACE; nit: indent https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1781: hashInlineSiblings(dict, node); Maybe hashInlineSiblings or whatever you end up calling it could handle whether it's a static text node or inline text box. Right now the logic of dealing with that subtle distinction is spread across two places, and it might be cleaner to put it all in hashInlineSiblings and document that more clearly.
https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1578: // Note that there are two sets of indicies here. On 2016/11/10 17:17:22, dmazzoni wrote: > nit: indicies -> indices (throughout) Done. https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1585: // The second set of indicies is set by the browser and On 2016/11/10 17:17:22, dmazzoni wrote: > Nit: clarify that the second set of indices you're referring to > are |selStart| and |selEnd|, if I understand correctly, like > you referred to |startIndex| and |endIndex|, above. > > How about renaming startIndex -> rangeStart, and > endIndex -> rangeEnd, or something along those lines? > "Index" is kind of redundant since they're all indices, but > if the type is useful then to be consistent you could have > rangeStartIndex, selStartIndex, and so on. Done. https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1611: text = range.start.getText(); On 2016/11/10 17:17:22, dmazzoni wrote: > How about making this an if/else with a comment explaining both the > speech case and the braille case. For speech, the spoken text is > just the subrange, whereas for braille, it's the whole node with the > cursor (or selection, right) underlined. Done (if/else). The else case is both speech and non-editable braille. Non-editable bralile probably has no selection and we want to show what ChromeVox selects. We'll have to re-think this a little more because DOM selections (i.e. anchorOffset, and friends) are not well represented here. With three ways to represent selections, I guess at some point we could abstract away with a utility class. https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1727: var hashInlineSiblings = function(dict, node) { On 2016/11/10 17:17:22, dmazzoni wrote: > It seems you just want a set, right? > > How about: addNodesOnSameLineToSet(set, node) that uses a Set()? Good point; using WeakSet now. https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1732: dict[inlineSibling.parent] = inlineSibling.parent; Alright, I simplified this more and it appears display inline is all we need. https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1768: curNodes.some(lookupInline)) On 2016/11/10 17:17:22, dmazzoni wrote: > nit: fits on previous line Acknowledged. https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1771: separator = Output.SPACE; On 2016/11/10 17:17:22, dmazzoni wrote: > nit: indent Done. https://codereview.chromium.org/2487043002/diff/80001/chrome/browser/resource... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1781: hashInlineSiblings(dict, node); On 2016/11/10 17:17:22, dmazzoni wrote: > Maybe hashInlineSiblings or whatever you end up calling it could handle whether > it's a static text node or inline text box. Right now the logic of dealing with > that subtle distinction is spread across two places, and it might be cleaner > to put it all in hashInlineSiblings and document that more clearly. Acknowledged.
The CQ bit was checked by dtseng@chromium.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1607: // This is output for speech or non-editable braille. This appears backwards from your logic. Above, you have if (this.formatOptions_.braille && !node.state.editable), but your comment says that the else case is for non-editable braille https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1747: if (result.length == 0 || hasInlineNode || prevHasInlineNode) I'm not sure we want no separator if either of them has an inline node anywhere, that might be too aggressively sticking things together with no whitespace. But I'm fine with landing this and improving as we find such cases. https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs (right): https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/cvox2/background/output_test.extjs:672: TEST_F('OutputE2ETest', 'BraileWhitespace', function() { nit: Braile -> Braille
https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1607: // This is output for speech or non-editable braille. On 2016/11/10 23:31:29, dmazzoni wrote: > This appears backwards from your logic. Above, you have if > (this.formatOptions_.braille && !node.state.editable), but your comment says > that the else case is for non-editable braille Changed comment. The code is right. In editable braille, we want to restrict to the start/end bounds of the range which is always a line. The if clause covers when we're in non-editable braille and moving by characters and words not showing just the spoken word.
The CQ bit was checked by dtseng@chromium.org
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/2487043002/#ps140001 (title: "Add additional test.")
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 dtseng@chromium.org
https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js (right): https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1607: // This is output for speech or non-editable braille. On 2016/11/10 23:54:59, David Tseng wrote: > On 2016/11/10 23:31:29, dmazzoni wrote: > > This appears backwards from your logic. Above, you have if > > (this.formatOptions_.braille && !node.state.editable), but your comment says > > that the else case is for non-editable braille > > Changed comment. > The code is right. In editable braille, we want to restrict to the start/end > bounds of the range which is always a line. The if clause covers when we're in > non-editable braille and moving by characters and words not showing just the > spoken word. Alright, to clarify things, I added a test and constrained more of this SelectionSpan. We are now only setting the SelectionSpan on non-editable content when that selection is less than the full contents of the range (it's really distracting to read braille when everything's underlined). Also, I filed a bug to clean up output.js in general in phase4. We can split a lot of these things up (e.g. BaseOutput, OutputParser, BrailleOutput, SpeechOutput, OutputRule, OutputRuleGroup, etc); should look a lot prettier afterwards.. https://codereview.chromium.org/2487043002/diff/100001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/cvox2/background/output.js:1747: if (result.length == 0 || hasInlineNode || prevHasInlineNode) On 2016/11/10 23:31:29, dmazzoni wrote: > I'm not sure we want no separator if either of them has an inline > node anywhere, that might be too aggressively sticking things > together with no whitespace. But I'm fine with landing this > and improving as we find such cases. I found good examples of block <-> inline and inline <-> block elements that look bad with separators. Namely, there's double spaces between them without this code. For example, google chrome lnk some more text is what I see for an inline link and block static text.
The CQ bit was checked by dtseng@chromium.org
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 ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used - add support for character, word navigation (i.e. show the line and move the cursor under the character selected). BUG=663003,545487,523285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used - add support for character, word navigation (i.e. show the line and move the cursor under the character selected). BUG=663003,545487,523285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used - add support for character, word navigation (i.e. show the line and move the cursor under the character selected). BUG=663003,545487,523285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Refine braille output - whitespace: handle exclusion of separator character ' ' for inline nodes - fix bug where we were adding additional value selection spans when adding text for selected notifications - general code cleanup: move logic to merge braille spans down into the code sites where they are being used - add support for character, word navigation (i.e. show the line and move the cursor under the character selected). BUG=663003,545487,523285 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/f9a9eed835172e6ecdb351901f1b5f40266d1ce3 Cr-Commit-Position: refs/heads/master@{#431492} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f9a9eed835172e6ecdb351901f1b5f40266d1ce3 Cr-Commit-Position: refs/heads/master@{#431492} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
