Check whether aria-expanded attribute is defined

Check whether aria-expanded attribute is defined

This patch introduces new enum type, AccessibilityExpanded, 
for handling expanded and collapsed states to 
follow http://goo.gl/dVClpj.

BUG=98982
R=dmazzoni@chromium.org, mkwst@chromium.org

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

[je_julie(Not used), 2014-10-24 07:43:38]

Hi Dominic and Mike,
I found we need to distinguish 'undefined' and 'false' for aria-expanded and 
there are some patches related to this.
I tried to follow basic implementation from WebKit and 
I modified it to fit to blink.

There is another patch related to aria-expand, 
the patch written by James Craig  <jcraig@apple.com> wkrev.com/166817
It makes combobox and disclosure always have expand attribute, even though
aria-expand attribute is not defined.
There is no comment as to it on the spec.
Do you need that rule as well?

I updated the patch for chromium side as well.
https://codereview.chromium.org/665843002/

please check it as well.
Thanks,

[dmazzoni, 2014-10-24 07:51:52]

To make this a bit cleaner, we should really add some new enum types, like

enum OptionalBoolean {
  TrueValue,
  FalseValue,
  UndefinedValue
}

enum CheckboxValue {
  Unchecked,
  Checked,
  Mixed
}

Would you be up for doing that? It'd require several patches but it'd really be
a nice improvement to readability.

https://codereview.chromium.org/679623002/diff/1/public/web/WebAXObject.h
File public/web/WebAXObject.h (right):

https://codereview.chromium.org/679623002/diff/1/public/web/WebAXObject.h#new...
public/web/WebAXObject.h:214: BLINK_EXPORT bool supportsARIAExpanded() const;
Nit: let's just group this one with the large set of functions above where most
other ARIA attributes are - in sorted order immediately after stringValue() and
before title().

[dmazzoni, 2014-10-24 07:56:14]

Another idea: perhaps Blink should just compute isCollapsed and isExpanded
correctly, and abstract whether or not it came from ARIA at all - how does that
sound?

[je_julie(Not used), 2014-10-24 08:49:22]

On 2014/10/24 07:56:14, dmazzoni wrote:
> Another idea: perhaps Blink should just compute isCollapsed and isExpanded
> correctly, and abstract whether or not it came from ARIA at all - how does
that
> sound?

Dominic,
Before I found patches from WebKit, 
I was implementing like your first suggestion, adding new enum types,
because it looks more general way to handle several attributes having true,false
and undefined(or some other values) as a value.
In order to handle new enum, we need a new conversion API like
ui::AXTextDirection AXTextDirectionFromBlink.

And I was also thinking your second suggestion.
In that case, we can handle it inside blink and chromium could be simple
but we have to find other solution for each attribute.

So, I prefer the first solution. As you mentioned, it could require several
patches but I will do that with pleasure.
What do you think?

[je_julie(Not used), 2014-10-24 15:16:02]

Dominic,
I updated patch to follow your review comment.

BTW, as to discussion above,
What do you think?

https://codereview.chromium.org/679623002/diff/1/public/web/WebAXObject.h
File public/web/WebAXObject.h (right):

https://codereview.chromium.org/679623002/diff/1/public/web/WebAXObject.h#new...
public/web/WebAXObject.h:214: BLINK_EXPORT bool supportsARIAExpanded() const;
Done.
On 2014/10/24 07:51:51, dmazzoni wrote:
> Nit: let's just group this one with the large set of functions above where
most
> other ARIA attributes are - in sorted order immediately after stringValue()
and
> before title().

[dmazzoni, 2014-10-24 15:39:39]

