Issue 742353004: Implement computedRole and computedName (behind a flag)

aboxhall

https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.cpp#newcode91 Source/core/dom/Node.cpp:91: #include "modules/accessibility/AXObject.h" These are illegal includes, I had to ...

6 years ago (2014-12-03 00:00:13 UTC) #1

dmazzoni

https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.cpp File Source/core/dom/Node.cpp (right): https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.cpp#newcode91 Source/core/dom/Node.cpp:91: #include "modules/accessibility/AXObject.h" On 2014/12/03 00:00:13, aboxhall wrote: > These ...

6 years ago (2014-12-03 23:32:13 UTC) #3

aboxhall

Still lots of rough edges and fprintfs, but what do you think of this basic ...

6 years ago (2014-12-08 23:51:00 UTC) #4

Still lots of rough edges and fprintfs, but what do you think of this basic
design? Any opinions on where things should live?

https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.cpp
File Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:91: #include "modules/accessibility/AXObject.h"
On 2014/12/03 23:32:13, dmazzoni wrote:
> On 2014/12/03 00:00:13, aboxhall wrote:
> > These are illegal includes, I had to bypass-hooks to upload this.
> 
> This is as intended. What we want to do now is add a narrow, well-documented
> interface to AXObjectCache.h, which is now inside of core/dom - and
implemented
> by AXObjectCacheImpl inside of modules/accessibility
> 
> Ideally the interface should abstract all of the accessibility details away,
so
> put the guts of these functions there rather than in Node.

Done, PTAL.

https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:1426: AXObjectCache* cache =
document().alwaysCreateAxObjectCache();
On 2014/12/03 23:32:13, dmazzoni wrote:
> I think some sort of ScopedAXObjectCache class would be great. If
accessibility
> is already enabled, it wraps the existing class (by overriding the ->
operator,
> like a scoped_ptr), but if not it creates one in a RAII way.

Done, PTAL.

https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:1430: return AXObject::roleName(NoRole);
On 2014/12/03 23:32:13, dmazzoni wrote:
> nit: indentation (and below)

Done.

https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.cpp#new...
Source/core/dom/Node.cpp:1442: // TODO(aboxhall): figure out why this is
crashing
On 2014/12/03 23:32:13, dmazzoni wrote:
> Probably need to layout first. We require layout is clean before calling any
> AXObject accessors.
> 
> See WebAXObject::updateLayoutAndCheckValidity for more info, but I think you
> want something like:
> 
> document().topDocument().view()->updateLayoutAndStyleIfNeededRecursive();

Done.

https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.idl
File Source/core/dom/Node.idl (right):

https://codereview.chromium.org/742353004/diff/1/Source/core/dom/Node.idl#new...
Source/core/dom/Node.idl:69: readonly attribute DOMString? computedRole;
On 2014/12/03 23:32:13, dmazzoni wrote:
> I think you want something like [RuntimeEnabled =
ExperimentalAccessibilityAPIs]
> or something like that.
> 

Will figure that out later :)

https://codereview.chromium.org/742353004/diff/1/Source/modules/accessibility...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/742353004/diff/1/Source/modules/accessibility...
Source/modules/accessibility/AXObject.cpp:172: { EditableTextRole,
"editableText" },
On 2014/12/03 23:32:13, dmazzoni wrote:
> I think the API is only supposed to return ARIA roles, not internal roles or
> native roles.
> 
> Could you use the existing map in createARIARoleMap() from this same file?
Maybe
> we need a reverse mapping from all internal roles to their equivalent ARIA
role,
> if any - I think we have this in Chromium in content/renderer/accessibility so
> we could maybe port it to Blink.

Haven't looked into this yet.

https://codereview.chromium.org/742353004/diff/1/Source/modules/accessibility...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/742353004/diff/1/Source/modules/accessibility...
Source/modules/accessibility/AXObject.h:55: NoRole = 0,
On 2014/12/03 23:32:13, dmazzoni wrote:
> We already have UnknownRole, can that work?

Maybe... though this is more like "no role because there's no AX object".

dmazzoni

Good progress. How about splitting the AXObjectCache pointer in each AXObject into a standalone change ...

6 years ago (2014-12-09 00:49:51 UTC) #5

dmazzoni

Looking pretty good https://codereview.chromium.org/742353004/diff/180001/Source/modules/accessibility/AXObject.cpp File Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/742353004/diff/180001/Source/modules/accessibility/AXObject.cpp#newcode90 Source/modules/accessibility/AXObject.cpp:90: // "option" isn't here because it ...

6 years ago (2014-12-12 00:41:48 UTC) #9

aboxhall

Addressed comments and added a test for computedRole. Decided to rename computedText to computedName, as ...

6 years ago (2014-12-13 01:38:36 UTC) #10

adamk

First round of comments... https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjectCache.cpp File Source/core/dom/AXObjectCache.cpp (right): https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjectCache.cpp#newcode64 Source/core/dom/AXObjectCache.cpp:64: m_cache = AXObjectCache::create(m_document); This looks ...

6 years ago (2014-12-16 01:54:49 UTC) #14

aboxhall

