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

Issue 640231 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug
Team-Accessibility



Sign in to add a comment

Web pages with nested framesets not accessible

Project Member Reported by dmazz...@chromium.org, Aug 23 2016

Issue description

Comment 1 by sgu...@chromium.org, Aug 23 2016

Cc: sgu...@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/08139c18be3cbc6199636ae63783574dd317bafd

commit 08139c18be3cbc6199636ae63783574dd317bafd
Author: dmazzoni <dmazzoni@chromium.org>
Date: Wed Aug 31 20:08:38 2016

Fix loading accessibility tree for child frame that's already loaded.

Code leftover from the pre-OOPIF days was causing us to exit early from
the RenderAccessibilityImpl constructor for some child frames that were
already loaded. Everything worked fine if accessibility was already enabled
when loading the frame, but if the frame was already loaded and then
accessibility was enabled, this could cause it to fail to create an
accessibility tree.

The code in RenderAccessibilityImpl is no longer needed because now we
have exactly one accessibility tree per frame.

This wasn't caught by tests because we didn't cover both scenarios, we
always enabled accessibility first.

Added two variants of existing tests that load the page first and then
enable accessibility.

BUG= 640231 

Review-Url: https://codereview.chromium.org/2299673002
Cr-Commit-Position: refs/heads/master@{#415733}

[modify] https://crrev.com/08139c18be3cbc6199636ae63783574dd317bafd/content/browser/accessibility/dump_accessibility_browsertest_base.cc
[modify] https://crrev.com/08139c18be3cbc6199636ae63783574dd317bafd/content/browser/accessibility/dump_accessibility_browsertest_base.h
[modify] https://crrev.com/08139c18be3cbc6199636ae63783574dd317bafd/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
[modify] https://crrev.com/08139c18be3cbc6199636ae63783574dd317bafd/content/renderer/accessibility/render_accessibility_impl.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 1 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/015e6c8aa7b0af34021c8ea7454a9e0318695d47

commit 015e6c8aa7b0af34021c8ea7454a9e0318695d47
Author: yhirano <yhirano@chromium.org>
Date: Thu Sep 01 02:23:32 2016

Revert of Fix loading accessibility tree for child frame that's already loaded. (patchset #1 id:1 of https://codereview.chromium.org/2299673002/ )

Reason for revert:
The added tests are failing on Mac. https://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/7967

Original issue's description:
> Fix loading accessibility tree for child frame that's already loaded.
>
> Code leftover from the pre-OOPIF days was causing us to exit early from
> the RenderAccessibilityImpl constructor for some child frames that were
> already loaded. Everything worked fine if accessibility was already enabled
> when loading the frame, but if the frame was already loaded and then
> accessibility was enabled, this could cause it to fail to create an
> accessibility tree.
>
> The code in RenderAccessibilityImpl is no longer needed because now we
> have exactly one accessibility tree per frame.
>
> This wasn't caught by tests because we didn't cover both scenarios, we
> always enabled accessibility first.
>
> Added two variants of existing tests that load the page first and then
> enable accessibility.
>
> BUG= 640231 
>
> Committed: https://crrev.com/08139c18be3cbc6199636ae63783574dd317bafd
> Cr-Commit-Position: refs/heads/master@{#415733}

TBR=dtseng@chromium.org,dmazzoni@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 640231 

Review-Url: https://codereview.chromium.org/2299113002
Cr-Commit-Position: refs/heads/master@{#415851}

[modify] https://crrev.com/015e6c8aa7b0af34021c8ea7454a9e0318695d47/content/browser/accessibility/dump_accessibility_browsertest_base.cc
[modify] https://crrev.com/015e6c8aa7b0af34021c8ea7454a9e0318695d47/content/browser/accessibility/dump_accessibility_browsertest_base.h
[modify] https://crrev.com/015e6c8aa7b0af34021c8ea7454a9e0318695d47/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
[modify] https://crrev.com/015e6c8aa7b0af34021c8ea7454a9e0318695d47/content/renderer/accessibility/render_accessibility_impl.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e3d6283c33ce68650657647a842f21eaacb15cd8

commit e3d6283c33ce68650657647a842f21eaacb15cd8
Author: dmazzoni <dmazzoni@chromium.org>
Date: Fri Sep 02 15:46:55 2016

Re-land: Fix loading accessibility tree for child frame that's already loaded.

(Original issue: http://crrev.com/2299673002 - fixed test failures on Mac
by disabling accessibility between subsequent runs.)

Code leftover from the pre-OOPIF days was causing us to exit early from
the RenderAccessibilityImpl constructor for some child frames that were
already loaded. Everything worked fine if accessibility was already enabled
when loading the frame, but if the frame was already loaded and then
accessibility was enabled, this could cause it to fail to create an
accessibility tree.

The code in RenderAccessibilityImpl is no longer needed because now we
have exactly one accessibility tree per frame.

This wasn't caught by tests because we didn't cover both scenarios, we
always enabled accessibility first.

Added two variants of existing tests that load the page first and then
enable accessibility.

BUG= 640231 
TBR=dtseng@chromium.org

Review-Url: https://codereview.chromium.org/2299283002
Cr-Commit-Position: refs/heads/master@{#416274}

[modify] https://crrev.com/e3d6283c33ce68650657647a842f21eaacb15cd8/content/browser/accessibility/dump_accessibility_browsertest_base.cc
[modify] https://crrev.com/e3d6283c33ce68650657647a842f21eaacb15cd8/content/browser/accessibility/dump_accessibility_browsertest_base.h
[modify] https://crrev.com/e3d6283c33ce68650657647a842f21eaacb15cd8/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
[modify] https://crrev.com/e3d6283c33ce68650657647a842f21eaacb15cd8/content/renderer/accessibility/render_accessibility_impl.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dcb4915ac03bb8ce123975fd1bead718410d0a70

commit dcb4915ac03bb8ce123975fd1bead718410d0a70
Author: dmazzoni <dmazzoni@chromium.org>
Date: Fri Sep 02 22:09:59 2016

Revert of Re-land: Fix loading accessibility tree for child frame that's already loaded. (patchset #1 id:1 of https://codereview.chromium.org/2299283002/ )

Reason for revert:
Leads to a lot of try flakes / flakes in general:

Original issue's description:
> Re-land: Fix loading accessibility tree for child frame that's already loaded.
>
> (Original issue: http://crrev.com/2299673002 - fixed test failures on Mac
> by disabling accessibility between subsequent runs.)
>
> Code leftover from the pre-OOPIF days was causing us to exit early from
> the RenderAccessibilityImpl constructor for some child frames that were
> already loaded. Everything worked fine if accessibility was already enabled
> when loading the frame, but if the frame was already loaded and then
> accessibility was enabled, this could cause it to fail to create an
> accessibility tree.
>
> The code in RenderAccessibilityImpl is no longer needed because now we
> have exactly one accessibility tree per frame.
>
> This wasn't caught by tests because we didn't cover both scenarios, we
> always enabled accessibility first.
>
> Added two variants of existing tests that load the page first and then
> enable accessibility.
>
> BUG= 640231 
> TBR=dtseng@chromium.org
>
> Committed: https://crrev.com/e3d6283c33ce68650657647a842f21eaacb15cd8
> Cr-Commit-Position: refs/heads/master@{#416274}

TBR=dtseng@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 640231 

Review-Url: https://codereview.chromium.org/2302353005
Cr-Commit-Position: refs/heads/master@{#416363}

[modify] https://crrev.com/dcb4915ac03bb8ce123975fd1bead718410d0a70/content/browser/accessibility/dump_accessibility_browsertest_base.cc
[modify] https://crrev.com/dcb4915ac03bb8ce123975fd1bead718410d0a70/content/browser/accessibility/dump_accessibility_browsertest_base.h
[modify] https://crrev.com/dcb4915ac03bb8ce123975fd1bead718410d0a70/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
[modify] https://crrev.com/dcb4915ac03bb8ce123975fd1bead718410d0a70/content/renderer/accessibility/render_accessibility_impl.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1942f9c21b858d39d748132d4a756f3526445cdb

commit 1942f9c21b858d39d748132d4a756f3526445cdb
Author: dmazzoni <dmazzoni@chromium.org>
Date: Thu Sep 08 05:40:37 2016

Re-land (2): Fix loading accessibility tree for child frame that's already loaded.

Original issue: http://crrev.com/2299673002
Second attempt: http://crrev.com/2299283002

Fix the flakiness by not trying to toggle accessibility off and on between the
two runs of the same page within each test.

Code leftover from the pre-OOPIF days was causing us to exit early from
the RenderAccessibilityImpl constructor for some child frames that were
already loaded. Everything worked fine if accessibility was already enabled
when loading the frame, but if the frame was already loaded and then
accessibility was enabled, this could cause it to fail to create an
accessibility tree.

The code in RenderAccessibilityImpl is no longer needed because now we
have exactly one accessibility tree per frame.

This wasn't caught by tests because we didn't cover both scenarios, we
always enabled accessibility first.

Added two variants of existing tests that load the page first and then
enable accessibility.

BUG= 640231 
TBR=dtseng@chromium.org

Review-Url: https://codereview.chromium.org/2317323002
Cr-Commit-Position: refs/heads/master@{#417194}

[modify] https://crrev.com/1942f9c21b858d39d748132d4a756f3526445cdb/content/browser/accessibility/dump_accessibility_browsertest_base.cc
[modify] https://crrev.com/1942f9c21b858d39d748132d4a756f3526445cdb/content/browser/accessibility/dump_accessibility_browsertest_base.h
[modify] https://crrev.com/1942f9c21b858d39d748132d4a756f3526445cdb/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
[modify] https://crrev.com/1942f9c21b858d39d748132d4a756f3526445cdb/content/renderer/accessibility/render_accessibility_impl.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 8 2016

Labels: merge-merged-2854
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1942f9c21b858d39d748132d4a756f3526445cdb

commit 1942f9c21b858d39d748132d4a756f3526445cdb
Author: dmazzoni <dmazzoni@chromium.org>
Date: Thu Sep 08 05:40:37 2016

Re-land (2): Fix loading accessibility tree for child frame that's already loaded.

Original issue: http://crrev.com/2299673002
Second attempt: http://crrev.com/2299283002

Fix the flakiness by not trying to toggle accessibility off and on between the
two runs of the same page within each test.

Code leftover from the pre-OOPIF days was causing us to exit early from
the RenderAccessibilityImpl constructor for some child frames that were
already loaded. Everything worked fine if accessibility was already enabled
when loading the frame, but if the frame was already loaded and then
accessibility was enabled, this could cause it to fail to create an
accessibility tree.

The code in RenderAccessibilityImpl is no longer needed because now we
have exactly one accessibility tree per frame.

This wasn't caught by tests because we didn't cover both scenarios, we
always enabled accessibility first.

Added two variants of existing tests that load the page first and then
enable accessibility.

BUG= 640231 
TBR=dtseng@chromium.org

Review-Url: https://codereview.chromium.org/2317323002
Cr-Commit-Position: refs/heads/master@{#417194}

[modify] https://crrev.com/1942f9c21b858d39d748132d4a756f3526445cdb/content/browser/accessibility/dump_accessibility_browsertest_base.cc
[modify] https://crrev.com/1942f9c21b858d39d748132d4a756f3526445cdb/content/browser/accessibility/dump_accessibility_browsertest_base.h
[modify] https://crrev.com/1942f9c21b858d39d748132d4a756f3526445cdb/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
[modify] https://crrev.com/1942f9c21b858d39d748132d4a756f3526445cdb/content/renderer/accessibility/render_accessibility_impl.cc

Status: Fixed (was: Assigned)

Sign in to add a comment