On 2014/10/24 08:49:22, je_julie wrote:
> On 2014/10/24 07:56:14, dmazzoni wrote:
> > Another idea: perhaps Blink should just compute isCollapsed and isExpanded
> > correctly, and abstract whether or not it came from ARIA at all - how does
> that
> > sound?
> 
> Dominic,
> Before I found patches from WebKit, 
> I was implementing like your first suggestion, adding new enum types,
> because it looks more general way to handle several attributes having
true,false
> and undefined(or some other values) as a value.
> In order to handle new enum, we need a new conversion API like
> ui::AXTextDirection AXTextDirectionFromBlink.
> 
> And I was also thinking your second suggestion.
> In that case, we can handle it inside blink and chromium could be simple
> but we have to find other solution for each attribute.
> 
> So, I prefer the first solution. As you mentioned, it could require several
> patches but I will do that with pleasure.
> What do you think?

OK, sounds great. After thinking about it some more, I think we should use a
custom enum for the specific case of collapsed / expanded. Make sure that the
default or most common case for an enum is "0", that will save a bit of memory
because we don't need to include it when it's that value.

enum WebAXExpanded {
  WebAXExpandedUndefined = 0,
  WebAXExpandedCollapsed,
  WebAXExpandedExpanded
}

Let's take other attributes one at a time and decide whether we need a new enum
or we can reuse an existing one for each one.

[je_julie(Not used), 2014-10-28 08:26:27]

On 2014/10/24 15:39:39, dmazzoni wrote:
> enum WebAXExpanded {
>   WebAXExpandedUndefined = 0,
>   WebAXExpandedCollapsed,
>   WebAXExpandedExpanded
> }
> 
> Let's take other attributes one at a time and decide whether we need a new
enum
> or we can reuse an existing one for each one.

Hi Dominic,

Thanks to your suggestion, I added new enum type for expanded.
This patchset handles expanded and collapsed states with same condition.
1) introduced new enumm type, WebAXExpanded. 
2) expanded state is from aria-expanded state.
3) In case of AXMenuList, 
expanded state is from aria-expanded state.
if it's not defined, it will be from state from visible, show/hide.
4) Changed API name for aria-expanded because it's not a kined of isXXX.
Normally isXXX has true or false.

And we need new enum type for general true, false, undefined for other
attribute.
Becase aria-expanded can not be a representative for those attribute.
After handle aria-expanded, I'll keep working on them.

Could you take a look at this patchset?

[dmazzoni, 2014-10-29 07:46:59]

Enum looks great

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
File Source/core/accessibility/AXMenuList.cpp (right):

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
Source/core/accessibility/AXMenuList.cpp:92: AccessibilityExpanded expanded =
getExpandedAttr();
The ARIA spec says that ARIA shouldn't be allowed to override states that have
native semantics. So in this case, an AXMenuList is a native select element and
there's already a semantic meaning to whether it's expanded and collapsed, so
ARIA is not allowed to override it, that would only create confusion.

http://www.w3.org/TR/wai-aria-implementation/#mapping_conflicts

So basically I think this function should ignore the aria-expanded attribute and
just return whether the AXMenuList is really collapsed or expanded.

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
File Source/core/accessibility/AXNodeObject.h (right):

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
Source/core/accessibility/AXNodeObject.h:145: AccessibilityExpanded
getExpandedAttr() const;
I'd do the opposite here - since this only returns the ARIA attribute, I'd call
this ariaExpandedAttribute

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
File Source/core/accessibility/AXObject.h (right):

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
Source/core/accessibility/AXObject.h:436: virtual AccessibilityExpanded
ariaExpanded() const { return ExpandedUndefined; }
I think this should be called isExpanded or getExpanded, because it doesn't just
take ARIA into account, sometimes it uses native semantics too.

A function with "aria" in the name implies that it only returns ARIA properties.

[je_julie(Not used), 2014-10-29 13:07:27]