https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjectCache.cpp File Source/core/dom/AXObjectCache.cpp (right): https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjectCache.cpp#newcode64 Source/core/dom/AXObjectCache.cpp:64: m_cache = AXObjectCache::create(m_document); On 2014/12/16 01:54:48, adamk wrote: > ...

6 years ago (2014-12-16 04:15:27 UTC) #15

adamk

https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjectCache.cpp File Source/core/dom/AXObjectCache.cpp (right): https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjectCache.cpp#newcode64 Source/core/dom/AXObjectCache.cpp:64: m_cache = AXObjectCache::create(m_document); On 2014/12/16 04:15:26, aboxhall wrote: > ...

6 years ago (2014-12-16 18:25:15 UTC) #16

https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjec...
File Source/core/dom/AXObjectCache.cpp (right):

https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjec...
Source/core/dom/AXObjectCache.cpp:64: m_cache =
AXObjectCache::create(m_document);
On 2014/12/16 04:15:26, aboxhall wrote:
> On 2014/12/16 01:54:48, adamk wrote:
> > This looks like a pretty heavy operation; is this something we'd really want
> to
> > do upon accessing a DOM property?
> 
> It's not as heavy as it might appear - this is essentially just a constructor
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> 
> It only lazily creates the actual cache, so in many cases this cache will only
> ever have one object in it.

It's "just" a constructor, but that constructor itself constructs:

5 HashMaps
2 HashSets
1 Timer
1 Vector

> I'm not really sure what the alternative would be - we probably don't want to
> always create the cache when we create the document, but it will need to be
> created at some point for these methods to work.

So my lack of understanding of Blink's accessibility code is clearly showing: it
seems like the bit of information we want is pretty small. It looks like the
only method we use on AXObjectCacheImpl is getOrCreate(), whose statefulness
mostly seems to be about caching AXObjects, something that we don't make any use
of if we're about to destroy the cache anyway.

Would it be possible to extract some of the logic from getOrCreate and use it
statically in the case where there's no AXObjectCache?

https://codereview.chromium.org/742353004/diff/260001/Source/core/testing/Int...
File Source/core/testing/InternalSettings.h (right):

https://codereview.chromium.org/742353004/diff/260001/Source/core/testing/Int...
Source/core/testing/InternalSettings.h:76: bool
m_originalExperimentalAccessibilityFeaturesEnabled;
On 2014/12/16 04:15:26, aboxhall wrote:
> On 2014/12/16 01:54:49, adamk wrote:
> > Do you need this bit? I thought experimental features were on-by-default in
> > Layout Tests...
> 
> Do you mean I should revert everything in core/testing ?

Yes, sorry, that's what I meant to say. I think you'd only need it here if you
wanted to be able to turn it on and off from inside a layout test.

https://codereview.chromium.org/742353004/diff/260001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/742353004/diff/260001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:143: static Vector<AtomicString>*
createRoleNameVector()
On 2014/12/16 04:15:26, aboxhall wrote:
> On 2014/12/16 01:54:49, adamk wrote:
> > Sorry, I'm having trouble figuring out exactly why we need this vector, can
> you
> > explain?
> 
> It's just a map of role enum to name; used on line 991. If there's an
> obvious-to-you better way to do this, please don't assume it's obvious to me;
I
> would be very glad to hear it :)

What about making the array of roles statically scoped to the file, and then
indexing into that in roleName()? You could even add a COMPILE_ASSERT that the
array has the right length, which would make your ASSERT below a compile-time
instead of a runtime error. I guess what I'm missing is what the Vector is
buying you that the array doesn't get you already.

dmazzoni

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibility/AXObject.cpp File Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibility/AXObject.cpp#newcode141 Source/modules/accessibility/AXObject.cpp:141: { NoRole, "none" }, On 2014/12/13 01:38:36, aboxhall wrote: ...

6 years ago (2014-12-16 19:18:20 UTC) #17

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibi...
Source/modules/accessibility/AXObject.cpp:141: { NoRole, "none" },
On 2014/12/13 01:38:36, aboxhall wrote:
> On 2014/12/09 00:49:51, dmazzoni wrote:
> > Actually there is an ARIA role of "none" now, it means the same as
> > "presentational", and it's *not* the same as not having an ARIA role. I
think
> > NoRole or UnknownRole should correspond to the empty string ""
> 
> Good point, although I note it doesn't seem to work, or be specced anywhere?

The spec here:
http://www.w3.org/TR/wai-aria-1.1/#none

I'm not sure why you think it doesn't work - see further down in this array:

{ NoneRole, "none" },

I believe all of the places in the code that explicitly checked for
PresentationalRole now also check for NoneRole.

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibi...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibi...
Source/modules/accessibility/AXObject.h:56: NoRole = 0,
On 2014/12/13 01:38:36, aboxhall wrote:
> On 2014/12/09 00:49:51, dmazzoni wrote:
> > I still vote for reusing UnknownRole rather than introducing a new NoRole.
> > 
> > If you introduce NoRole, you'll have to add a WebAXRole and plumb it through
> to
> > Chromium code too.
> 
> Is that accurate? It seems to compile and run fine without it.

