762876 - IA2_STATE_HORIZONTAL state is set for “aria-orientation=vertical”

Status: RESOLVED FIXED

Description

[Jost Eckhardt, Ai Squared]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Build ID: 20120531155942

Steps to reproduce:

Run a test case with AccProbe

http://www.aisquared.com/testdocs/Aria/AriaInvalid.html 


Actual results:

aria-invalid is not exposed as text attribute


Expected results:

aria-invalid should be exposed as text attribute

Comment 1

[Jost Eckhardt, Ai Squared]

Sorry, I got it mixed up. The aria-invalid is an existing issue: 445510

this is about aria-orientation

aria-orientation should be mapped to IA2_STATE_HORIZONTAL

The test case is: http://www.aisquared.com/testdocs/Aria/AriaOrientation.html

Comment 2

[alexander :surkov]

(In reply to Jost Eckhardt, Ai Squared from comment #1)

> aria-orientation should be mapped to IA2_STATE_HORIZONTAL
> 
> The test case is: http://www.aisquared.com/testdocs/Aria/AriaOrientation.html

I see that aria-orientation="vertical" are mapped to vertical and horizontal states which is a bug. Bug as far as I can see by accprobe aria-orientation="horizontal" is mapped to horizontal state. Can you give me exact steps to reproduce please?

Comment 3

[Alex Kulikov, Ai Squared]

Accordingly to the spec(http://www.w3.org/TR/wai-aria-implementation/#mapping_state-property_table) user agents must set IA2_STATE_HORIZONTAL attribute if aria-orientation="horizontal" and clear it if aria-orientation="vertical" (aria-orientation="vertical" is not mapped to the IA2_STATE_VERTICAL). 
Steps to repro:
a)Test case #1 (http://www.aisquared.com/testdocs/Aria/AriaOrientation.html) presents three items that have vertical orientation (aria-orientation="vertical"), but AccProbe reports the following states: [vertical, ...., horizontal];
b)Test case #3  (http://www.aisquared.com/testdocs/Aria/AriaOrientation.html) presents an ARIA scrollbar that have no aria-orientation attribute defined. AccProbe reports that the orientation of the item is set to horizontal (IA2_STATE_HORIZONTAL is set), but, accordingly to the spec., aria-orientation="vertical" is a default orientation for an aria scroll bar.

Comment 4

[alexander :surkov]

Thanks, Alex, that's what I observe. Can you fix bug summary then?

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Looks like a regression from bug 741398 but I don't know how our mochitest passes.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #2)
> I see that aria-orientation="vertical" are mapped to vertical and horizontal
> states

Aha.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

I'm a little nervous that we rely on memory alignment for the mValues (when calling FindAttrValueIn in our state map code).

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #7)
> I'm a little nervous that we rely on memory alignment for the mValues (when
> calling FindAttrValueIn in our state map code).

why? I'm pretty sure the spec says that's legit.

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Trevor can you take this one?

Comment 10

[Trevor Saunders (:tbsaunde)]

Created attachment 642972 [details] [diff] [review]
bug 762876 - patch

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 642972 [details] [diff] [review]
bug 762876 - patch

Review of attachment 642972 [details] [diff] [review]:
-----------------------------------------------------------------

looks right.

Comment 12

[Trevor Saunders (:tbsaunde)]

Created attachment 644487 [details] [diff] [review]
fix some tests

these tests were cheked in doing this with no reason given.  It doesn't seem terribly useful to say links are "horizontal", so I'm proposing we just change them.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 644487 [details] [diff] [review]
fix some tests

Review of attachment 644487 [details] [diff] [review]:
-----------------------------------------------------------------

If removing this state from links is wrong, I don't want to be right. r=me.

Comment 14

[alexander :surkov]

Comment on attachment 642972 [details] [diff] [review]
bug 762876 - patch

