Last Comment Bug 762876 - IA2_STATE_HORIZONTAL state is set for “aria-orientation=vertical”
: IA2_STATE_HORIZONTAL state is set for “aria-orientation=vertical”
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: 13 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Trevor Saunders (:tbsaunde)
:
: alexander :surkov
Mentors:
Depends on: 741398 776472
Blocks: aria
  Show dependency treegraph
 
Reported: 2012-06-08 06:01 PDT by Jost Eckhardt, Ai Squared
Modified: 2012-08-21 05:35 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
bug 762876 - patch (7.35 KB, patch)
2012-07-17 08:48 PDT, Trevor Saunders (:tbsaunde)
dbolter: review+
Details | Diff | Splinter Review
fix some tests (10.32 KB, patch)
2012-07-20 14:55 PDT, Trevor Saunders (:tbsaunde)
dbolter: review+
Details | Diff | Splinter Review

Description Jost Eckhardt, Ai Squared 2012-06-08 06:01:45 PDT
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 2012-06-08 08:27:55 PDT
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 2012-06-08 09:11:34 PDT
(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 2012-06-11 07:01:49 PDT
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 2012-06-11 07:31:07 PDT
Thanks, Alex, that's what I observe. Can you fix bug summary then?
Comment 5 David Bolter [:davidb] 2012-06-12 06:25:51 PDT
Looks like a regression from bug 741398 but I don't know how our mochitest passes.
Comment 6 David Bolter [:davidb] 2012-06-12 06:29:46 PDT
(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] 2012-06-12 07:52:24 PDT
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) 2012-06-12 08:22:27 PDT
(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] 2012-07-16 07:27:31 PDT
Trevor can you take this one?
Comment 10 Trevor Saunders (:tbsaunde) 2012-07-17 08:48:18 PDT
Created attachment 642972 [details] [diff] [review]
bug 762876 - patch
Comment 11 David Bolter [:davidb] 2012-07-17 09:05:28 PDT
Comment on attachment 642972 [details] [diff] [review]
bug 762876 - patch

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

looks right.
Comment 12 Trevor Saunders (:tbsaunde) 2012-07-20 14:55:23 PDT
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] 2012-07-20 17:27:34 PDT
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 2012-07-20 18:17:06 PDT
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) 2012-07-20 18:43:30 PDT
(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 2012-07-20 18:49:14 PDT
(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] 2012-07-21 06:21:36 PDT
https://hg.mozilla.org/mozilla-central/rev/dc3835f235cb
Comment 18 Trevor Saunders (:tbsaunde) 2012-07-21 06:57:46 PDT
(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 2012-08-21 05:35:20 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.