Switch statements on enums should always explicitly handle all cases - it's a
very important tool to catch bugs at compile time. The only reason we can't do
that now is because Blink is a separate repository. As soon as the repositories
merge I'm going to fix that.

Right now we have to manually ensure that all Blink role enums are handled by
Chromium. By introducing a role that Blink won't actually return, you're
imposing a cognitive load on anyone who's tracing through the code and trying to
figure out if we're handling all of the possible roles.

> Note that we're
> never actually setting it as something's official role - it's a placeholder
for
> "this thing doesn't have any particular role".

Yes, but how is that different than UnknownRole? It's just a different name for
the same thing.

> It's also nice to have a zero value so that NumRoles accurately reflects the
> number of roles.

Great, so you're welcome to move UnknownRole to the top of the list and set it
to value 0.

https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjec...
File Source/core/dom/AXObjectCache.cpp (right):

https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjec...
Source/core/dom/AXObjectCache.cpp:64: m_cache =
AXObjectCache::create(m_document);
On 2014/12/16 18:25:14, adamk wrote:
> On 2014/12/16 04:15:26, aboxhall wrote:
> > On 2014/12/16 01:54:48, adamk wrote:
> > > This looks like a pretty heavy operation; is this something we'd really
want
> > to
> > > do upon accessing a DOM property?
> > 
> > It's not as heavy as it might appear - this is essentially just a
constructor
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > 
> > It only lazily creates the actual cache, so in many cases this cache will
only
> > ever have one object in it.
> 
> It's "just" a constructor, but that constructor itself constructs:
> 
> 5 HashMaps
> 2 HashSets
> 1 Timer
> 1 Vector
> 
> > I'm not really sure what the alternative would be - we probably don't want
to
> > always create the cache when we create the document, but it will need to be
> > created at some point for these methods to work.
> 
> So my lack of understanding of Blink's accessibility code is clearly showing:
it
> seems like the bit of information we want is pretty small. It looks like the
> only method we use on AXObjectCacheImpl is getOrCreate(), whose statefulness
> mostly seems to be about caching AXObjects, something that we don't make any
use
> of if we're about to destroy the cache anyway.
> 
> Would it be possible to extract some of the logic from getOrCreate and use it
> statically in the case where there's no AXObjectCache?

No, the problem is that computing the role or name of an accessible object may
be recursive - i.e. it may have to compute the role of its parent, etc - so we
need the AXObjectCache to own any additional AXObjects that end up getting
created implicitly, so it can delete them all when it goes out of scope.

I think we really shouldn't worry about it at this point - this is just an
experimental API and optimizing it isn't the primary concern.

If we did want to optimize it, I'd first try to amortize the cost by keeping the
AXObjectCache around for one microtask, then look into making AXObjectCache
initialization faster. I think there are some potential optimizations we could
do once we've permanently switched to Oilpan too.

But not creating an AXObjectCache would require major, major refactoring and
special cases, which is not what we want to do at this point.

https://codereview.chromium.org/742353004/diff/280001/LayoutTests/accessibili...
File LayoutTests/accessibility/computedRole.html (right):

https://codereview.chromium.org/742353004/diff/280001/LayoutTests/accessibili...
LayoutTests/accessibility/computedRole.html:1: <!DOCTYPE HTML PUBLIC
"-//IETF//DTD HTML//EN">
nit: all of the other files in this directory use lowercase-with-hyphens for the
filenames, not mixed-case, please stay consistent.

adamk

https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjectCache.cpp File Source/core/dom/AXObjectCache.cpp (right): https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjectCache.cpp#newcode64 Source/core/dom/AXObjectCache.cpp:64: m_cache = AXObjectCache::create(m_document); On 2014/12/16 19:18:19, dmazzoni wrote: > ...

6 years ago (2014-12-16 19:25:24 UTC) #18

https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjec...
File Source/core/dom/AXObjectCache.cpp (right):

https://codereview.chromium.org/742353004/diff/260001/Source/core/dom/AXObjec...
Source/core/dom/AXObjectCache.cpp:64: m_cache =
AXObjectCache::create(m_document);
On 2014/12/16 19:18:19, dmazzoni wrote:
> On 2014/12/16 18:25:14, adamk wrote:
> > On 2014/12/16 04:15:26, aboxhall wrote:
> > > On 2014/12/16 01:54:48, adamk wrote:
> > > > This looks like a pretty heavy operation; is this something we'd really
> want
> > > to
> > > > do upon accessing a DOM property?
> > > 
> > > It's not as heavy as it might appear - this is essentially just a
> constructor
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > > 
> > > It only lazily creates the actual cache, so in many cases this cache will
> only
> > > ever have one object in it.
> > 
> > It's "just" a constructor, but that constructor itself constructs:
> > 
> > 5 HashMaps
> > 2 HashSets
> > 1 Timer
> > 1 Vector
> > 
> > > I'm not really sure what the alternative would be - we probably don't want
> to
> > > always create the cache when we create the document, but it will need to
be
> > > created at some point for these methods to work.
> > 
> > So my lack of understanding of Blink's accessibility code is clearly
showing:
> it
> > seems like the bit of information we want is pretty small. It looks like the
> > only method we use on AXObjectCacheImpl is getOrCreate(), whose statefulness
> > mostly seems to be about caching AXObjects, something that we don't make any
> use
> > of if we're about to destroy the cache anyway.
> > 
> > Would it be possible to extract some of the logic from getOrCreate and use
it
> > statically in the case where there's no AXObjectCache?
> 
> No, the problem is that computing the role or name of an accessible object may
> be recursive - i.e. it may have to compute the role of its parent, etc - so we
> need the AXObjectCache to own any additional AXObjects that end up getting
> created implicitly, so it can delete them all when it goes out of scope.
> 
> I think we really shouldn't worry about it at this point - this is just an
> experimental API and optimizing it isn't the primary concern.
> 
> If we did want to optimize it, I'd first try to amortize the cost by keeping
the
> AXObjectCache around for one microtask, then look into making AXObjectCache
> initialization faster. I think there are some potential optimizations we could
> do once we've permanently switched to Oilpan too.
> 
> But not creating an AXObjectCache would require major, major refactoring and
> special cases, which is not what we want to do at this point.

