ARIA state/property(aria-relevant,aria-busy) are not exposed correctly on MAC.

ARIA state/property(aria-relevant,aria-busy) are not exposed correctly on MAC.

According to latest ediotr's draft http://www.w3.org/WAI/PF/aria-implementation/

aria-busy is exposed wrongly on windows and mac.
On windows it is exposed with busy and container-busy attributes, rather according to specs it should be exposed as STATE_SYSTEM_BUSY state.
And on mac it should be exposed as AXARIABusy.

aria-relevant should be exposed as AXARIARelevant: <value> on mac.

BUG=423630, 428377
Committed: https://crrev.com/8a9a8a55e4a1e3ae95cb5d1511fd1b816f14ac46
Cr-Commit-Position: refs/heads/master@{#304860}

[shreeramk, 2014-11-07 15:28:04]

shreeram.k@samsung.com changed reviewers:
+ dmazzoni@chromium.org

[shreeramk, 2014-11-07 15:28:05]

Please review. 
Thanks!

[shreeramk, 2014-11-07 16:38:03]

shreeram.k@samsung.com changed reviewers:
+ aboxhall@chromium.org

[dmazzoni, 2014-11-07 21:28:43]

https://codereview.chromium.org/692643005/diff/60001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/692643005/diff/60001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_cocoa.mm:1313: if
(browserAccessibility_->HasStringAttribute(
I'm not sure this change is needed. aria-relevant is meaningless on an element
that doesn't already have aria-live set.

https://codereview.chromium.org/692643005/diff/60001/content/test/data/access...
File content/test/data/accessibility/aria-relevant.html (right):

https://codereview.chromium.org/692643005/diff/60001/content/test/data/access...
content/test/data/accessibility/aria-relevant.html:13: <div tabindex=4
role="log" aria-relevant="additions-text"></div>
This is space-delimited, it should be "additions text"

[shreeramk, 2014-11-08 09:21:34]

https://codereview.chromium.org/692643005/diff/60001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/692643005/diff/60001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_cocoa.mm:1313: if
(browserAccessibility_->HasStringAttribute(
On 2014/11/07 21:28:43, dmazzoni wrote:
> I'm not sure this change is needed. aria-relevant is meaningless on an element
> that doesn't already have aria-live set.

But for ARIA role log, Specs says:
Note: Elements with the role log have an implicit aria-live value of polite.

And so, on windows without aria-live property, relevant and container-relevants
are exposed.

Do you mean to say, AXARIArelevant/relevant/container-relevant should
automatically be exposed if aria-live attribute is present and aria-relevant is
not present?(Please check aria-live.html and aria-live-expected-win.txt -
without aria-relevant, container-relevant and relevant are exposed.)

Or, expose AXARIArelevant/relevant/container-relevant only if aria-live and
aria-relevant both are present?

[dmazzoni, 2014-11-11 06:44:02]

https://codereview.chromium.org/692643005/diff/60001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/692643005/diff/60001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_cocoa.mm:1313: if
(browserAccessibility_->HasStringAttribute(
On 2014/11/08 09:21:34, shreeramk wrote:
> On 2014/11/07 21:28:43, dmazzoni wrote:
> > I'm not sure this change is needed. aria-relevant is meaningless on an
element
> > that doesn't already have aria-live set.
> 
> But for ARIA role log, Specs says:
> Note: Elements with the role log have an implicit aria-live value of polite.
> 
> And so, on windows without aria-live property, relevant and
container-relevants
> are exposed.

That's correct. But we handle this in Blink, so if something has a role of log,
then ui::AX_ATTR_LIVE_STATUS should be set to "polite". If that's not what
you're getting
then we should debug that in Blink.

What I'm saying is that there should never be a case where we have aria-relevant
but we don't also have aria-live. Even if aria-live isn't set as an HTML
attribute, it should be set by Blink due to the role, so by the time we get to
browser_accessibility_cocoa.mm we should have a "live" status.

[shreeramk, 2014-11-19 03:45:36]

I think this change might be required.

https://codereview.chromium.org/692643005/diff/60001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_cocoa.mm (right):

https://codereview.chromium.org/692643005/diff/60001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_cocoa.mm:1313: if
(browserAccessibility_->HasStringAttribute(
On 2014/11/11 06:44:02, dmazzoni wrote:
> On 2014/11/08 09:21:34, shreeramk wrote:
> > On 2014/11/07 21:28:43, dmazzoni wrote:
> > > I'm not sure this change is needed. aria-relevant is meaningless on an
> element
> > > that doesn't already have aria-live set.
> > 
> > But for ARIA role log, Specs says:
> > Note: Elements with the role log have an implicit aria-live value of polite.
> > 
> > And so, on windows without aria-live property, relevant and
> container-relevants
> > are exposed.
> 
> That's correct. But we handle this in Blink, so if something has a role of
log,
> then ui::AX_ATTR_LIVE_STATUS should be set to "polite". If that's not what
> you're getting
> then we should debug that in Blink.
> 
> What I'm saying is that there should never be a case where we have
aria-relevant
> but we don't also have aria-live. Even if aria-live isn't set as an HTML
> attribute, it should be set by Blink due to the role, so by the time we get to
> browser_accessibility_cocoa.mm we should have a "live" status.

Yes correct its handled in blink.

I was looking into the code and found out this, (blink_ax_tree_source.cc) - from
which we can conclude that if its in live region then following attributes are
set for respective states/property(live, relevant... etc.)

if (src.isInLiveRegion()) {
    dst->AddBoolAttribute(ui::AX_ATTR_LIVE_ATOMIC, src.liveRegionAtomic());
    .
    .
    .
    dst->AddStringAttribute(ui::AX_ATTR_LIVE_RELEVANT,
                            UTF16ToUTF8(src.liveRegionRelevant()));
    dst->AddBoolAttribute(ui::AX_ATTR_CONTAINER_LIVE_ATOMIC,
                          src.containerLiveRegionAtomic());
    dst->AddStringAttribute(ui::AX_ATTR_CONTAINER_LIVE_STATUS,
                            UTF16ToUTF8(src.containerLiveRegionStatus()));
    dst->AddStringAttribute(ui::AX_ATTR_CONTAINER_LIVE_RELEVANT,
                            UTF16ToUTF8(src.containerLiveRegionRelevant()));
  }

Hence I think we should use this ui::* checks in browser_accessibility_*.cc/mm
files. 

This ui::* attributes check has been used correctly on windows, hence there
those attributes are exposed correctly, 

browser_accessibility_win.cc 

// Expose live region attributes.
  StringAttributeToIA2(ui::AX_ATTR_LIVE_STATUS, "live");
  StringAttributeToIA2(ui::AX_ATTR_LIVE_RELEVANT, "relevant");
  BoolAttributeToIA2(ui::AX_ATTR_LIVE_ATOMIC, "atomic");
  BoolAttributeToIA2(ui::AX_ATTR_LIVE_BUSY, "busy");

  // Expose container live region attributes.
  StringAttributeToIA2(ui::AX_ATTR_CONTAINER_LIVE_STATUS,
                       "container-live");
  StringAttributeToIA2(ui::AX_ATTR_CONTAINER_LIVE_RELEVANT,
                       "container-relevant");
  BoolAttributeToIA2(ui::AX_ATTR_CONTAINER_LIVE_ATOMIC,
                     "container-atomic");
  BoolAttributeToIA2(ui::AX_ATTR_CONTAINER_LIVE_BUSY,
                     "container-busy");


But on mac it has not been exposed properly due to wrong checks.

[dmazzoni, 2014-11-19 17:17:28]

I'm pretty sure Firefox exposes the 'busy' and 'container-busy' IA2 attributes.

It's good to set the BUSY state also, but don't delete the attributes because
existing screen readers might be depending on those.

https://codereview.chromium.org/692643005/diff/200001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_manager_win.cc (left):

https://codereview.chromium.org/692643005/diff/200001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_manager_win.cc:199: if
(node->GetBoolAttribute(ui::AX_ATTR_CONTAINER_LIVE_BUSY))
I don't think you should delete this.

We don't want to fire a "live region changed" event on a live region that's
busy.

https://codereview.chromium.org/692643005/diff/200001/content/renderer/access...
File content/renderer/accessibility/blink_ax_tree_source.cc (left):

https://codereview.chromium.org/692643005/diff/200001/content/renderer/access...
content/renderer/accessibility/blink_ax_tree_source.cc:381:
dst->AddBoolAttribute(ui::AX_ATTR_CONTAINER_LIVE_BUSY,
Why are you deleting this?

AX_STATE_BUSY is not the same as CONTAINER_LIVE_BUSY.

[shreeramk, 2014-11-19 18:18:13]

On 2014/11/19 17:17:28, dmazzoni wrote:
> I'm pretty sure Firefox exposes the 'busy' and 'container-busy' IA2
attributes.
> 
> It's good to set the BUSY state also, but don't delete the attributes because
> existing screen readers might be depending on those.

PTAL. Comments incorporated.

Thanks!!

[dmazzoni, 2014-11-19 18:26:57]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2014-11-19 18:26:57]

lgtm

Thanks!

[commit-bot: I haz the power, 2014-11-19 18:28:11]

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

[commit-bot: I haz the power, 2014-11-19 19:26:33]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-11-19 19:27:10]

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