Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2601883002:
MacViews/a11y: Mark Views as invisible if their parents are invisible. (Closed)
|
Created:
3 years, 11 months ago by Patti Lor Modified:
3 years, 11 months ago CC:
chromium-reviews, aboxhall+watch_chromium.org, tfarina, nektar+watch_chromium.org, yuzo+watch_chromium.org, je_julie, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews/a11y: Mark Views as invisible if their parents are invisible.
Currently, only Views returning false from View::visible() will be marked
invisible to a11y clients. This doesn't account for all invisible Views, since
Views which have an invisible ancestor will also be invisible, but still return
true from View::visible(). Account for this when tagging Views invisible for
a11y by switching to View::IsDrawn() instead.
MacViews a11y already ignores invisible Views, so this will fix issues where
a11y clients are seeing invisible controls.
BUG=657848, 660230
TEST=On Mac, with the #secondary-ui-md flag turned on, open the OIB by clicking
the icon on the left of the URL, then open the collected cookies dialog by
clicking the link under "Cookies", which should read "<X> in use". Turn on
VoiceOver by pressing Cmd+F5 and navigate through the contents of the "Allowed"
tab with VO by pressing Ctrl+Right Arrow. Verify it doesn't read out controls
that belong to the "Blocked" tab.
Committed: https://crrev.com/452d2878e6c634245c5757fc0549876e4631722c
Cr-Commit-Position: refs/heads/master@{#441001}
Patch Set 1 #Patch Set 2 : Add test. #
Total comments: 9
Patch Set 3 : Switch to View::IsDrawn(). #
Messages
Total messages: 29 (19 generated)
The CQ bit was checked by patricialor@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 patricialor@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...
Description was changed from ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since some of them will be invisible because one of their ancestors is, thus returning true from View::visible(). Account for this when tagging Views invisible for a11y by recursively checking parent visibility. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 ========== to ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since Views which have an invisible ancestor will also be invisible, but still return true from View::visible(). Account for this when tagging Views invisible for a11y by recursively checking parent visibility. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
patricialor@chromium.org changed reviewers: + karandeepb@chromium.org
Hey Karan, PTAL! :P The first bug has a repro case, but the CL attached to the second bug has a bit more context (it's the change that ignores invisible views in the first place). Thanks.
Maybe also add a TEST= field Patti? https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility.cc:103: if (!IsViewVisible(view_)) It seems to me that View::IsDrawn accomplishes the same thing. Think you can use it. https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility_unittest.cc:91: TEST_F(NativeViewAccessibilityTest, InvisibleViews) { Doesn't this first require widget_->Show() or is the widget already visible? You can probably start with EXPECT_TRUE(widget_->IsVisible()). Also, you can probably add a comment for the test like - Verify views with invisible ancestors have AX_STATE_INVISIBLE even if View::visible() returns true for them. https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility_unittest.cc:99: EXPECT_TRUE( I believe this is FALSE currently, which this CL corrects?
https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility.cc:21: while (view) { Also a visible view with no parent would return true, which is probably incorrect.
Description was changed from ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since Views which have an invisible ancestor will also be invisible, but still return true from View::visible(). Account for this when tagging Views invisible for a11y by recursively checking parent visibility. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 ========== to ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since Views which have an invisible ancestor will also be invisible, but still return true from View::visible(). Account for this when tagging Views invisible for a11y by recursively checking parent visibility. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 TEST=On Mac, with the #secondary-ui-md flag turned on, open the OIB by clicking the icon on the left of the URL, then open the collected cookies dialog by clicking the link under "Cookies", which should read "<X> in use". Turn on VoiceOver by pressing Cmd+F5 and navigate through the contents of the "Allowed" tab with VO by pressing Ctrl+Right Arrow. Verify it doesn't read out controls that belong to the "Blocked" tab. ==========
The CQ bit was checked by patricialor@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...
Thanks Karan, have also added a TEST= field now :) https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... File ui/views/accessibility/native_view_accessibility.cc (right): https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility.cc:21: while (view) { On 2016/12/28 10:59:59, karandeepb wrote: > Also a visible view with no parent would return true, which is probably > incorrect. This was fixed when replacing this method with IsDrawn() :) https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility.cc:103: if (!IsViewVisible(view_)) On 2016/12/28 10:57:10, karandeepb wrote: > It seems to me that View::IsDrawn accomplishes the same thing. Think you can use > it. Oops, you're totally right. Thanks for pointing it out :D https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility_unittest.cc:91: TEST_F(NativeViewAccessibilityTest, InvisibleViews) { On 2016/12/28 10:57:10, karandeepb wrote: > Doesn't this first require widget_->Show() or is the widget already visible? You > can probably start with EXPECT_TRUE(widget_->IsVisible()). > > Also, you can probably add a comment for the test like - Verify views with > invisible ancestors have AX_STATE_INVISIBLE even if View::visible() returns true > for them. It seems to work still even when the widget is invisible, which also seems a bit weird to me :S Added widget_->Show() to SetUp(), plus your IsVisible() suggestion and the comment as well. I left out the bit about View::visible returning true because it's an implementation detail the test doesn't really need to know about. https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility_unittest.cc:99: EXPECT_TRUE( On 2016/12/28 10:57:10, karandeepb wrote: > I believe this is FALSE currently, which this CL corrects? Yep! Have checked it fails without changes to native_view_accessibility.cc.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. You'll also need to update the CL description. It still says - "Account for this when tagging Views invisible for a11y by recursively checking parent visibility." https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... File ui/views/accessibility/native_view_accessibility_unittest.cc (right): https://codereview.chromium.org/2601883002/diff/20001/ui/views/accessibility/... ui/views/accessibility/native_view_accessibility_unittest.cc:91: TEST_F(NativeViewAccessibilityTest, InvisibleViews) { On 2016/12/29 00:57:56, Patti Lor wrote: > On 2016/12/28 10:57:10, karandeepb wrote: > > Doesn't this first require widget_->Show() or is the widget already visible? > You > > can probably start with EXPECT_TRUE(widget_->IsVisible()). > > > > Also, you can probably add a comment for the test like - Verify views with > > invisible ancestors have AX_STATE_INVISIBLE even if View::visible() returns > true > > for them. > > It seems to work still even when the widget is invisible, which also seems a bit > weird to me Weird!
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
lgtm
Description was changed from ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since Views which have an invisible ancestor will also be invisible, but still return true from View::visible(). Account for this when tagging Views invisible for a11y by recursively checking parent visibility. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 TEST=On Mac, with the #secondary-ui-md flag turned on, open the OIB by clicking the icon on the left of the URL, then open the collected cookies dialog by clicking the link under "Cookies", which should read "<X> in use". Turn on VoiceOver by pressing Cmd+F5 and navigate through the contents of the "Allowed" tab with VO by pressing Ctrl+Right Arrow. Verify it doesn't read out controls that belong to the "Blocked" tab. ========== to ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since Views which have an invisible ancestor will also be invisible, but still return true from View::visible(). Account for this when tagging Views invisible for a11y by switching to View::IsDrawn() instead. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 TEST=On Mac, with the #secondary-ui-md flag turned on, open the OIB by clicking the icon on the left of the URL, then open the collected cookies dialog by clicking the link under "Cookies", which should read "<X> in use". Turn on VoiceOver by pressing Cmd+F5 and navigate through the contents of the "Allowed" tab with VO by pressing Ctrl+Right Arrow. Verify it doesn't read out controls that belong to the "Blocked" tab. ==========
Thanks both for the reviews! Have updated the CL description as Karan said.
The CQ bit was checked by patricialor@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483051442810730,
"parent_rev": "6ae05405f25c64143c4baabb12548893f14b6601", "commit_rev":
"8a808b1e6830c875a87a65f0660f1a4793c8560f"}
Message was sent while issue was closed.
Description was changed from ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since Views which have an invisible ancestor will also be invisible, but still return true from View::visible(). Account for this when tagging Views invisible for a11y by switching to View::IsDrawn() instead. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 TEST=On Mac, with the #secondary-ui-md flag turned on, open the OIB by clicking the icon on the left of the URL, then open the collected cookies dialog by clicking the link under "Cookies", which should read "<X> in use". Turn on VoiceOver by pressing Cmd+F5 and navigate through the contents of the "Allowed" tab with VO by pressing Ctrl+Right Arrow. Verify it doesn't read out controls that belong to the "Blocked" tab. ========== to ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since Views which have an invisible ancestor will also be invisible, but still return true from View::visible(). Account for this when tagging Views invisible for a11y by switching to View::IsDrawn() instead. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 TEST=On Mac, with the #secondary-ui-md flag turned on, open the OIB by clicking the icon on the left of the URL, then open the collected cookies dialog by clicking the link under "Cookies", which should read "<X> in use". Turn on VoiceOver by pressing Cmd+F5 and navigate through the contents of the "Allowed" tab with VO by pressing Ctrl+Right Arrow. Verify it doesn't read out controls that belong to the "Blocked" tab. Review-Url: https://codereview.chromium.org/2601883002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since Views which have an invisible ancestor will also be invisible, but still return true from View::visible(). Account for this when tagging Views invisible for a11y by switching to View::IsDrawn() instead. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 TEST=On Mac, with the #secondary-ui-md flag turned on, open the OIB by clicking the icon on the left of the URL, then open the collected cookies dialog by clicking the link under "Cookies", which should read "<X> in use". Turn on VoiceOver by pressing Cmd+F5 and navigate through the contents of the "Allowed" tab with VO by pressing Ctrl+Right Arrow. Verify it doesn't read out controls that belong to the "Blocked" tab. Review-Url: https://codereview.chromium.org/2601883002 ========== to ========== MacViews/a11y: Mark Views as invisible if their parents are invisible. Currently, only Views returning false from View::visible() will be marked invisible to a11y clients. This doesn't account for all invisible Views, since Views which have an invisible ancestor will also be invisible, but still return true from View::visible(). Account for this when tagging Views invisible for a11y by switching to View::IsDrawn() instead. MacViews a11y already ignores invisible Views, so this will fix issues where a11y clients are seeing invisible controls. BUG=657848, 660230 TEST=On Mac, with the #secondary-ui-md flag turned on, open the OIB by clicking the icon on the left of the URL, then open the collected cookies dialog by clicking the link under "Cookies", which should read "<X> in use". Turn on VoiceOver by pressing Cmd+F5 and navigate through the contents of the "Allowed" tab with VO by pressing Ctrl+Right Arrow. Verify it doesn't read out controls that belong to the "Blocked" tab. Committed: https://crrev.com/452d2878e6c634245c5757fc0549876e4631722c Cr-Commit-Position: refs/heads/master@{#441001} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/452d2878e6c634245c5757fc0549876e4631722c Cr-Commit-Position: refs/heads/master@{#441001} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