Okay, looking at this from the other direction, why not just create the
Document's AXObjectCache if one of these APIs is used? If we're not interested
in optimization, that seems like the easiest thing to do, instead of adding a
whole new case to deal with?

dmazzoni

On Tue, Dec 16, 2014 at 11:25 AM, <adamk@chromium.org> wrote: > > But not creating ...

6 years ago (2014-12-16 20:11:47 UTC) #19

aboxhall

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibility/AXObject.cpp File Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibility/AXObject.cpp#newcode141 Source/modules/accessibility/AXObject.cpp:141: { NoRole, "none" }, On 2014/12/16 19:18:19, dmazzoni wrote: ...

6 years ago (2014-12-16 23:56:37 UTC) #20

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibi...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibi...
Source/modules/accessibility/AXObject.cpp:141: { NoRole, "none" },
On 2014/12/16 19:18:19, dmazzoni wrote:
> On 2014/12/13 01:38:36, aboxhall wrote:
> > On 2014/12/09 00:49:51, dmazzoni wrote:
> > > Actually there is an ARIA role of "none" now, it means the same as
> > > "presentational", and it's *not* the same as not having an ARIA role. I
> think
> > > NoRole or UnknownRole should correspond to the empty string ""
> > 
> > Good point, although I note it doesn't seem to work, or be specced anywhere?
> 
> The spec here:
> http://www.w3.org/TR/wai-aria-1.1/#none
> 
> I'm not sure why you think it doesn't work - see further down in this array:
> 
> { NoneRole, "none" },
> 
> I believe all of the places in the code that explicitly checked for
> PresentationalRole now also check for NoneRole.

Ah - I didn't look at the 1.1 spec.

As discussed - this didn't work as I expected, but probably my expectations were
wrong, and regardless it does work in other places.

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibi...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/742353004/diff/40001/Source/modules/accessibi...
Source/modules/accessibility/AXObject.h:56: NoRole = 0,
On 2014/12/16 19:18:19, dmazzoni wrote:
> On 2014/12/13 01:38:36, aboxhall wrote:
> > On 2014/12/09 00:49:51, dmazzoni wrote:
> > > I still vote for reusing UnknownRole rather than introducing a new NoRole.
> > > 
> > > If you introduce NoRole, you'll have to add a WebAXRole and plumb it
through
> > to
> > > Chromium code too.
> > 
> > Is that accurate? It seems to compile and run fine without it.
> 
> Switch statements on enums should always explicitly handle all cases - it's a
> very important tool to catch bugs at compile time. The only reason we can't do
> that now is because Blink is a separate repository. As soon as the
repositories
> merge I'm going to fix that.
> 
> Right now we have to manually ensure that all Blink role enums are handled by
> Chromium. By introducing a role that Blink won't actually return, you're
> imposing a cognitive load on anyone who's tracing through the code and trying
to
> figure out if we're handling all of the possible roles.
> 
> > Note that we're
> > never actually setting it as something's official role - it's a placeholder
> for
> > "this thing doesn't have any particular role".
> 
> Yes, but how is that different than UnknownRole? It's just a different name
for
> the same thing.
> 
> > It's also nice to have a zero value so that NumRoles accurately reflects the
> > number of roles.
> 
> Great, so you're welcome to move UnknownRole to the top of the list and set it
> to value 0.

Done.

https://codereview.chromium.org/742353004/diff/280001/LayoutTests/accessibili...
File LayoutTests/accessibility/computedRole.html (right):

https://codereview.chromium.org/742353004/diff/280001/LayoutTests/accessibili...
LayoutTests/accessibility/computedRole.html:1: <!DOCTYPE HTML PUBLIC
"-//IETF//DTD HTML//EN">
On 2014/12/16 19:18:19, dmazzoni wrote:
> nit: all of the other files in this directory use lowercase-with-hyphens for
the
> filenames, not mixed-case, please stay consistent.

Done.

adamk

A few more style nits, and please add tests for computedName. Figured I'd send these ...

