aria-orientation is applied to more roles and each role defines own default value (horizontal, vertical, undefined) http://rawgit.com/w3c/aria/master/spec/aria.html#aria-orientation

Comment on attachment 8435844 [details] [diff] [review] patch If you want to do whitespace cleanup that's fine, but its a lot easier to read patches if you keep it seperate. > static void > MapEnumType(dom::Element* aElement, uint64_t* aState, const EnumTypeData& aData) > { I'd rather not allow this stuff to be more complicated, for one case I'd just handle it manually, and if another comes up we can add a new type of mapping function.

(In reply to Trevor Saunders (:tbsaunde) from comment #2) > If you want to do whitespace cleanup that's fine, but its a lot easier to > read patches if you keep it seperate. right, that was my editor > > static void > > MapEnumType(dom::Element* aElement, uint64_t* aState, const EnumTypeData& aData) > > { > > I'd rather not allow this stuff to be more complicated, for one case I'd > just handle it manually, and if another comes up we can add a new type of > mapping function. I don't follow that, you dislike new aClearState stuff?

(In reply to alexander :surkov from comment #3) > (In reply to Trevor Saunders (:tbsaunde) from comment #2) > > > If you want to do whitespace cleanup that's fine, but its a lot easier to > > read patches if you keep it seperate. > > right, that was my editor sure, but can you filter it out into a different patch? or just go through and clean it all up at once? > > > static void > > > MapEnumType(dom::Element* aElement, uint64_t* aState, const EnumTypeData& aData) > > > { > > > > I'd rather not allow this stuff to be more complicated, for one case I'd > > just handle it manually, and if another comes up we can add a new type of > > mapping function. > > I don't follow that, you dislike new aClearState stuff? yes

(In reply to Trevor Saunders (:tbsaunde) from comment #4) > > > I'd rather not allow this stuff to be more complicated, for one case I'd > > > just handle it manually, and if another comes up we can add a new type of > > > mapping function. > > > > I don't follow that, you dislike new aClearState stuff? > > yes well, I don't insist this is the best approach, what do you suggest?

(In reply to alexander :surkov from comment #5) > (In reply to Trevor Saunders (:tbsaunde) from comment #4) > > > > I'd rather not allow this stuff to be more complicated, for one case I'd > > > > just handle it manually, and if another comes up we can add a new type of > > > > mapping function. > > > > > > I don't follow that, you dislike new aClearState stuff? > > > > yes > > well, I don't insist this is the best approach, what do you suggest? I'd just handle it with its own code like now, but change the exact stuff it does.

(In reply to Trevor Saunders (:tbsaunde) from comment #6) > > well, I don't insist this is the best approach, what do you suggest? > > I'd just handle it with its own code like now, but change the exact stuff it > does. do you suggest to copy/paste MapEnumType into eARIAOrientation case or to have something like if (role == scrollbar) else if (role == tree) else if (role = ...)

(In reply to alexander :surkov from comment #7) > (In reply to Trevor Saunders (:tbsaunde) from comment #6) > > > > well, I don't insist this is the best approach, what do you suggest? > > > > I'd just handle it with its own code like now, but change the exact stuff it > > does. > > do you suggest to copy/paste MapEnumType into eARIAOrientation case or to > have something like > if (role == scrollbar) > else if (role == tree) > else if (role = ...) I was think copy paste and then specialize at which point it isn't that similar. Its not great, but simpler seems better.

I'm not sure it's get nicer than the proposed approach, what about to have special EnumTypeData constructor for eARIAOrientation to keep eARIABusy and eARIAAutocomplete unchanged? That's the point, right?

(In reply to alexander :surkov from comment #9) > I'm not sure it's get nicer than the proposed approach, what about to have > special EnumTypeData constructor for eARIAOrientation to keep eARIABusy and > eARIAAutocomplete unchanged? That's the point, right? no, I want to keep MapEnumType() simple.

It is simple and I'd say clearing state is natural part of setting states in case if those states are mutually exclusive. Is it major concern for you? I would go with current approach.

(In reply to alexander :surkov from comment #11) > It is simple and I'd say clearing state is natural part of setting states in It seems to me it now does two only sort of related things, and it has more conditional data you need to know to understand what it does. > case if those states are mutually exclusive. Is it major concern for you? I > would go with current approach. I'd rather not, could probably be convinced to not care.

Comment on attachment 8435844 [details] [diff] [review] patch Review of attachment 8435844 [details] [diff] [review]: ----------------------------------------------------------------- then let's ask Neil for his opinion, maybe he has ideas how to make eARIAOrientation nice.

So the problem here is that previously attributes have only added states but now they might need to remove states?

yes, I introduced default role-based states that are unconditionally applied but if the attribute is presented then it needs to override the default state.

Neil, any chance to take a look please?

(Sorry, this has been a bit of a slow month for me.) OK, so you have three attributes where this applies: aria_autocomplete: Adds SUPPORTS_AUTOCOMPLETION if "inlinevalue", also adds HASPOPUP if "list" or "both". aria_busy: Adds BUSY if "true" or INVALID if "error". aria_orientation: Set to HORIZONTAL if "horizontal" or VERTICAL if "vertical". Can these flags be set for other reasons or is this the only place where they can be set e.g. can you have an element with aria_busy="error" that has the BUSY flag for other reasons?

the state comes either from native markup or ARIA or both. Native markup should win in semantic collision (like aria-readonly on editable input). aria-busy doesn't get into conflict with native state (loading document exposes BUSY state and aria-busy="false" doesn't have any weight) and from what I can tell aria-autocomplete doesn't conflict as well. But aria-orientation I think should override native state (and of course default state coming from ARIA role).

(In reply to alexander surkov from comment #18) > aria-busy doesn't get into conflict with native state (loading document > exposes BUSY state and aria-busy="false" doesn't have any weight) But what about aria-busy="error" on a loading document?

(In reply to neil@parkwaycc.co.uk from comment #19) > But what about aria-busy="error" on a loading document? it adds INVALID state whose presence should be ok in conjunction with BUSY state in general (exposed during document loading). On the other hand this may be considered confusing since "true" and "error" values of aria-busy are mutually exclusive but either case I wouldn't worry to ignore aria-busy on loading documents until we reported it's a problem.

(In reply to alexander surkov from comment #20) > (In reply to comment #19) > > But what about aria-busy="error" on a loading document? > > it adds INVALID state whose presence should be ok in conjunction with BUSY > state in general (exposed during document loading). On the other hand this > may be considered confusing since "true" and "error" values of aria-busy are > mutually exclusive but either case I wouldn't worry to ignore aria-busy on > loading documents until we reported it's a problem. OK, so it would be wrong to report just the INVALID state in this case?

(In reply to neil@parkwaycc.co.uk from comment #21) > OK, so it would be wrong to report just the INVALID state in this case? I don't think spec address that and what's more important there's no use case for that so that should be ok but I would try to not change behavior. Do you have nice solution if it's done?

Well, this is my possible idea: static void MapEnumType(dom::Element* aElement, uint64_t* aState, const EnumTypeData& aData) { switch (aElement->FindAttrValueIn(kNameSpaceID_None, aData.mAttrName, &aData.mValue1, eCaseMatters)) { case 0: *aState = (*aState & ~(aData.mState2 | aData.mState3)) | aData.mState1; return; case 1: *aState = (*aState & ~(aData.mState1 | aData.mState3)) | aData.mState2; return; case 2: *aState = (*aState & ~(aData.mState1 | aData.mState2)) | aData.mState3; return; } }

on a second glance that'd be rather a problem I think since it overrides native state. Similar situation I get for <input aria-autocomplete="inline"> which exposes haspopup natively and that shouldn't be overridden too. So if not this one then what other approach is preferred by you?

Comment on attachment 8435844 [details] [diff] [review] patch > struct EnumTypeData > { >- EnumTypeData(nsIAtom* aAttrName, >+ EnumTypeData(nsIAtom* aAttrName, uint64_t aClearState, > nsIAtom** aValue1, uint64_t aState1, > nsIAtom** aValue2, uint64_t aState2, > nsIAtom** aValue3 = 0, uint64_t aState3 = 0) : Nit: use MOZ_CONSTEXPR to avoid the static initialiser on Mac/Linux. I tested on Windows and the optimiser is clever enough to avoid the static initialiser if you make this a global variable rather than scoped to the function. The only way I could get it to optimise away the static initialiser when used as a local variable was to remove the constructor completely like this: struct EnumTypeData { nsIAtom* mAttrName; nsIAtom* mValues[3]; nsIAtom* mStates[3]; uint64_t mClearState; }; static const EnumTypeData data = { nsGkAtoms::aria_autocomplete, { &nsGkAtoms::inlinevalue, &nsGkAtoms::list, &nsGkAtoms::both }. { states::SUPPORTS_AUTOCOMPLETION, states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION, states::HASPOPUP | states::SUPPORTS_AUTOCOMPLETION } }; static const EnumTypeData data = { nsGkAtoms::aria_orientation, { &nsGkAtoms::horizontal, &nsGkAtoms::vertical }, { states::HORIZONTAL, states::VERTICAL }, states::HORIZONTAL | states::VERTICAL, }; (Not relevant here, but the stupid MSVC optimiser didn't know that data = { 1, { 0 } }; is the same as data = { 1 };)

Comment on attachment 8435844 [details] [diff] [review] patch > > // Role mapping rule: maps to this nsIAccessibleRole > mozilla::a11y::role role; >- >+ extra whitespace cleanup is seriously annoying > const uint64_t mState2; > const uint64_t mState3; >- >- // Default state if no one enum value is matched. >- const uint64_t mDefaultState; >+ const uint64_t mClearState; comment what its for > &aData.mValue1, eCaseMatters)) { > case 0: >- *aState |= aData.mState1; >+ *aState = (*aState & ~aData.mClearState) | aData.mState1; I'd still really rather you just used custom logic for orientation, but whatever its aria so it'll be awful no matter what and I don't care enough to argue more.

(In reply to neil@parkwaycc.co.uk from comment #25) > Comment on attachment 8435844 [details] [diff] [review] > patch > > > struct EnumTypeData > > { > >- EnumTypeData(nsIAtom* aAttrName, > >+ EnumTypeData(nsIAtom* aAttrName, uint64_t aClearState, > > nsIAtom** aValue1, uint64_t aState1, > > nsIAtom** aValue2, uint64_t aState2, > > nsIAtom** aValue3 = 0, uint64_t aState3 = 0) : > Nit: use MOZ_CONSTEXPR to avoid the static initialiser on Mac/Linux. > I tested on Windows and the optimiser is clever enough to avoid the static > initialiser if you make this a global variable rather than scoped to the if you make these global then you get relocations, and I guess you need to do something else about the nsIAtom* mName since nsGkAtoms::whatever only starts pointing at the atom after startup. the initialization is a little silly certainly, but its probably not too much worse than the relocations, and the common case will be this code is never used. Why exactly gcc feels the need to do all of the initialization at runtime instead of embedding some of it in .data is unclear to me, but whatever we're already going to be doing moves to those cache lines. > function. The only way I could get it to optimise away the static > initialiser when used as a local variable was to remove the constructor > completely like this: how does it deal with mAttrName then since it needs to get the value of nsGkAtoms::whatever?