Last few steps of text alternative computation algorithm

Last few steps of text alternative computation algorithm

BUG=473220
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201484

[aboxhall, 2015-08-19 20:48:21]

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

[aboxhall, 2015-08-19 20:48:21]

Last few cases. Looking at tests now.

[aboxhall, 2015-08-20 23:40:01]

Added a bunch of tests, fixed a bunch of issues, PTAL!

[dmazzoni, 2015-08-24 18:02:03]

lgtm

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
File LayoutTests/accessibility/name-calc-figure.html (right):

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-figure.html:14: </details>
Should be </figure>

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-figure.html:27: </details>
Same here, and below

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
File LayoutTests/accessibility/name-calc-native-markup-buttons.html (right):

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-native-markup-buttons.html:52:
assert_equals(axButton4.name, "cake");
I'm excited that this works correctly!

Could you add an example to test whitespace handling, like:

<button id="button4">My favorite food is <img src="resources/cake.png"
alt="cake">!</button>

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
File LayoutTests/accessibility/name-calc-native-markup-input-buttons.html
(right):

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-native-markup-input-buttons.html:81: },
"Input button with label wrapped and no value");
Did you mean for the <label> to wrap the <input> here?

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-native-markup-input-buttons.html:227:
assert_equals(axImageInput5.name, "Submit");
Why is this Submit?

https://codereview.chromium.org/1301993003/diff/60001/Source/modules/accessib...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1301993003/diff/60001/Source/modules/accessib...
Source/modules/accessibility/AXNodeObject.cpp:2608: // localised default value
("Submit")
Oh, got it - <input type=image> implies a Submit button by default.

[aboxhall, 2015-08-25 02:45:53]

dmazzoni@: PTAL and submit if it LGTY? I'm out for the rest of the week (so if
you do submit and it causes issues, feel free to just roll back).

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
File LayoutTests/accessibility/name-calc-figure.html (right):

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-figure.html:14: </details>
On 2015/08/24 18:02:03, dmazzoni wrote:
> Should be </figure>

Ahem :) Fixed.

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-figure.html:27: </details>
On 2015/08/24 18:02:03, dmazzoni wrote:
> Same here, and below

Done.

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
File LayoutTests/accessibility/name-calc-native-markup-buttons.html (right):

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-native-markup-buttons.html:52:
assert_equals(axButton4.name, "cake");
On 2015/08/24 18:02:03, dmazzoni wrote:
> I'm excited that this works correctly!
> 
> Could you add an example to test whitespace handling, like:
> 
> <button id="button4">My favorite food is <img src="resources/cake.png"
> alt="cake">!</button>

Done.

It turned out it _didn't_ work, so I made a small fix for that with Levi's help.

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
File LayoutTests/accessibility/name-calc-native-markup-input-buttons.html
(right):

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-native-markup-input-buttons.html:81: },
"Input button with label wrapped and no value");
On 2015/08/24 18:02:03, dmazzoni wrote:
> Did you mean for the <label> to wrap the <input> here?

Oops!

Fixing this uncovered an infinite loop issue, which I fixed in the latest patch
by adding a check for "is the label in `visited`". PTAL.

https://codereview.chromium.org/1301993003/diff/60001/LayoutTests/accessibili...
LayoutTests/accessibility/name-calc-native-markup-input-buttons.html:227:
assert_equals(axImageInput5.name, "Submit");
On 2015/08/24 18:02:03, dmazzoni wrote:
> Why is this Submit?

Yeah, you figured it out :)
The slight weirdness is that this is name-from: attribute whereas <input
type=button> and <input type=submit> have name-from: contents for the same
thing. However, I'm ok with that because they have visible text saying "Submit"
whereas this doesn't.

[aboxhall, 2015-08-25 02:57:22]

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

[aboxhall, 2015-08-25 02:57:23]

+esprehn for the inline block thing and for Source/web

[esprehn, 2015-08-30 00:07:48]

Nice tests, people don't often write thorough and nice looking tests like that!

https://codereview.chromium.org/1301993003/diff/100001/LayoutTests/accessibil...
File LayoutTests/accessibility/name-calc-img.html (right):

https://codereview.chromium.org/1301993003/diff/100001/LayoutTests/accessibil...
LayoutTests/accessibility/name-calc-img.html:4: 
nice tests!

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:341: if
(ComposedTreeTraversal::parent(*node()) &&
isHTMLDetailsElement(ComposedTreeTraversal::parent(*node())))
ComposedTreeTraversal::parent is not cheap enough you want to call it
repeatedly, I'd save it in a local variable.