6 years ago (2014-12-16 23:56:49 UTC) #21

adamk

On 2014/12/16 20:11:47, dmazzoni wrote: > On Tue, Dec 16, 2014 at 11:25 AM, <mailto:adamk@chromium.org> ...

6 years ago (2014-12-16 23:57:30 UTC) #22

aboxhall

Working on the test... https://codereview.chromium.org/742353004/diff/300001/Source/core/dom/AXObjectCache.h File Source/core/dom/AXObjectCache.h (right): https://codereview.chromium.org/742353004/diff/300001/Source/core/dom/AXObjectCache.h#newcode130 Source/core/dom/AXObjectCache.h:130: class ScopedAXObjectCache { On 2014/12/16 ...

6 years ago (2014-12-17 02:06:24 UTC) #23

dmazzoni

lgtm Changes to tests are optional, but at a minimum please add some way to ...

6 years ago (2014-12-17 06:24:19 UTC) #25

adamk

https://codereview.chromium.org/742353004/diff/360001/Source/core/dom/AXObjectCache.cpp File Source/core/dom/AXObjectCache.cpp (right): https://codereview.chromium.org/742353004/diff/360001/Source/core/dom/AXObjectCache.cpp#newcode64 Source/core/dom/AXObjectCache.cpp:64: m_cache = AXObjectCache::create(m_document); Nit: might as well do this ...

6 years ago (2014-12-17 17:47:00 UTC) #26

aboxhall

https://codereview.chromium.org/742353004/diff/260001/Source/modules/accessibility/AXObject.cpp File Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/742353004/diff/260001/Source/modules/accessibility/AXObject.cpp#newcode143 Source/modules/accessibility/AXObject.cpp:143: static Vector<AtomicString>* createRoleNameVector() On 2014/12/16 18:25:15, adamk wrote: > ...

6 years ago (2014-12-17 22:21:27 UTC) #27

adamk

Thanks for the explanation, I understand the issue now. Just a slight simplification suggestion in ...

6 years ago (2014-12-17 22:52:46 UTC) #28

aboxhall

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibility/computed-name.html File LayoutTests/accessibility/computed-name.html (right): https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibility/computed-name.html#newcode10 LayoutTests/accessibility/computed-name.html:10: <div role="alertdialog" aria-label="alertdialog name">This is an alertdialog</div> On 2014/12/17 ...

6 years ago (2014-12-18 04:34:47 UTC) #29

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
File LayoutTests/accessibility/computed-name.html (right):

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
LayoutTests/accessibility/computed-name.html:10: <div role="alertdialog"
aria-label="alertdialog name">This is an alertdialog</div>
On 2014/12/17 06:24:19, dmazzoni wrote:
> I feel like a test of computed-name shouldn't iterate over every possible
role,
> but rather every possible way for an object to have an accessible name.
> 
> How about something more like this?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/data/...

I think both axes are valuable, actually - we definitely don't need the full
matrix, but note that there are a bunch of cases in this list which don't work
as expected (bugs to follow).

Added test cases copied over from input-text-name-calc.html, found some bugs
(and whoa, fixing them took a while) - any more test cases you can suggest would
be extremely welcome! (Although I don't think we need to find/fix all possible
bugs in this change.)

Also, finding these bugs is demonstrating a subtle (?) issue with the spec - the
name is actually dependent on the platform and the assistive technology - Chrome
exposes different things on different platforms (in no small part because the
platforms in turn support different APIs), and then different assistive
technologies may choose to speak things differently, so the notion of a single
computed Name may be somewhat misleading. Anyway, this is just something we
might want to feed back into the spec discussion.

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
LayoutTests/accessibility/computed-name.html:236: </script>
On 2014/12/17 06:24:19, dmazzoni wrote:
> Optional: I've seen some tests set display:none on most of the body, so that
at
> the end all that's left is the test output, and not the text dump of all of
the
> controls.

Wouldn't that mess up the accessibility tree?

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
File LayoutTests/accessibility/computed-role.html (right):

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
LayoutTests/accessibility/computed-role.html:202:
shouldBe('roleElement.computedRole', 'role');
On 2014/12/17 06:24:19, dmazzoni wrote:
> Suppose each element had an id, then this could be:
> 
> shouldBe('document.getElementById("' + id + '").computedRole', '"' + role +
> '"');
> 
> Then it would print something like:
> 
>   PASS: document.getElementById("input-type-number").computedRole =
> "spinbutton";
> 
> ...which is more informative and much easier to debug if it were to fail.

It now prints out the role name, which should be enough to debug.

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
File LayoutTests/accessibility/computedRole-expected.txt (right):

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
LayoutTests/accessibility/computedRole-expected.txt:1: On success, you will see
a series of "PASS" messages, followed by "TEST COMPLETE".
On 2014/12/17 06:24:19, dmazzoni wrote:
> nit: this filename needs to be updated

Done.

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
LayoutTests/accessibility/computedRole-expected.txt:4: PASS
roleElement.computedRole is role
On 2014/12/17 06:24:19, dmazzoni wrote:
> Is there any way that this test output could print the roles too? Imagine if
the
> 35th line failed, how would you know which one it was?

