Fix bugs in accessible name calculation code exposed by Bryan Garaventa's test files.

Fix bugs in accessible name calculation code exposed by Bryan Garaventa's test files.

BUG=560900
Committed: https://crrev.com/faecb736f94853e0c12c1a232a941285e41d68de
Cr-Commit-Position: refs/heads/master@{#363028}

[dmazzoni, 2015-11-25 16:37:48]

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

[aboxhall, 2015-11-30 19:20:20]

lgtm

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/name-calc-visibility.html
(right):

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/name-calc-visibility.html:95:
assert_equals(axInput4.name, " 1 2 7 ");
Why does this one alone get the extra leading/trailing whitespace?

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/name-calc-visibility.html:96: },
"Visibility: hidden and display: none inside aria-labelledby label subtree");
This description is the same as the one on line 50, and I think it's incomplete
in this case. Some reference to the parent `display: none` div?

(BTW, I found these sentences pretty hard to parse given it seems to read "...
none inside aria-labelledby subtree"; putting back-ticks or similar around the
CSS might make it a little easier.)

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:679: // Simplify
and whitespace, but preserve a single leading and trailing whitespace
First clause seems a little weird - can't tell if it's missing a word or got an
extra one.

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:741: // This is an
obscure corner case: if a node has no LayoutObject, that means it's not
rendered,
Helpful comment, thanks!

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:763: if
(!inAriaLabelledByTraversal && isHiddenForTextAlternativeCalculation()) {
This looks much better now.

[dmazzoni, 2015-12-01 08:52:39]

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/name-calc-visibility.html
(right):

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/name-calc-visibility.html:95:
assert_equals(axInput4.name, " 1 2 7 ");
On 2015/11/30 19:20:19, aboxhall wrote:
> Why does this one alone get the extra leading/trailing whitespace?

Because all of the static text in this example comes from inside a display: none
block, it doesn't really get rendered on-screen, so we can't apply any logic
about what whitespace gets collapsed and what doesn't. To be safe we end up with
leading and trailing whitespace in this case.

It doesn't bother me that much, especially just for a corner case.

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/name-calc-visibility.html:96: },
"Visibility: hidden and display: none inside aria-labelledby label subtree");
On 2015/11/30 19:20:19, aboxhall wrote:
> This description is the same as the one on line 50, and I think it's
incomplete
> in this case. Some reference to the parent `display: none` div?
> 
> (BTW, I found these sentences pretty hard to parse given it seems to read "...
> none inside aria-labelledby subtree"; putting back-ticks or similar around the
> CSS might make it a little easier.)

Done.

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1473833002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObject.cpp:679: // Simplify
and whitespace, but preserve a single leading and trailing whitespace
On 2015/11/30 19:20:19, aboxhall wrote:
> First clause seems a little weird - can't tell if it's missing a word or got
an
> extra one.

Done.

[dmazzoni, 2015-12-01 08:52:51]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-12-01 08:52:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/1473833002/#ps100001
(title: "Fix win test")

[commit-bot: I haz the power, 2015-12-01 08:53:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473833002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473833002/100001

[commit-bot: I haz the power, 2015-12-01 10:05:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-01 10:05:55]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[dmazzoni, 2015-12-01 19:38:45]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-12-01 19:38:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/1473833002/#ps120001
(title: "Proper fix, we shouldn't be collapsing static text")

[commit-bot: I haz the power, 2015-12-01 19:39:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473833002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473833002/120001

[commit-bot: I haz the power, 2015-12-01 21:42:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-01 21:42:28]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[dmazzoni, 2015-12-02 21:54:13]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-12-02 21:54:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/1473833002/#ps140001
(title: "Fix android test expectation")

[commit-bot: I haz the power, 2015-12-02 21:55:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473833002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473833002/140001

[commit-bot: I haz the power, 2015-12-02 22:03:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 22:03:38]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[dmazzoni, 2015-12-02 22:36:25]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-12-02 22:37:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473833002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473833002/140001

[commit-bot: I haz the power, 2015-12-02 23:28:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 23:28:35]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[dmazzoni, 2015-12-03 18:17:58]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2015-12-03 18:28:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473833002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473833002/140001

[commit-bot: I haz the power, 2015-12-03 19:05:51]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-03 19:07:45]

Description was changed from

==========
Fix bugs in accessible name calculation code exposed by Bryan Garaventa's test
files.

BUG=560900
==========

to

==========
Fix bugs in accessible name calculation code exposed by Bryan Garaventa's test
files.

BUG=560900

Committed: https://crrev.com/faecb736f94853e0c12c1a232a941285e41d68de
Cr-Commit-Position: refs/heads/master@{#363028}
==========

[commit-bot: I haz the power, 2015-12-03 19:07:46]

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