ContainerNode* parent = ComposedTreeTraversal::parent(...);

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:1546: static LayoutBlockFlow*
nonInlineBlockFlow(LayoutObject* obj)
object

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:1548: LayoutObject* curr = obj;
current, we don't abbreviate like this now.

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:1551: LayoutBlockFlow* lbf =
toLayoutBlockFlow(curr);
blockFlow

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:1564: static bool
isSameNonInlineBlockFlow(LayoutObject* r1, LayoutObject* r2)
isInSameNonInlineBlockFlow? isSame makes me think they're actually the same.

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:1889: return
getAttribute(aria_valuetextAttr).string();
calling hasAttribute and then getAttribute will be slower than just calling
getAttribute and then checking isNull(). I'm not sure how perf sensitive this
code is though

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:1924: if (hasAttribute(titleAttr))
{
ditto

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:2000: *nameObjects =
localNameObjects;
I'm not a huge fan of this kind of optional out param... but dmazzoni knows the
AX code better.

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:2546: if (nameObjects) {
having both nameSources and nameObjects is confusing, can you explain what they
do?

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:2655: if
(element->fastHasAttribute(placeholderAttr)) {
checking has and then get is slower than just get and isNull()

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:2772: AtomicString summary =
getAttribute(summaryAttr);
ditto

[aboxhall, 2015-08-30 22:25:22]

https://codereview.chromium.org/1301993003/diff/100001/LayoutTests/accessibil...
File LayoutTests/accessibility/name-calc-img.html (right):

https://codereview.chromium.org/1301993003/diff/100001/LayoutTests/accessibil...
LayoutTests/accessibility/name-calc-img.html:4: 
On 2015/08/30 00:07:47, esprehn wrote:
> nice tests!

Cheers! They found a bunch of bugs as I wrote them, so it wasn't just an
intellectual exercise :D

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
File Source/modules/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:341: if
(ComposedTreeTraversal::parent(*node()) &&
isHTMLDetailsElement(ComposedTreeTraversal::parent(*node())))
On 2015/08/30 00:07:47, esprehn wrote:
> ComposedTreeTraversal::parent is not cheap enough you want to call it
> repeatedly, I'd save it in a local variable.
> 
> ContainerNode* parent = ComposedTreeTraversal::parent(...);

Done.

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:1564: static bool
isSameNonInlineBlockFlow(LayoutObject* r1, LayoutObject* r2)
On 2015/08/30 00:07:47, esprehn wrote:
> isInSameNonInlineBlockFlow? isSame makes me think they're actually the same.

Done.

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:1889: return
getAttribute(aria_valuetextAttr).string();
On 2015/08/30 00:07:47, esprehn wrote:
> calling hasAttribute and then getAttribute will be slower than just calling
> getAttribute and then checking isNull(). I'm not sure how perf sensitive this
> code is though

Done.

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:1924: if (hasAttribute(titleAttr))
{
On 2015/08/30 00:07:47, esprehn wrote:
> ditto

Done.

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:2000: *nameObjects =
localNameObjects;
On 2015/08/30 00:07:47, esprehn wrote:
> I'm not a huge fan of this kind of optional out param... but dmazzoni knows
the
> AX code better.

In recursive calls, nameObjects will always be null (as we know it will not be
used). Having nameObjects be nullable simplifies the calling code in
AXObject::recursiveTextAlternative().

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:2546: if (nameObjects) {
On 2015/08/30 00:07:48, esprehn wrote:
> having both nameSources and nameObjects is confusing, can you explain what
they
> do?

nameSources will be used by devtools frontend to do stuff like this:
https://code.google.com/p/chromium/issues/detail?id=509893&can=3&q=devtools&c...
- note that if present, the name computation algorithm will _not_ early-out, but
continue through all potential name sources.

nameObjects represents solely any AXObjects used as labels. These may be used in
platform APIs and are serialized and sent over to the content module.

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:2655: if
(element->fastHasAttribute(placeholderAttr)) {
On 2015/08/30 00:07:47, esprehn wrote:
> checking has and then get is slower than just get and isNull()

Done.

https://codereview.chromium.org/1301993003/diff/100001/Source/modules/accessi...
Source/modules/accessibility/AXNodeObject.cpp:2772: AtomicString summary =
getAttribute(summaryAttr);
On 2015/08/30 00:07:47, esprehn wrote:
> ditto

Done.

[esprehn, 2015-08-31 01:13:35]

lgtm okay

[aboxhall, 2015-08-31 13:26:09]

The CQ bit was checked by aboxhall@chromium.org

[aboxhall, 2015-08-31 13:26:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1301993003/#ps120001
(title: "esprehn comments")

[commit-bot: I haz the power, 2015-08-31 13:26:19]

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

[commit-bot: I haz the power, 2015-08-31 15:38:52]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201484