It actually prints better information for the failure case, e.g.
FAIL roleElement.computedRole should be tree_fail (of type string). Was null (of
type object).

However, I agree this does look confusing, so I made it more like the
computed-text test by pre-computing the expected role string.

https://codereview.chromium.org/742353004/diff/360001/Source/core/dom/AXObjec...
File Source/core/dom/AXObjectCache.cpp (right):

https://codereview.chromium.org/742353004/diff/360001/Source/core/dom/AXObjec...
Source/core/dom/AXObjectCache.cpp:64: m_cache =
AXObjectCache::create(m_document);
On 2014/12/17 17:47:00, adamk wrote:
> Nit: might as well do this in the constructor, then m_cache will always be
> non-NULL after the constructor's run, and m_isScoped only gets touched in the
> constructor and destructor.

Done.

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:143: static Vector<AtomicString>*
createRoleNameVector()
On 2014/12/17 17:47:00, adamk wrote:
> I suggested an alternative to the vector, but didn't see a response. Did I
miss
> something that made that option untenable?

Acknowledged.

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.h (right):

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
Source/modules/accessibility/AXObject.h:57: AlertDialogRole = 1,
On 2014/12/17 06:24:19, dmazzoni wrote:
> nit: get rid of the = 1

Done.

https://codereview.chromium.org/742353004/diff/360001/public/web/WebAXEnums.h
File public/web/WebAXEnums.h (right):

https://codereview.chromium.org/742353004/diff/360001/public/web/WebAXEnums.h...
public/web/WebAXEnums.h:75: WebAXRoleAlertDialog = 1,
On 2014/12/17 06:24:19, dmazzoni wrote:
> nit: get rid of the = 1 here now that you have Unknown = 0

Done.

aboxhall

On 2014/12/17 22:52:46, adamk wrote: > Thanks for the explanation, I understand the issue now. ...

6 years ago (2014-12-18 06:29:55 UTC) #30