Dominic,
I updated patch to follow your comment. PTAL.

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
File Source/core/accessibility/AXMenuList.cpp (right):

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
Source/core/accessibility/AXMenuList.cpp:92: AccessibilityExpanded expanded =
getExpandedAttr();
Thanks for your comment. I updated it.
On 2014/10/29 07:46:59, dmazzoni wrote:
> The ARIA spec says that ARIA shouldn't be allowed to override states that have
> native semantics. So in this case, an AXMenuList is a native select element
and
> there's already a semantic meaning to whether it's expanded and collapsed, so
> ARIA is not allowed to override it, that would only create confusion.
> 
> http://www.w3.org/TR/wai-aria-implementation/#mapping_conflicts
> 
> So basically I think this function should ignore the aria-expanded attribute
and
> just return whether the AXMenuList is really collapsed or expanded.
>

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
File Source/core/accessibility/AXNodeObject.h (right):

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
Source/core/accessibility/AXNodeObject.h:145: AccessibilityExpanded
getExpandedAttr() const;
I updated API name with isExpanded.
On 2014/10/29 07:46:59, dmazzoni wrote:
> I'd do the opposite here - since this only returns the ARIA attribute, I'd
call
> this ariaExpandedAttribute

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
File Source/core/accessibility/AXObject.h (right):

https://codereview.chromium.org/679623002/diff/40001/Source/core/accessibilit...
Source/core/accessibility/AXObject.h:436: virtual AccessibilityExpanded
ariaExpanded() const { return ExpandedUndefined; }
I updated API name with isExpanded.
On 2014/10/29 07:46:59, dmazzoni wrote:
> I think this should be called isExpanded or getExpanded, because it doesn't
just
> take ARIA into account, sometimes it uses native semantics too.
> 
> A function with "aria" in the name implies that it only returns ARIA
properties.

[dmazzoni, 2014-10-29 15:20:52]

dmazzoni@chromium.org changed reviewers:
+ dglazkov@chromium.org
- mkwst@chromium.org

[dmazzoni, 2014-10-29 15:20:52]

lgtm

+dglazkov for OWNERS review

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

https://codereview.chromium.org/679623002/diff/60001/public/web/WebAXEnums.h#...
public/web/WebAXEnums.h:235: // values for aria-expanded attribute
Nit: comments should be a complete sentence - capitalize the first letter and
add a period.

[Mike West, 2014-10-29 15:25:04]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org

[Mike West, 2014-10-29 15:25:05]

Source/web and public/web LGTM % one big comment. Please add the compile assert
before landing this CL.

https://codereview.chromium.org/679623002/diff/60001/Source/web/AssertMatchin...
File Source/web/AssertMatchingEnums.cpp (left):

https://codereview.chromium.org/679623002/diff/60001/Source/web/AssertMatchin...
Source/web/AssertMatchingEnums.cpp:303:
COMPILE_ASSERT_MATCHING_ENUM(WebAXStateCollapsed, AXCollapsedState);
You should add compile asserts for the new enum you've added.

https://codereview.chromium.org/679623002/diff/60001/Source/web/WebAXObject.cpp
File Source/web/WebAXObject.cpp (right):

https://codereview.chromium.org/679623002/diff/60001/Source/web/WebAXObject.c...
Source/web/WebAXObject.cpp:282: return
static_cast<WebAXExpanded>(m_private->isExpanded());
Without compile asserts verifying that WebAXExpanded matches, this isn't a safe
cast.

[je_julie(Not used), 2014-10-30 03:27:19]

Hi Mike and Dominic,
I updated patchset adding compile assert.
Now, can I land it?

On 2014/10/29 15:25:05, Mike West (OOO until 3rd) wrote:
> Source/web and public/web LGTM % one big comment. Please add the compile
assert
> before landing this CL.

[dmazzoni, 2014-10-30 05:36:12]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2014-10-30 05:36:13]

lgtm

AssertMatchingEnums lgtm

[commit-bot: I haz the power, 2014-10-30 05:37:37]

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

[commit-bot: I haz the power, 2014-10-30 06:52:49]

Committed patchset #5 (id:80001) as 184621