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

Issue 834653 link

Starred by 5 users
Status: Fixed
Owner:
dmazz...@chromium.org
Closed: Oct 23
Cc:
aboxhall@chromium.org
█ iwk@google.com
dmazz...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug
Team-Accessibility



Sign in to add a comment

Content missing from accessibility tree

Project Member Reported by dmazz...@chromium.org, Apr 19 2018 
Try loading this extremely simple HTML. You can replace <foo> and <bar> with <span> and get the same result:

  <foo>
    <bar>
      <div>Before</div>
    </bar><div>After</div>
  </foo>

The resulting accessibility tree contains "Before", but doesn't contain "After".

Almost any tiny change to the HTML will fix it, even adding space between </bar> and <div>. It's a tiny corner case.

It seems super hard to fix. The bug is in walking the layout tree and continuations. I think we need to drop the code to walk the layout tree altogether.

Comment 1 by dmazz...@chromium.org, Aug 31 

Here's another repro, from: https://critique-ng-staging.corp.google.com/cl/210918853/playground

I minimized it to:

<span>
  <span>
    <div>
      <button>Before</button>
    </div></span><div>After

Comment 2 by iwk@google.com, Aug 31

Cc: iwk@google.com

Comment 3 by dmazz...@chromium.org, Sep 5

Cc: aboxhall@chromium.org

Comment 4 by dmazz...@chromium.org, Oct 18 

Yet another repro, minimized from: http://www.dailyrotation.com/

<div>
  <span>
    <a name="a">
      <div>cell</div>
      <div>
        Foo
        <a href="#">X</a>
      </div>
    </a>
  </span>
</div>

I'm going to try bisecting this.

Comment 5 by dmazz...@chromium.org, Oct 22

Status: Started (was: Assigned)
Turned out to be a bug that's been in the code for years, dating back to WebKit. I have a patch that fixes it, here:

https://chromium-review.googlesource.com/c/chromium/src/+/1292870
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 23 

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

commit 99bfce414a10e56ed93b8baef6d9d13651596c1c
Author: Dominic Mazzoni <dmazzoni@chromium.org>
Date: Tue Oct 23 19:26:42 2018

Accessibility shouldn't skip continuations in layout tree

Code to walk the layout tree for accessibility had a subtle bug
when encountering continuations, resulting in strange corner
cases where content could be completely missing from the
accessibility tree.

This patch switches to a new, simpler algorithm to walk the
layout tree for accessibility. It's simpler to explain and
requires less code.

All existing tests pass, included are three new
regression tests demonstrating cases that failed previously,
plus another simple test illustrating the importance of
handling continuations correctly.

Bug:  834653 

Change-Id: I0704e79c94332862065e2976b90de61697f7f83f
Reviewed-on: https://chromium-review.googlesource.com/c/1292870
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602051}
[add] https://crrev.com/99bfce414a10e56ed93b8baef6d9d13651596c1c/third_party/WebKit/LayoutTests/accessibility/block-in-inline.html
[add] https://crrev.com/99bfce414a10e56ed93b8baef6d9d13651596c1c/third_party/WebKit/LayoutTests/accessibility/continuation.html
[add] https://crrev.com/99bfce414a10e56ed93b8baef6d9d13651596c1c/third_party/WebKit/LayoutTests/accessibility/continuation2.html
[add] https://crrev.com/99bfce414a10e56ed93b8baef6d9d13651596c1c/third_party/WebKit/LayoutTests/accessibility/continuation3.html
[modify] https://crrev.com/99bfce414a10e56ed93b8baef6d9d13651596c1c/third_party/blink/renderer/modules/accessibility/ax_layout_object.cc
[modify] https://crrev.com/99bfce414a10e56ed93b8baef6d9d13651596c1c/third_party/blink/renderer/modules/accessibility/ax_node_object.cc

Comment 7 by dmazz...@chromium.org, Oct 23

Status: Fixed (was: Started)
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 25 

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

commit 563659ccbc3bb19c0797e55f0652a5109e579d9d
Author: Nektarios Paisios <nektar@chromium.org>
Date: Thu Oct 25 17:12:01 2018

Revert "Accessibility shouldn't skip continuations in layout tree"

This reverts commit 99bfce414a10e56ed93b8baef6d9d13651596c1c.

Reason for revert: Broke NVDA and Jaws access to several pages, including the Google's corp homepage.

Original change's description:
> Accessibility shouldn't skip continuations in layout tree
> 
> Code to walk the layout tree for accessibility had a subtle bug
> when encountering continuations, resulting in strange corner
> cases where content could be completely missing from the
> accessibility tree.
> 
> This patch switches to a new, simpler algorithm to walk the
> layout tree for accessibility. It's simpler to explain and
> requires less code.
> 
> All existing tests pass, included are three new
> regression tests demonstrating cases that failed previously,
> plus another simple test illustrating the importance of
> handling continuations correctly.
> 
> Bug:  834653 
> 
> Change-Id: I0704e79c94332862065e2976b90de61697f7f83f
> Reviewed-on: https://chromium-review.googlesource.com/c/1292870
> Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
> Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
> Reviewed-by: Emil A Eklund <eae@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#602051}