On 2014/12/17 22:52:46, adamk wrote:
> Thanks for the explanation, I understand the issue now. Just a slight
> simplification suggestion in AXObject.cpp; I don't think you need to create an
> intermediate vector.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> File Source/modules/accessibility/AXObject.cpp (right):
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:62: RoleEntry roles[] = {
> seems like this could just be a file-level static instead of filling
> rolesVector.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:138: for (size_t i = 0; i <
> roles.size(); ++i)
> This loop could stay a loop over the file-static array.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:146:
roleNameVector->resize(NumRoles);
> You can just use:
> 
> new Vector<AtomicString>(NumRoles)
> 
> on the line above.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:148: roleNameVector->at(i) =
nullAtom;
> Ah, this was what I missed the first time, that you're initializing the
default
> to nullAtom. I think this ought to be documented in the header file (that
it'll
> return nullAtom for roles that don't have a name).
> 
> Also, I'd recommend using operator[] here instead of at(), which is unusual on
> the left side of an assignment (but that's just a style nit).
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:153: for (size_t i = 0; i <
> roles.size(); ++i)
> Same here as in createARIARoleMap, this could just be a loop over the
> file-static.

(Sorry, I didn't get to these comments yet - just acknowledging that I've seen
them.)

aboxhall

On 2014/12/17 22:52:46, adamk wrote: > Thanks for the explanation, I understand the issue now. ...

6 years ago (2014-12-18 06:30:02 UTC) #31

On 2014/12/17 22:52:46, adamk wrote:
> Thanks for the explanation, I understand the issue now. Just a slight
> simplification suggestion in AXObject.cpp; I don't think you need to create an
> intermediate vector.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> File Source/modules/accessibility/AXObject.cpp (right):
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:62: RoleEntry roles[] = {
> seems like this could just be a file-level static instead of filling
> rolesVector.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:138: for (size_t i = 0; i <
> roles.size(); ++i)
> This loop could stay a loop over the file-static array.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:146:
roleNameVector->resize(NumRoles);
> You can just use:
> 
> new Vector<AtomicString>(NumRoles)
> 
> on the line above.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:148: roleNameVector->at(i) =
nullAtom;
> Ah, this was what I missed the first time, that you're initializing the
default
> to nullAtom. I think this ought to be documented in the header file (that
it'll
> return nullAtom for roles that don't have a name).
> 
> Also, I'd recommend using operator[] here instead of at(), which is unusual on
> the left side of an assignment (but that's just a style nit).
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:153: for (size_t i = 0; i <
> roles.size(); ++i)
> Same here as in createARIARoleMap, this could just be a loop over the
> file-static.

(Sorry, I didn't get to these comments yet - just acknowledging that I've seen
them.)

aboxhall

On 2014/12/17 22:52:46, adamk wrote: > Thanks for the explanation, I understand the issue now. ...

6 years ago (2014-12-18 06:30:03 UTC) #32

On 2014/12/17 22:52:46, adamk wrote:
> Thanks for the explanation, I understand the issue now. Just a slight
> simplification suggestion in AXObject.cpp; I don't think you need to create an
> intermediate vector.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> File Source/modules/accessibility/AXObject.cpp (right):
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:62: RoleEntry roles[] = {
> seems like this could just be a file-level static instead of filling
> rolesVector.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:138: for (size_t i = 0; i <
> roles.size(); ++i)
> This loop could stay a loop over the file-static array.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:146:
roleNameVector->resize(NumRoles);
> You can just use:
> 
> new Vector<AtomicString>(NumRoles)
> 
> on the line above.
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:148: roleNameVector->at(i) =
nullAtom;
> Ah, this was what I missed the first time, that you're initializing the
default
> to nullAtom. I think this ought to be documented in the header file (that
it'll
> return nullAtom for roles that don't have a name).
> 
> Also, I'd recommend using operator[] here instead of at(), which is unusual on
> the left side of an assignment (but that's just a style nit).
> 
>
https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:153: for (size_t i = 0; i <
> roles.size(); ++i)
> Same here as in createARIARoleMap, this could just be a loop over the
> file-static.

(Sorry, I didn't get to these comments yet - just acknowledging that I've seen
them.)

dmazzoni

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibility/computed-name.html File LayoutTests/accessibility/computed-name.html (right): https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibility/computed-name.html#newcode10 LayoutTests/accessibility/computed-name.html:10: <div role="alertdialog" aria-label="alertdialog name">This is an alertdialog</div> On 2014/12/18 ...

6 years ago (2014-12-18 18:02:22 UTC) #33

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
File LayoutTests/accessibility/computed-name.html (right):

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
LayoutTests/accessibility/computed-name.html:10: <div role="alertdialog"
aria-label="alertdialog name">This is an alertdialog</div>
On 2014/12/18 04:34:47, aboxhall wrote:
> On 2014/12/17 06:24:19, dmazzoni wrote:
> > I feel like a test of computed-name shouldn't iterate over every possible
> role,
> > but rather every possible way for an object to have an accessible name.
> > 
> > How about something more like this?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/data/...
> 
> I think both axes are valuable, actually - we definitely don't need the full
> matrix, but note that there are a bunch of cases in this list which don't work
> as expected (bugs to follow).
> 
> Added test cases copied over from input-text-name-calc.html, found some bugs
> (and whoa, fixing them took a while) - any more test cases you can suggest
would
> be extremely welcome! (Although I don't think we need to find/fix all possible
> bugs in this change.)
> 
> Also, finding these bugs is demonstrating a subtle (?) issue with the spec -
the
> name is actually dependent on the platform and the assistive technology -
Chrome
> exposes different things on different platforms (in no small part because the
> platforms in turn support different APIs), and then different assistive
> technologies may choose to speak things differently, so the notion of a single
> computed Name may be somewhat misleading. Anyway, this is just something we
> might want to feed back into the spec discussion.

I think that the latest spec simplifies this, finally. There really should be a
single unified accessible name. On Mac, we'll just expose it in AXTitle if it's
visible and AXDescription if it's not.

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
LayoutTests/accessibility/computed-name.html:236: </script>
On 2014/12/18 04:34:47, aboxhall wrote:
> On 2014/12/17 06:24:19, dmazzoni wrote:
> > Optional: I've seen some tests set display:none on most of the body, so that
> at
> > the end all that's left is the test output, and not the text dump of all of
> the
> > controls.
> 
> Wouldn't that mess up the accessibility tree?

You'd do it after all of your accessibility tree tests.

See LayoutTests/accessibility/title-ui-element-correctness.html for one example.

dmazzoni

https://codereview.chromium.org/742353004/diff/400001/Source/modules/accessibility/AXObjectCacheImpl.cpp File Source/modules/accessibility/AXObjectCacheImpl.cpp (right): https://codereview.chromium.org/742353004/diff/400001/Source/modules/accessibility/AXObjectCacheImpl.cpp#newcode1085 Source/modules/accessibility/AXObjectCacheImpl.cpp:1085: String AXObjectCacheImpl::computedNameForNode(Node* node) The logic for this looks fine, ...

6 years ago (2014-12-18 23:12:20 UTC) #34

aboxhall

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibility/computed-name.html File LayoutTests/accessibility/computed-name.html (right): https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibility/computed-name.html#newcode10 LayoutTests/accessibility/computed-name.html:10: <div role="alertdialog" aria-label="alertdialog name">This is an alertdialog</div> On 2014/12/18 ...

6 years ago (2014-12-19 00:02:08 UTC) #35

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
File LayoutTests/accessibility/computed-name.html (right):

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
LayoutTests/accessibility/computed-name.html:10: <div role="alertdialog"
aria-label="alertdialog name">This is an alertdialog</div>
On 2014/12/18 18:02:21, dmazzoni wrote:
> On 2014/12/18 04:34:47, aboxhall wrote:
> > On 2014/12/17 06:24:19, dmazzoni wrote:
> > > I feel like a test of computed-name shouldn't iterate over every possible
> > role,
> > > but rather every possible way for an object to have an accessible name.
> > > 
> > > How about something more like this?
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/test/data/...
> > 
> > I think both axes are valuable, actually - we definitely don't need the full
> > matrix, but note that there are a bunch of cases in this list which don't
work
> > as expected (bugs to follow).
> > 
> > Added test cases copied over from input-text-name-calc.html, found some bugs
> > (and whoa, fixing them took a while) - any more test cases you can suggest
> would
> > be extremely welcome! (Although I don't think we need to find/fix all
possible
> > bugs in this change.)
> > 
> > Also, finding these bugs is demonstrating a subtle (?) issue with the spec -
> the
> > name is actually dependent on the platform and the assistive technology -
> Chrome
> > exposes different things on different platforms (in no small part because
the
> > platforms in turn support different APIs), and then different assistive
> > technologies may choose to speak things differently, so the notion of a
single
> > computed Name may be somewhat misleading. Anyway, this is just something we
> > might want to feed back into the spec discussion.
> 
> I think that the latest spec simplifies this, finally. There really should be
a
> single unified accessible name. On Mac, we'll just expose it in AXTitle if
it's
> visible and AXDescription if it's not.

Acknowledged.

https://codereview.chromium.org/742353004/diff/360001/LayoutTests/accessibili...
LayoutTests/accessibility/computed-name.html:236: </script>
On 2014/12/18 18:02:21, dmazzoni wrote:
> On 2014/12/18 04:34:47, aboxhall wrote:
> > On 2014/12/17 06:24:19, dmazzoni wrote:
> > > Optional: I've seen some tests set display:none on most of the body, so
that
> > at
> > > the end all that's left is the test output, and not the text dump of all
of
> > the
> > > controls.
> > 
> > Wouldn't that mess up the accessibility tree?
> 
> You'd do it after all of your accessibility tree tests.
> 
> See LayoutTests/accessibility/title-ui-element-correctness.html for one
example.

Oh I see, done.

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:62: RoleEntry roles[] = {
On 2014/12/17 22:52:46, adamk wrote:
> seems like this could just be a file-level static instead of filling
> rolesVector.

Done.

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:138: for (size_t i = 0; i <
roles.size(); ++i)
On 2014/12/17 22:52:46, adamk wrote:
> This loop could stay a loop over the file-static array.

Done.

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:146: roleNameVector->resize(NumRoles);
On 2014/12/17 22:52:46, adamk wrote:
> You can just use:
> 
> new Vector<AtomicString>(NumRoles)
> 
> on the line above.

Oh yeah :)

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:148: roleNameVector->at(i) = nullAtom;
On 2014/12/17 22:52:46, adamk wrote:
> Ah, this was what I missed the first time, that you're initializing the
default
> to nullAtom. I think this ought to be documented in the header file (that
it'll
> return nullAtom for roles that don't have a name).
> 
> Also, I'd recommend using operator[] here instead of at(), which is unusual on
> the left side of an assignment (but that's just a style nit).

Oh yeah, and for good reason (as I found out when I tried to use it in the loop
below and ended up with all sorts of weirdness). Done.

https://codereview.chromium.org/742353004/diff/360001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:153: for (size_t i = 0; i <
roles.size(); ++i)
On 2014/12/17 22:52:46, adamk wrote:
> Same here as in createARIARoleMap, this could just be a loop over the
> file-static.

Done.

https://codereview.chromium.org/742353004/diff/400001/Source/modules/accessib...
File Source/modules/accessibility/AXObjectCacheImpl.cpp (right):

https://codereview.chromium.org/742353004/diff/400001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1085: String
AXObjectCacheImpl::computedNameForNode(Node* node)
On 2014/12/18 23:12:20, dmazzoni wrote:
> The logic for this looks fine, but instead of putting it here, how about
putting
> it in AXObject? Call it computedName and add a comment saying that this should
> do the official accessible name calculation, and should eventually replace
> title().

Ok, done.

https://codereview.chromium.org/742353004/diff/400001/Source/modules/accessib...
Source/modules/accessibility/AXObjectCacheImpl.cpp:1110: String placeholder;
On 2014/12/18 23:12:20, dmazzoni wrote:
> The official accessible name calculation says that the placeholder attribute
> overrides the title attribute - is it doing that correctly?
> 
> (Not AXObject::title, but the HTML attr "title", as in a tooltip.)

Yeah, it doesn't. I added a test case but I don't really want to fix the issue
in this change, as discussed.

adamk

Code lgtm now https://codereview.chromium.org/742353004/diff/420001/Source/modules/accessibility/AXObject.cpp File Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/742353004/diff/420001/Source/modules/accessibility/AXObject.cpp#newcode60 Source/modules/accessibility/AXObject.cpp:60: RoleEntry roles[] = { Hopefully final ...

6 years ago (2014-12-19 00:07:46 UTC) #36

aboxhall

https://codereview.chromium.org/742353004/diff/420001/Source/modules/accessibility/AXObject.cpp File Source/modules/accessibility/AXObject.cpp (right): https://codereview.chromium.org/742353004/diff/420001/Source/modules/accessibility/AXObject.cpp#newcode60 Source/modules/accessibility/AXObject.cpp:60: RoleEntry roles[] = { On 2014/12/19 00:07:46, adamk wrote: ...

6 years ago (2014-12-19 00:20:38 UTC) #37