Fix accessibility objects with zero width and height.

Fix accessibility objects with zero width and height.

VoiceOver totally ignores a container object with zero width and
height, which can happen when an object's children all
come from aria-owns.

BUG=489590
Committed: https://crrev.com/f1a32c0101a8e0567773986d403ab92853482f75
Cr-Commit-Position: refs/heads/master@{#335852}

[dmazzoni, 2015-06-22 19:44:27]

dmazzoni@chromium.org changed reviewers:
+ dtseng@chromium.org

[David Tseng, 2015-06-22 21:29:36]

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:709: else
I'm confused why you have this branch at all?

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.h:289: // correspond to a
layed-out element sometimes don't have bounds.
What happens if there are multiple levels of empty virtual nodes?

[dmazzoni, 2015-06-23 05:36:42]

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:709: else
On 2015/06/22 21:29:36, David Tseng wrote:
> I'm confused why you have this branch at all?

If the width and height are already nonzero the function early-exits at the top.

Otherwise we enter the loop. The first time through the loop it will set *bounds
to the bounds of the first child, after that it will union it.

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.h:289: // correspond to a
layed-out element sometimes don't have bounds.
On 2015/06/22 21:29:36, David Tseng wrote:
> What happens if there are multiple levels of empty virtual nodes?

It calls GetLocalBoundsRect on each child, which calls FixEmptyBounds - so it
should work correctly with multiple levels of empty virtual nodes too.

[David Tseng, 2015-06-23 18:27:53]

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:709: else
On 2015/06/23 05:36:42, dmazzoni wrote:
> On 2015/06/22 21:29:36, David Tseng wrote:
> > I'm confused why you have this branch at all?
> 
> If the width and height are already nonzero the function early-exits at the
top.
> 
> Otherwise we enter the loop. The first time through the loop it will set
*bounds
> to the bounds of the first child, after that it will union it.

So, you're assuming child's height and width are never both 0? Is this always
the case? Could you make this more explicit by checking the index in this
branch?

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.h:289: // correspond to a
layed-out element sometimes don't have bounds.
On 2015/06/23 05:36:42, dmazzoni wrote:
> On 2015/06/22 21:29:36, David Tseng wrote:
> > What happens if there are multiple levels of empty virtual nodes?
> 
> It calls GetLocalBoundsRect on each child, which calls FixEmptyBounds - so it
> should work correctly with multiple levels of empty virtual nodes too. 

Ok got it; it's alternating recursively calling
FixEmptyBounds/GetLocalBoundsRect.

[David Tseng, 2015-06-23 18:48:58]

lgtm

[dmazzoni, 2015-06-23 23:37:11]

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/1198203002/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility.cc:709: else
On 2015/06/23 18:27:53, David Tseng wrote:
> On 2015/06/23 05:36:42, dmazzoni wrote:
> > On 2015/06/22 21:29:36, David Tseng wrote:
> > > I'm confused why you have this branch at all?
> > 
> > If the width and height are already nonzero the function early-exits at the
> top.
> > 
> > Otherwise we enter the loop. The first time through the loop it will set
> *bounds
> > to the bounds of the first child, after that it will union it.
> 
> So, you're assuming child's height and width are never both 0? Is this always
> the case? Could you make this more explicit by checking the index in this
> branch?

OK, I made this more explicit and added a comment.

However, this made me realize that we shouldn't assume the child bounds are
nonempty. Fixed that too.

[dmazzoni, 2015-06-23 23:40:47]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-06-23 23:40:47]

The patchset sent to the CQ was uploaded after l-g-t-m from dtseng@chromium.org
Link to the patchset: https://codereview.chromium.org/1198203002/#ps40001
(title: "Clarify, don't assume child bounds are nonempty too")

[commit-bot: I haz the power, 2015-06-23 23:42:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1198203002/40001

[commit-bot: I haz the power, 2015-06-24 01:51:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-24 02:01:08]

Patchset 3 (id:??) landed as
https://crrev.com/f1a32c0101a8e0567773986d403ab92853482f75
Cr-Commit-Position: refs/heads/master@{#335852}