TBR=dmazzoni@chromium.org,szager@chromium.org,aboxhall@chromium.org,eae@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  834653 
Change-Id: I3d2bcf5300006394880c0c95221b8dea3276ca4a
Reviewed-on: https://chromium-review.googlesource.com/c/1299435
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602767}
[delete] https://crrev.com/afe65cf118cec7824601a6699ada9ebed62b0992/third_party/WebKit/LayoutTests/accessibility/block-in-inline.html
[delete] https://crrev.com/afe65cf118cec7824601a6699ada9ebed62b0992/third_party/WebKit/LayoutTests/accessibility/continuation.html
[delete] https://crrev.com/afe65cf118cec7824601a6699ada9ebed62b0992/third_party/WebKit/LayoutTests/accessibility/continuation2.html
[delete] https://crrev.com/afe65cf118cec7824601a6699ada9ebed62b0992/third_party/WebKit/LayoutTests/accessibility/continuation3.html
[modify] https://crrev.com/563659ccbc3bb19c0797e55f0652a5109e579d9d/third_party/blink/renderer/modules/accessibility/ax_layout_object.cc
[modify] https://crrev.com/563659ccbc3bb19c0797e55f0652a5109e579d9d/third_party/blink/renderer/modules/accessibility/ax_node_object.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 26 

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

commit d9dcfa787a62d6bd7dfe9d13e131cbe52d920063
Author: Dominic Mazzoni <dmazzoni@chromium.org>
Date: Fri Oct 26 06:54:15 2018

Re-land: Accessibility shouldn't skip continuations in layout tree""

Original: http://crrev.com/c/1292870
Revert: http://crrev.com/c/1299435

Fixes a corner case that wasn't addressed in the first patch,
where a LayoutObject has a continuation but no children - in
that case the continuation needs to be the first child. Adds a
new test for that case. See the diff between patch sets 1 and 2
for the fix.

Original description:

Code to walk the layout tree for accessibility had a subtle bug
when encountering continuations, resulting in strange corner
cases where content could be completely missing from the
accessibility tree.

This patch switches to a new, simpler algorithm to walk the
layout tree for accessibility. It's simpler to explain and
requires less code.

All existing tests pass, included are three new
regression tests demonstrating cases that failed previously,
plus another simple test illustrating the importance of
handling continuations correctly.

Bug:  834653 
Change-Id: Id28611f3cd4f4ffbcf4c1b4bf2db91da13a9caa0
Reviewed-on: https://chromium-review.googlesource.com/c/1299609
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603011}
[add] https://crrev.com/d9dcfa787a62d6bd7dfe9d13e131cbe52d920063/third_party/WebKit/LayoutTests/accessibility/block-in-inline.html
[add] https://crrev.com/d9dcfa787a62d6bd7dfe9d13e131cbe52d920063/third_party/WebKit/LayoutTests/accessibility/continuation.html
[add] https://crrev.com/d9dcfa787a62d6bd7dfe9d13e131cbe52d920063/third_party/WebKit/LayoutTests/accessibility/continuation2.html
[add] https://crrev.com/d9dcfa787a62d6bd7dfe9d13e131cbe52d920063/third_party/WebKit/LayoutTests/accessibility/continuation3.html
[add] https://crrev.com/d9dcfa787a62d6bd7dfe9d13e131cbe52d920063/third_party/WebKit/LayoutTests/accessibility/continuation4.html
[modify] https://crrev.com/d9dcfa787a62d6bd7dfe9d13e131cbe52d920063/third_party/blink/renderer/modules/accessibility/ax_layout_object.cc
[modify] https://crrev.com/d9dcfa787a62d6bd7dfe9d13e131cbe52d920063/third_party/blink/renderer/modules/accessibility/ax_node_object.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 29 

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

commit 5674301cefcae4f66e6c736f8744afda7eacd1e2
Author: Koji Ishii <kojii@chromium.org>
Date: Mon Oct 29 21:03:05 2018

[LayoutNG] Handle list marker in AXLayoutObject

This patch revives the lost code during the refactoring in
r603011 (CL:1299609).

Bug:  834653 
Change-Id: Ibbe49c00eb3623f5bcefbcfda28651400cc81090
Reviewed-on: https://chromium-review.googlesource.com/c/1304282
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603623}
[modify] https://crrev.com/5674301cefcae4f66e6c736f8744afda7eacd1e2/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/5674301cefcae4f66e6c736f8744afda7eacd1e2/third_party/blink/renderer/modules/accessibility/ax_layout_object.cc

Comment 11 by dmazz...@chromium.org, Oct 29 

Issue 898565 has been merged into this issue.
►

Sign in to add a comment