When aria_checked/aria_pressed are changed then we don't know if their previous value was mixed and therefore we can't fire right state change event. Currently we use a hack (and support the one case only) - we fire state change event for focused accessible only.

Cache interesting ARIA attribute value in AttributeWillChange to use it in AttributeChanged. Also this patch contains two unrelated fixes (to make mochitests pass): 1) do not coalesce state change events initialized by DOM nodes (let bug 569356 to deal with correct fix later) 2) fix busy state presence flag in state change event (wrong logic from bug 648133) I hope that's ok.

> 2) fix busy state presence flag in state change event (wrong logic from bug > 648133) so, were the tests in that bug just plain wrong, which seems odd since iirc Marco tested it too, or is this a issue braught up by not coalescing state change events > I hope that's ok. yeah, didn't cause too much of an issue, and generally looks reasonable module some nity stuff

(In reply to Trevor Saunders (:tbsaunde) from comment #2) > > 2) fix busy state presence flag in state change event (wrong logic from bug > > 648133) > > so, were the tests in that bug just plain wrong, which seems odd they weren't wrong, they were incomplete > since iirc > Marco tested it too, or is this a issue braught up by not coalescing state > change events I don't think so. Maybe Marco's testing were similar (in terms of testing subject) to mochitests, for example, that happens in tests by Windows screen reader (when event is handled then state is queried from accessible target). > > I hope that's ok. > > yeah, didn't cause too much of an issue, and generally looks reasonable > module some nity stuff

Comment on attachment 552158 [details] [diff] [review] patch >+nsIAtom* >+nsAccUtils::GetARIAToken(dom::Element* aElement, nsIAtom* aAttr) >+{ >+ if (!nsAccUtils::HasDefinedARIAToken(aElement, aAttr)) >+ return nsAccessibilityAtoms::_empty; >+ >+ if (aElement->AttrValueIs(kNameSpaceID_None, aAttr, >+ nsAccessibilityAtoms::_false, eCaseMatters)) >+ return nsAccessibilityAtoms::_false; >+ >+ if (aElement->AttrValueIs(kNameSpaceID_None, aAttr, >+ nsAccessibilityAtoms::_true, eCaseMatters)) >+ return nsAccessibilityAtoms::_true; >+ >+ if (aElement->AttrValueIs(kNameSpaceID_None, aAttr, >+ nsAccessibilityAtoms::mixed, eCaseMatters)) >+ return nsAccessibilityAtoms::mixed; >+ >+ return nsnull; >+} it seems like FindAttrValue() might work nicely here did you consider that? >+++ b/accessible/src/base/nsAccUtils.h >@@ -45,16 +45,17 @@ >+#include "mozilla/dom/Element.h" why not forward declare mozilla::dom::Element here, and include the header in nsAccUtils.cpp? this reduces the places we include it where its not needed, or directly included. I believe the style guide also suggests this. > ACCESSIBILITY_ATOM(menu, "menu") > ACCESSIBILITY_ATOM(menuButton, "menu-button") > ACCESSIBILITY_ATOM(multiple, "multiple") >+ACCESSIBILITY_ATOM(mixed, "mixed") any reason not to make this a nsGkAtom now, so I won't have tom ove it soon (my plan is after the parts of bug 648265 that might conflict have landed) >+ nsAccessible* accessible = GetAccessible(aElement); >+ if (!accessible) { this is incase of some sort of inaccessible content within the document right? >+ if (aElement != mContent) >+ return; >+ >+ accessible = this; should this logic maybe be put in GetAccessible()? >+ bool wasMixed = (mARIAAttrOldValue == nsAccessibilityAtoms::mixed); >+ bool isMixed = aContent->AttrValueIs(kNameSpaceID_None, aAttribute, >+ nsAccessibilityAtoms::mixed, eCaseMatters); >+ if (isMixed != wasMixed) { you could get rid of wasMixed, but I'm not sure if that would be nicer. >+ //gA11yEventDumpID = "eventdump"; // debugging >+ //gA11yEventDumpToConsole = true; I think I like having // debugging on its own line more, but don't really care. > > gQueue.push(new expandNode("section", true)); > gQueue.push(new expandNode("section", false)); > gQueue.push(new expandNode("div", true)); > gQueue.push(new expandNode("div", false)); > > gQueue.push(new busyify("aria_doc", true)); > gQueue.push(new busyify("aria_doc", false)); >+ >+ gQueue.push(new setPressed("pressable", "mixed")); >+ gQueue.push(new setPressed("pressable", "")); >+ gQueue.push(new setPressed("pressable", "true")); >+ gQueue.push(new setPressed("pressable", "false")); >+ gQueue.push(new setPressed("pressable", "mixed")); >+ gQueue.push(new setPressed("pressable", "undefined")); what about true -> mixed, and mixed-> false? >+ >+ gQueue.push(new setChecked("checkable", "mixed")); >+ gQueue.push(new setChecked("checkable", "")); >+ gQueue.push(new setChecked("checkable", "true")); >+ gQueue.push(new setChecked("checkable", "false")); >+ gQueue.push(new setChecked("checkable", "mixed")); >+ gQueue.push(new setChecked("checkable", "undefined")); same >+const kOrdinalState = 0; ordinal seems a little weird / undescriptive, how about zero state or empty set?

(In reply to Trevor Saunders (:tbsaunde) from comment #4) > >+nsIAtom* > >+nsAccUtils::GetARIAToken(dom::Element* aElement, nsIAtom* aAttr) > > it seems like FindAttrValue() might work nicely here did you consider that? good catch > > >+++ b/accessible/src/base/nsAccUtils.h > >@@ -45,16 +45,17 @@ > > >+#include "mozilla/dom/Element.h" > > why not forward declare mozilla::dom::Element here, and include the header > in nsAccUtils.cpp? this reduces the places we include it where its not > needed, or directly included. I believe the style guide also suggests this. Element.h is wide used file across cpp files, nsAccUtils.cpp is included into cpp too, I don't think I would like to end up including hundreds of files into each cpp file. Is it really advised by style guide? > any reason not to make this a nsGkAtom now, so I won't have tom ove it soon > (my plan is after the parts of bug 648265 that might conflict have landed) I'd need to take some parts of that your patch, at least includes, also that makes us to use our atoms and gkatoms the same time what confusing and you need to merge that patch anyway. > >+ nsAccessible* accessible = GetAccessible(aElement); > >+ if (!accessible) { > > this is incase of some sort of inaccessible content within the document > right? yes > >+ if (aElement != mContent) > >+ return; > >+ > >+ accessible = this; > > should this logic maybe be put in GetAccessible()? I wouldn't do this at this point since this may be unpredictable. > >+ bool wasMixed = (mARIAAttrOldValue == nsAccessibilityAtoms::mixed); > >+ bool isMixed = aContent->AttrValueIs(kNameSpaceID_None, aAttribute, > >+ nsAccessibilityAtoms::mixed, eCaseMatters); > >+ if (isMixed != wasMixed) { > > you could get rid of wasMixed, but I'm not sure if that would be nicer. yep, I like wasMixed since it's more readable > I think I like having // debugging on its own line more, but don't really > care. fine with me > >+ gQueue.push(new setPressed("pressable", "mixed")); > >+ gQueue.push(new setPressed("pressable", "")); > >+ gQueue.push(new setPressed("pressable", "true")); > >+ gQueue.push(new setPressed("pressable", "false")); > >+ gQueue.push(new setPressed("pressable", "mixed")); > >+ gQueue.push(new setPressed("pressable", "undefined")); > > what about true -> mixed, and mixed-> false? doesn't make sense, right? no extra code logic paths. > >+const kOrdinalState = 0; > > ordinal seems a little weird / undescriptive, how about zero state or > empty set? We have two sets of constants: states and extra states. I didn't understand what is zero state and empty set. So what is your suggestion?

> > > > >+++ b/accessible/src/base/nsAccUtils.h > > >@@ -45,16 +45,17 @@ > > > > >+#include "mozilla/dom/Element.h" > > > > why not forward declare mozilla::dom::Element here, and include the header > > in nsAccUtils.cpp? this reduces the places we include it where its not > > needed, or directly included. I believe the style guide also suggests this. > > Element.h is wide used file across cpp files, nsAccUtils.cpp is included > into cpp too, I don't think I would like to end up including hundreds of > files into each cpp file. Is it really advised by style guide? the last time I looked yes, I also think its kind of ugly to bootleg include Element.h by includeing nsAccUtils.h, but if you really prefer I ues I can live with it. > > > any reason not to make this a nsGkAtom now, so I won't have tom ove it soon > > (my plan is after the parts of bug 648265 that might conflict have landed) > > I'd need to take some parts of that your patch, at least includes, also that > makes us to use our atoms and gkatoms the same time what confusing and you > need to merge that patch anyway. fair enough > > what about true -> mixed, and mixed-> false? > > doesn't make sense, right? no extra code logic paths. well, I tend to think we should have tests covering all reasonable cases, even though perhaps given the current implementation going one direction is the same as the path for the other. > > >+const kOrdinalState = 0; > > > > ordinal seems a little weird / undescriptive, how about zero state or > > empty set? > > We have two sets of constants: states and extra states. I didn't understand > what is zero state and empty set. So what is your suggestion? I was a little unclear what you were doing with kOrdinalState and kExtraState, perhaps kNonExtraState or just kState vs kExtraState works a little better, I don't have a great idea here, so leave it as is if you like

(In reply to Trevor Saunders (:tbsaunde) from comment #6) > > Element.h is wide used file across cpp files, nsAccUtils.cpp is included > > into cpp too, I don't think I would like to end up including hundreds of > > files into each cpp file. Is it really advised by style guide? > > the last time I looked yes, I also think its kind of ugly to bootleg > include Element.h by includeing nsAccUtils.h, but if you really prefer I ues > I can live with it. let's leave it as is for now > > > what about true -> mixed, and mixed-> false? > > > > doesn't make sense, right? no extra code logic paths. > > well, I tend to think we should have tests covering all reasonable cases, > even though perhaps given the current implementation going one direction is > the same as the path for the other. ok > I was a little unclear what you were doing with kOrdinalState and > kExtraState, perhaps kNonExtraState or just kState vs kExtraState works a > little better, I don't have a great idea here, so leave it as is if you like kState sounds unclear, kNotExtraState maybe ok, but extra state is used to contrast to state, not sure that state should be defined in contrast to extra state. Let's leave it as is until we have something really nice.

inbound land http://hg.mozilla.org/integration/mozilla-inbound/rev/c25eeb46c2a4

Merged: http://hg.mozilla.org/mozilla-central/rev/c25eeb46c2a4