Issue metadata
Sign in to add a comment
|
Remove the ability to navigate to a page behind fullscreen video |
||||||||||||||||||||||||
Issue descriptionWhen a video is in fullscreen on Android, TalkBack lets you navigate to the page behind the fullscreen. This shouldn't be allowed.
,
Jul 3 2017
,
Jul 6 2017
,
Aug 2 2017
foolip@, how much work would this require?
,
Aug 3 2017
What does "navigate" mean here? Is it somehow bringing into view the page that's in fullscreen, but without the fullscreen element? Can someone share a screen recording? (First I thought this was about navigating in the sense of changing the URL, but I think that's not the case.)
,
Aug 4 2017
TalkBack can iterate trough the tree. When an element goes fullscreen, it seems to still be able to iterate trough the tree behind the element.
,
Aug 4 2017
OK, so it will read things which aren't currently visible? Could someone test if this is a problem with the <dialog> element as well? If, by any chance, it isn't, then this problem should resolve itself when fullscreen and <dialog> are consolidated ( issue 240576 ).
,
Aug 4 2017
It seems that dialog doesn't have that bug. However, it is unclear to me if this is because of it being a top layer or because of a11y code specific to dialog. Do you have an ETA on bug 240576 being fixed?
,
Aug 4 2017
Without running any code, I suspect that AXNodeObject::IsModal() is what makes this work for dialog, and that code won't automatically work for fullscreen. Adding a fullscreen check there might fix the problem right away. Bug 240576 is on my personal OKRs for the quarter, and I'm working on regressions to avoid bug 402376 (on which it depends) from being reverted. So, end of September.
,
Aug 23 2017
I'm glad to see we're making progress here. I'm going to target M63 as a RB-Stable since that branches mid October and aligns with the deadline in c#9. Please ping me before punting since we have commitments here with the accessibility team.
,
Oct 17 2017
Assigning to make sure this is on your radar foolip.
,
Oct 30 2017
foolip@, any updates on this M63 blocker?
,
Nov 4 2017
Sorry, unassigning myself to make it clear that I'm not working on this, as it's not a regression. For the same reason I don't think this should be RBS, but I'll leave that for someone else to judge.
,
Nov 6 2017
+amineer, as per c#10 and our commitments with the accessibility team.
,
Nov 7 2017
I'm a little disappointed here - how do we go from "this will be done by end of Sept" back in Aug all the way to "I'm not working on this and don't think it's important" in Nov, with no updates in between? lpalmaro@, can you please help by commenting on relative priority here? How bad do you see this bug from an accessibility POV - Pri-1, Pri-2, or Pri-3? This was an issue that came up in our discussion with the Android accessibility team a few milestones back. We committed to fix this in a reasonable timeframe, which seemed viable given c#9, but it looks like that's no longer the case. If you think this is high priority, we will treat it as such - if you think it can wait a bit, please let us know.
,
Nov 9 2017
+1 to the sentiments in #15. This really needs to be fixed, and I would consider this a P1 issue, especially since it's been around for awhile and leads to a really unpleasant experience and makes it much harder to find the actual video controls. I understand it might not be possible to get this in for 63 if we don't yet have an owner, but I would say that we need this to be fixed by 64 at the latest.
,
Nov 9 2017
Re-assigning back to foolip@ given c#16. foolip@, can you please commit to fixing this in M64 (which branches Nov 30)? If not please let me know by tomorrow. My understanding from mlamouri@ is that you are the best person suited to address this but please let me know if you feel otherwise and we can work with Blink management to find another owner - but I want to be able to escalate tomorrow if needed.
,
Nov 13 2017
I was OOO last week Thu-Fri (recovering from TPAC) but here I am. Since I suspect that the fix and tests will be in modules/accessibility, and testing it (manually) requires interacting with TalkBack, I'm adding the Blink>Accessibility component and assigning back to amineer@ to escalate and find a suitable owner.
,
Nov 15 2017
Since there was some urgency here and I see there's no new owner yet, I uploaded to reviews that might be moving in the right direction: https://chromium-review.googlesource.com/c/chromium/src/+/771152 (to discover tests) https://chromium-review.googlesource.com/c/chromium/src/+/771154 (a possible fix) CC Source/modules/accessibility/OWNERS aboxhall@, dmazzoni@ and nektar@. With this possible fix in hand, I'm not sure how to test it on a device to see if it really works, even if I had a unit test for it. Would any of you be able to either try the fix, or guide me through the setup steps?
,
Nov 15 2017
Your fix does not seem to fix the issue. These are my steps to reproduce: 0. Start TalkBack 1. Open https://mounirlamouri.github.io/sandbox/fullscreen/div.html 2. Using right swipe, navigate to "Fullscreen" button 3. When the page is fullscreen, use right and left swipe Expected result: does not navigate in the page Actual result: navigates across the various divs
,
Nov 16 2017
Thanks Mounir! Following https://support.google.com/accessibility/android/answer/6007100 I can use the feature and see the problem, pretty straightforward. With https://output.jsbin.com/bemavet I can also see it working differently, skipping over the divs outside the dialog. I'll check if there's some other candidate code path that might might be responsible. modules/accessibility/OWNERS, ideas still very much appreciated.
,
Nov 16 2017
Working backwards in cs.chromium.org from AXNodeObject::IsModal(), the number of possible things that could be involved here grows, and I can't see anything obviously worth pursuing. Emailing a11y owners.
,
Nov 17 2017
+lethalantidote@ who can help foolip@ if needed.
,
Nov 20 2017
The first CL from #19 revealed that these are the tests: accessibility/aom-boolean-properties.html accessibility/aria-modal.html But since the proposed fix doesn't actually work, that doesn't really help.
,
Nov 21 2017
I'm happy to own this, but I've got a lot of other priorities and can't promise this for Chrome 64. I don't believe this is a regression, the bug has been there for a while - is there a reason it's a release blocker now?
,
Nov 22 2017
,
Dec 6 2017
Re: why it's a blocker now, because we promised the Android team we'd fix it in a reasonable timeframe, and lpalmaro@ feels it's a pretty important accessibility issue. Seems as though M64 might be tight given it's already branched and the blocking bug isn't yet closed out, so M65 might be OK, but we really need to stop punting this and get it fixed at some point in the near future.
,
Dec 7 2017
It's quite likely that fixing issue 787867 will also fix this with no further changes, and that's been landed and reverted. Now trying to make a test not flaky in https://chromium-review.googlesource.com/c/chromium/src/+/810584. I could request merge of that to M64 if it's likely to be approved, WDYT, amineer@?
,
Dec 8 2017
With issue 787867 fixed, this is probably also fixed, but should be verified on a build including the fix.
,
Dec 8 2017
Re: merging to M64, it looks like the fix is only ~20 LOC (though in Blink, which worries me a bit). Since we're so early in the release cycle, if this is deemed low risk, then I'd support it but ultimately that decision falls to cmasso@ now. At the very least we can now tell the Android team we have a fix that'll be rolling out soon (64 or 65) which is a much, much better story than in months past. Thanks for helping address this, appreciate it.
,
Dec 8 2017
Please verify the fix on M65 and confirm everything looks good. Then request merge approval for M64. We branched last week so we have sufficient time for this to bake in M64 beta and test it thoroughly to make sure if is working as expected.
,
Dec 13 2017
,
Dec 13 2017
Verified in M65-65.03592.0 build with Pixel Xl.
,
Dec 13 2017
,
Dec 14 2017
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b078ac342807ec59b7b994d8e59145d698337e72 commit b078ac342807ec59b7b994d8e59145d698337e72 Author: Philip Jägenstedt <foolip@chromium.org> Date: Thu Dec 14 10:13:04 2017 Reland "Make elements outside of the fullscreen element inert" This is a reland of f3535b851736a4163abee41073c753d7cdf714d5 The IgnoreElementsOutsideFullscreenElement test was originally flaky, due to changing textContent not reliably causing the accessibility tree to be updated. See https://crbug.com/793078 . Original change's description: > Make elements outside of the fullscreen element inert > > There is no spec for this, but an old open spec issue: > https://github.com/whatwg/fullscreen/issues/15 > > Bug: 787867 > Change-Id: Icde796405fca96e910480aef6f0d6be835f7a27a > Reviewed-on: https://chromium-review.googlesource.com/788052 > Commit-Queue: Philip Jägenstedt <foolip@chromium.org> > Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> > Cr-Commit-Position: refs/heads/master@{#520620} Bug: 736832 , 787867 , 793078 Change-Id: I5ddc56a8783dce77acf02728afc9ed621145e222 Reviewed-on: https://chromium-review.googlesource.com/810584 Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#522771}(cherry picked from commit 17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2) Reviewed-on: https://chromium-review.googlesource.com/826943 Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#223} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [add] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/content/browser/accessibility/fullscreen_browsertest.cc [modify] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/content/test/BUILD.gn [add] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/content/test/data/accessibility/fullscreen/links.html [modify] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/third_party/WebKit/LayoutTests/fullscreen/full-screen-contentEditable-crash-expected.txt [add] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/third_party/WebKit/LayoutTests/fullscreen/keyboard-focus.html [modify] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/third_party/WebKit/Source/core/dom/Node.cpp [modify] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by lethalantidote@chromium.org
, Jun 30 2017Status: Assigned (was: Untriaged)