Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
405433010:
Avoid processing mutations when ChromeVox active indicator moves. (Closed)
|
Created:
6 years, 5 months ago by dmazzoni Modified:
6 years, 5 months ago Reviewers:
David Tseng CC:
chromium-reviews, oshima+watch_chromium.org, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, nkostylev+watch_chromium.org, yuzo+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, stevenjb+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAvoid processing mutations when ChromeVox active indicator moves.
This makes touch exploration a bit more responsive; a lot of the processing
time was spent responding to the mutations generated from the active
indicator.
BUG=377040
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285259
Patch Set 1 #
Total comments: 5
Patch Set 2 : Simpler fix #
Total comments: 1
Patch Set 3 : aria-hidden true too #Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/chromevox/chromevox/injected/active_indicator.js (right): https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/chromevox/chromevox/injected/active_indicator.js:831: elem.setAttribute('aria-hidden', 'true'); As opposed to cvoxIgnore? https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js (right): https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:172: cvox.AriaUtil.isHidden(node))); Why was this added?
https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/chromevox/chromevox/injected/active_indicator.js (right): https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/chromevox/chromevox/injected/active_indicator.js:831: elem.setAttribute('aria-hidden', 'true'); On 2014/07/18 22:16:49, David Tseng wrote: > As opposed to cvoxIgnore? cvoxIgnore is only meant to be used for transient nodes that we briefly insert into the dom and then immediately remove, within the live region processing code, in order to test whether or not a node just changed its visibility. Since the active indicator is more permanent, and since it's possible other screen readers might be running at the same time, aria-hidden seems more sensible to me. https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js (right): https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:172: cvox.AriaUtil.isHidden(node))); On 2014/07/18 22:16:49, David Tseng wrote: > Why was this added? This was added so that we return fast when we get mutation events on the active indicator, or any other node that's aria-hidden. cvoxIgnore is not the same as aria-hidden, see below for how it's used. It's only used transiently and we couldn't use aria-hidden there because we're specifically testing whether or not a node's visibility has changed and aria-hidden might be part of that calculation.
https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... File chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js (right): https://codereview.chromium.org/405433010/diff/1/chrome/browser/resources/chr... chrome/browser/resources/chromeos/chromevox/chromevox/injected/live_regions.js:172: cvox.AriaUtil.isHidden(node))); On 2014/07/18 23:28:13, dmazzoni wrote: > On 2014/07/18 22:16:49, David Tseng wrote: > > Why was this added? > > This was added so that we return fast when we get mutation events on the active > indicator, or any other node that's aria-hidden. But, like you said below, aria-hidden nodes still need to be processed to figure out when something is removed (for aria relevant removals) right? That's why I'm not really understanding the use of aria-hidden as a criteria to ignore a node. > > cvoxIgnore is not the same as aria-hidden, see below for how it's used. It's > only used transiently and we couldn't use aria-hidden there because we're > specifically testing whether or not a node's visibility has changed and > aria-hidden might be part of that calculation. Unless I'm totally not understanding this code, shouldIgnore will return true for a node with aria-hidden set (which could have possibly been a visibility change which we do want to announce). Maybe you want to set both cvoxIgnore and aria-hidden in the active indicator and s/||/&& here.
You're totally right. I wasn't thinking about the case where aria-hidden is being added to an element and the live region aria-relevant is set to include removals. So we can't short-circuit on aria-hidden at all. Here's a much simpler change, it just adds cvoxIgnore to the active indicator elements.
lgtm https://codereview.chromium.org/405433010/diff/20001/chrome/browser/resources... File chrome/browser/resources/chromeos/chromevox/chromevox/injected/active_indicator.js (right): https://codereview.chromium.org/405433010/diff/20001/chrome/browser/resources... chrome/browser/resources/chromeos/chromevox/chromevox/injected/active_indicator.js:835: elem.setAttribute('cvoxIgnore', ''); nit: might as well set aria-hidden too?
Also, once submitted, can you merge upstream? I don't think we're fully migrated yet (I'm stilling working on porting/automating the webstore publishing).
The CQ bit was checked by dmazzoni@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/405433010/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 285259 |