Review of attachment 642972 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/base/ARIAStateMap.cpp
@@ +235,5 @@
> +      } else {
> +        NS_ASSERTION(!(*aState & (states::HORIZONTAL | states::VERTICAL)),
> +                     "orientation state on role with default aria-orientation!");
> +        *aState |= GetRoleMap(aElement)->Is(nsGkAtoms::scrollbar) ?
> +          states::VERTICAL : states::HORIZONTAL;

it should be part of map

::: accessible/src/generic/Accessible.cpp
@@ +678,4 @@
>      if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::popup))
>        state |= states::HASPOPUP;
>  
> +    const nsStyleXUL *xulStyle = frame->GetStyleXUL();

type* name

@@ +678,5 @@
>      if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::popup))
>        state |= states::HASPOPUP;
>  
> +    const nsStyleXUL *xulStyle = frame->GetStyleXUL();
> +    if (xulStyle && frame->IsBoxFrame()) {

please an example of XUL element which is not box frame

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #14)
> Comment on attachment 642972 [details] [diff] [review]
> bug 762876 - patch
> 
> Review of attachment 642972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/base/ARIAStateMap.cpp
> @@ +235,5 @@
> > +      } else {
> > +        NS_ASSERTION(!(*aState & (states::HORIZONTAL | states::VERTICAL)),
> > +                     "orientation state on role with default aria-orientation!");
> > +        *aState |= GetRoleMap(aElement)->Is(nsGkAtoms::scrollbar) ?
> > +          states::VERTICAL : states::HORIZONTAL;
> 
> it should be part of map

I'm not sure what you mean all this function has access to is the eStateRule, perhaps we should pase more, but currently we don't

> ::: accessible/src/generic/Accessible.cpp
> @@ +678,4 @@
> >      if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::popup))
> >        state |= states::HASPOPUP;
> >  
> > +    const nsStyleXUL *xulStyle = frame->GetStyleXUL();
> 
> type* name

arg, I hate that I don't notice those when I just copy stuff around.
 
> @@ +678,5 @@
> >      if (mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::popup))
> >        state |= states::HASPOPUP;
> >  
> > +    const nsStyleXUL *xulStyle = frame->GetStyleXUL();
> > +    if (xulStyle && frame->IsBoxFrame()) {
> 
> please an example of XUL element which is not box frame

no idea, I think it was because bz said just IsXUL() was wrong and suggested that.

Comment 16

[alexander :surkov]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)

> > it should be part of map
> 
> I'm not sure what you mean all this function has access to is the
> eStateRule, perhaps we should pase more, but currently we don't

each entry has an option of default state, see http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsARIAMap.cpp#413 

> > type* name
> 
> arg, I hate that I don't notice those when I just copy stuff around.

if you do follow up then please fix it as well.

> > please an example of XUL element which is not box frame
> 
> no idea, I think it was because bz said just IsXUL() was wrong and suggested
> that.

in either way you put it under IsXUL() what could make bz comment irrelevant (in case if IsBoxFrame() returns true for some HTML).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc3835f235cb

Comment 18

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> 
> > > it should be part of map
> > 
> > I'm not sure what you mean all this function has access to is the
> > eStateRule, perhaps we should pase more, but currently we don't
> 
> each entry has an option of default state, see
> http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsARIAMap.
> cpp#413

true, but that doesn't quiet work because at some point you need to clear the default state if the non default state is set explicitly.

> > > please an example of XUL element which is not box frame

nsXULLabelFrame?

> > no idea, I think it was because bz said just IsXUL() was wrong and suggested
> > that.
> 
> in either way you put it under IsXUL() what could make bz comment irrelevant
> (in case if IsBoxFrame() returns true for some HTML).

I'm not sure

Comment 19

[alexander :surkov]

(In reply to Trevor Saunders (:tbsaunde) from comment #18)
> (In reply to alexander :surkov from comment #16)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > 
> > > > it should be part of map
> > > 
> > > I'm not sure what you mean all this function has access to is the
> > > eStateRule, perhaps we should pase more, but currently we don't
> > 
> > each entry has an option of default state, see
> > http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/nsARIAMap.
> > cpp#413
> 
> true, but that doesn't quiet work because at some point you need to clear
> the default state if the non default state is set explicitly.

I meant, stuffs like GetRoleMap(aElement)->Is(nsGkAtoms::scrollbar) should be replaced by default state.