New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 736832 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug
Team-Accessibility

Blocked on:
issue 787867



Sign in to add a comment

Remove the ability to navigate to a page behind fullscreen video

Project Member Reported by dah...@chromium.org, Jun 26 2017

Issue description

When a video is in fullscreen on Android, TalkBack lets you navigate to the page behind the fullscreen. This shouldn't be allowed.

 
Owner: beccahughes@chromium.org
Status: Assigned (was: Untriaged)
Becca, wanna give this a go?
Cc: beccahughes@chromium.org
Components: -Blink>Media>Controls Blink>Fullscreen
Owner: ----
Status: Untriaged (was: Assigned)

Comment 3 by e...@chromium.org, Jul 6 2017

Components: -Blink>Fullscreen UI>Accessibility>Compatibility
Cc: foolip@chromium.org
Components: -UI>Accessibility>Compatibility Blink>Fullscreen
Status: Available (was: Untriaged)
foolip@, how much work would this require?
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.)
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.
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 ).
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?
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. 
Labels: -Pri-3 ReleaseBlock-Stable M-63 Pri-1
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.

Comment 11 by e...@chromium.org, Oct 17 2017

Owner: foolip@chromium.org
Status: Assigned (was: Available)
Assigning to make sure this is on your radar foolip.
foolip@, any updates on this M63 blocker?
Owner: ----
Status: Available (was: Assigned)
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.
Cc: amineer@chromium.org
+amineer, as per c#10 and our commitments with the accessibility team.
Owner: lpalmaro@chromium.org
Status: Assigned (was: Available)
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.
+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. 
Labels: -M-63 M-64
Owner: foolip@chromium.org
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.
Components: Blink>Accessibility
Owner: amineer@chromium.org
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.
Cc: aboxhall@chromium.org nek...@chromium.org dmazz...@chromium.org
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?
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
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.
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.
Cc: lethalantidote@chromium.org
+lethalantidote@ who can help foolip@ if needed.
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.
Owner: dmazz...@chromium.org
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?

Blockedon: 787867
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.
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@?
With  issue 787867  fixed, this is probably also fixed, but should be verified on a build including the fix.
Cc: cma...@chromium.org
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.
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. 
Labels: Merge-Request-64
Status: Fixed (was: Assigned)
Verified in M65-65.03592.0 build with Pixel Xl.
Labels: -Merge-Request-64 Merge-Approved-64
Project Member

Comment 36 by bugdroid1@chromium.org, Dec 14 2017

Labels: -merge-approved-64 merge-merged-3282
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