Issue metadata
Sign in to add a comment
|
Select-to-speak highlights incorrect on native UI |
||||||||||||||||||||||||
Issue descriptionNative UI in Chrome OS doesn't have any character location information, so it appears we are highlighting entire static text nodes rather than the words. If we don't have character location information highlights should not be drawn at all. repro: https://drive.google.com/file/d/1DizRWBKOj3tjm5o_VIF_tRTrOY3wdouc/view version: 72.0.3618.0 We should get this fixed before M72 branch to avoid a regression.
,
Nov 27
I've also noticed that highlighting behavior is different in normal web windows as well. Images are highlighted now, for example, and they weren't highlighted before. This is also a regression. We should draw focus rings around things without actual words, and not highlights. https://drive.google.com/file/d/1jBvAwxjdMOMiHoQWi7ELZVxlvEBIFxEu/view https://drive.google.com/file/d/1pcv6ubJGTi-0e9K1FczKx91bNYihRGG-/view Sara, is this something you would like to investigate? I'm guessing it's related to your recent change to start highlighting in ARC++.
,
Nov 30
I can investigate
,
Dec 3
,
Dec 7
Adding releaseblock-beta because I think it's important to get this into M72 so we don't cause a visual regression! Thanks for working on this Sara!
,
Dec 8
Priority is being increased due to the next release approaching. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 10
FYI, M-72 Beta is scheduled for this Thurs, 12/13. Please be sure any planned work will be tested and verified by Tue, 12/11, or this issue may block the release.
,
Dec 10
,
Dec 10
We need to determine *immediately* if this is truly a RBB and fix it or remove the RBB label.
,
Dec 10
Sara is working on this change here: https://chromium-review.googlesource.com/c/chromium/src/+/1358105 The logic change is extremely small - https://chromium-review.googlesource.com/c/chromium/src/+/1358105/9/chrome/renderer/resources/extensions/automation/automation_node.js, but I think there's an issue with a test. We can change this to releaseblock-stable if the merge review team can agree to approve it for an M72 merge shortly after Beta branch. It is a regression and does need to get fixed in M72, but it's OK to go to Beta for a few days.
,
Dec 10
The M72 branch was created almost 2 weeks ago. Please re-label this as RBS if allowing this regression to hit Beta is OK.
,
Dec 10
,
Dec 11
update label to RBS
,
Dec 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2e3b6f28f1319693ffa0a8d55c4c1eba8facc465 commit 2e3b6f28f1319693ffa0a8d55c4c1eba8facc465 Author: Sara Kato <sarakato@chromium.org> Date: Tue Dec 11 06:09:49 2018 Add check for node role when calling boundsForRange. This boundsForRange should return null in Non-arc, non-text nodes. Otherwise, since bounds for these nodes exist (eg - on images), highlighting for these would occur unexpectedly on select-to-speak side. Test: Manual Bug: 908509 Change-Id: I344a050a9033d7d839cb242b1fc56196ec28bcce Reviewed-on: https://chromium-review.googlesource.com/c/1358105 Commit-Queue: Sara Kato <sarakato@chromium.org> Reviewed-by: Yuki Awano <yawano@chromium.org> Reviewed-by: David Tseng <dtseng@chromium.org> Cr-Commit-Position: refs/heads/master@{#615444} [modify] https://crrev.com/2e3b6f28f1319693ffa0a8d55c4c1eba8facc465/chrome/browser/extensions/api/automation/automation_apitest.cc [modify] https://crrev.com/2e3b6f28f1319693ffa0a8d55c4c1eba8facc465/chrome/renderer/resources/extensions/automation/automation_node.js [modify] https://crrev.com/2e3b6f28f1319693ffa0a8d55c4c1eba8facc465/chrome/test/data/extensions/api_test/automation/sites/bounds_for_range.html [modify] https://crrev.com/2e3b6f28f1319693ffa0a8d55c4c1eba8facc465/chrome/test/data/extensions/api_test/automation/tests/tabs/bounds_for_range.js
,
Dec 11
,
Dec 11
,
Dec 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ef4c932439967115bc9b4210a14b13ec2aa9bfb commit 2ef4c932439967115bc9b4210a14b13ec2aa9bfb Author: Sara Kato <sarakato@chromium.org> Date: Wed Dec 12 19:11:26 2018 Add check for node role when calling boundsForRange. This boundsForRange should return null in Non-arc, non-text nodes. Otherwise, since bounds for these nodes exist (eg - on images), highlighting for these would occur unexpectedly on select-to-speak side. Test: Manual Bug: 908509 Change-Id: I344a050a9033d7d839cb242b1fc56196ec28bcce Reviewed-on: https://chromium-review.googlesource.com/c/1358105 Commit-Queue: Sara Kato <sarakato@chromium.org> Reviewed-by: Yuki Awano <yawano@chromium.org> Reviewed-by: David Tseng <dtseng@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#615444}(cherry picked from commit 2e3b6f28f1319693ffa0a8d55c4c1eba8facc465) Reviewed-on: https://chromium-review.googlesource.com/c/1372970 Reviewed-by: Katie Dektar <katie@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#297} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/2ef4c932439967115bc9b4210a14b13ec2aa9bfb/chrome/browser/extensions/api/automation/automation_apitest.cc [modify] https://crrev.com/2ef4c932439967115bc9b4210a14b13ec2aa9bfb/chrome/renderer/resources/extensions/automation/automation_node.js [modify] https://crrev.com/2ef4c932439967115bc9b4210a14b13ec2aa9bfb/chrome/test/data/extensions/api_test/automation/sites/bounds_for_range.html [modify] https://crrev.com/2ef4c932439967115bc9b4210a14b13ec2aa9bfb/chrome/test/data/extensions/api_test/automation/tests/tabs/bounds_for_range.js
,
Dec 12
Marking fixed since Sara's changes are merged to M73 and M72.
,
Dec 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b032612edd22fa5d608faec45c4c11e57808907d commit b032612edd22fa5d608faec45c4c11e57808907d Author: Katie D <katie@chromium.org> Date: Fri Dec 14 00:37:39 2018 Fix bounds for range browser test. The previous test was pulling staticText nodes and applying BoundsForRange, which doesn't have a defined behavior, so it's no wonder they started failing. This forces inlineTextBox nodes by using a <p> instead of a <span>, and adjusts the tests now that the inlineTextBox is being tested instead of the staticText parent. Bug: 908509 Change-Id: I0a8a6b91fd0c4b3212ea9f849fb4fc8d82bc3b1b Reviewed-on: https://chromium-review.googlesource.com/c/1372310 Reviewed-by: David Tseng <dtseng@chromium.org> Commit-Queue: Katie Dektar <katie@chromium.org> Cr-Commit-Position: refs/heads/master@{#616526} [modify] https://crrev.com/b032612edd22fa5d608faec45c4c11e57808907d/chrome/browser/extensions/api/automation/automation_apitest.cc [modify] https://crrev.com/b032612edd22fa5d608faec45c4c11e57808907d/chrome/test/data/extensions/api_test/automation/sites/bounds_for_range.html [modify] https://crrev.com/b032612edd22fa5d608faec45c4c11e57808907d/chrome/test/data/extensions/api_test/automation/tests/tabs/bounds_for_range.js
,
Dec 19
Here's a summary of the rules that were executed: - OnlyMergeApprovedChange: Rule Failed -- Revision 2ef4c932439967115bc9b4210a14b13ec2aa9bfb was merged to refs/branch-heads/3626 branch with no merge approval from a TPM! Please explain why this change was merged to the branch! - AcknowledgeMerge: Notification Required --
,
Dec 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ef4c932439967115bc9b4210a14b13ec2aa9bfb Commit: 2ef4c932439967115bc9b4210a14b13ec2aa9bfb Author: sarakato@chromium.org Commiter: katie@chromium.org Date: 2018-12-12 19:11:26 +0000 UTC Add check for node role when calling boundsForRange. This boundsForRange should return null in Non-arc, non-text nodes. Otherwise, since bounds for these nodes exist (eg - on images), highlighting for these would occur unexpectedly on select-to-speak side. Test: Manual Bug: 908509 Change-Id: I344a050a9033d7d839cb242b1fc56196ec28bcce Reviewed-on: https://chromium-review.googlesource.com/c/1358105 Commit-Queue: Sara Kato <sarakato@chromium.org> Reviewed-by: Yuki Awano <yawano@chromium.org> Reviewed-by: David Tseng <dtseng@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#615444}(cherry picked from commit 2e3b6f28f1319693ffa0a8d55c4c1eba8facc465) Reviewed-on: https://chromium-review.googlesource.com/c/1372970 Reviewed-by: Katie Dektar <katie@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#297} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by katie@chromium.org
, Nov 27