Child of bug 423737. I'm a bit worried our mochitests don't fully cover this change; but I think we have to do it.

David, please describe each change why it's necessary.

(In reply to comment #1) > David, please describe each change why it's necessary. Ar you worried about the cost of putting the events in our queue?

In general I would prefer we fire all our events async unless it would cause a serious a11y regression. In our wiki table there is only one state change that is marked yes/green. It feels funky to me to have some state changes that are sync and some that are async. I know combo boxes are fragile WRT events, do we have decent mochitest coverage for them?

(In reply to comment #3) > In general I would prefer we fire all our events async unless it would cause a > serious a11y regression. In our wiki table there is only one state change that > is marked yes/green. It feels funky to me to have some state changes that are > sync and some that are async. I don't see a reason to fire events async while it's really necessary. Some events are sync and some of them are async, that's we have. We shouldn't change it without good reason because it changes an event order.

(In reply to comment #4) > (In reply to comment #3) > > In general I would prefer we fire all our events async unless it would cause a > > serious a11y regression. In our wiki table there is only one state change that > > is marked yes/green. It feels funky to me to have some state changes that are > > sync and some that are async. > > I don't see a reason to fire events async while it's really necessary. Some > events are sync and some of them are async, that's we have. We shouldn't change > it without good reason because it changes an event order. The order in the queue should be the same but I do wonder if that is preserved or not (because of remove dupes etc). Would be good to discuss this over IRC, but yes I am concerned about event order as well. I am also concerned about what might be safe to fire sync now, might not be in the future (future-proofing). Can you have a look at our wiki to see which state-change events you think need to be async (for safety). I'm still not sure I understand the yes/no yellow/green/red permutations.

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > In general I would prefer we fire all our events async unless it would cause a > > > serious a11y regression. In our wiki table there is only one state change that > > > is marked yes/green. It feels funky to me to have some state changes that are > > > sync and some that are async. > > > > I don't see a reason to fire events async while it's really necessary. Some > > events are sync and some of them are async, that's we have. We shouldn't change > > it without good reason because it changes an event order. > > The order in the queue should be the same but I do wonder if that is preserved > or not (because of remove dupes etc). In the case when all events are async. That's not true while we have sync events. > Would be good to discuss this over IRC, but yes I am concerned about event > order as well. I am also concerned about what might be safe to fire sync now, > might not be in the future (future-proofing). I can see your point but I'm not sure safe/unsafe state is a subject to change. Technically we could follow the rule, if it's internal notification then it can be unsafe, if it's DOM event then it must be safe. > > Can you have a look at our wiki to see which state-change events you think need > to be async (for safety). This should mean to fix the bug ;) If you like I'll look of course. > I'm still not sure I understand the yes/no > yellow/green/red permutations. if you mean the last column: green (yes) is safe, red (no) is unsafe, yellow (yes or no) means I'm not sure, i.e. it can be safe or unsafe depending on first safe/unsafe column. First of all we need to get somebody to check yes/no of the first column and remove '?' marks.

David, can you give a recommended security rating, please? https://wiki.mozilla.org/Security_Severity_Ratings

(In reply to comment #7) > David, can you give a recommended security rating, please? > > https://wiki.mozilla.org/Security_Severity_Ratings I would say "Critical" but I don't think it is easily exploitable. I'm no security expert though.

This patch makes the following state change firing cases safe: * the observe interface case. * the handle event cases. * the handle popup hiding case As per the initial investigation (wiki). If you agree with this in principle, I'll look at our test coverage and follow up with Marco etc.

Iirc Olli said DOM events are safe, I guess observer should be safe as well. Can we get wiki updated before to fix this bug?

(In reply to comment #10) > Iirc Olli said DOM events are safe, I guess observer should be safe as well. Olli said that handling DOM events is fine but... I'm not sure it is fine in the sense that we can fire an async event from our handler that could potentially cause the DOM to be mutated. Olli can you confirm? What should we expect IsSafeToRunScript to return during the handling of a DOM event (through observer)? > Can we get wiki updated before to fix this bug? When I last spoke to Olli he indicated he had done his updates at that work week late last year.

Observers are very different creatures than event listeners. It may or may not be safe to run scripts in the observer, it all depends on who and when fires the notification. I did send some reply to that email about the wiki page. I still don't quite understand those parts which have only '?'. (I probably said something else in the email too, need to find it...)

(In reply to comment #12) > I still don't quite understand those parts which have only '?'. I used '? marks to specify I don't know the core code that triggers a11y code, but mostly it's related with DOM events handling. For example, nsRootAccessible:: HandleEvent ("DOMContentLoaded") means that we handled DOMContentLoaded event. Would it more readable if I replace '?' marks on DOM event types?

Well, just say that DOM Event handler triggers some a11y code to be executed.

I've updated the wiki - https://intranet.mozilla.org/Accessibility/SafeA11yEvents

(In reply to comment #15) > I've updated the wiki - > https://intranet.mozilla.org/Accessibility/SafeA11yEvents OK so I understand we are waiting for Olli to confirm the first (left side) yes/no column for us.

They look right to me, expect that I'm not at all familiar with topic="obs_documentCreated" and topic = NS_XPCOM_SHUTDOWN_OBSERVER_ID might be actually safe. XPCOM shutdown handling is documented in MDC.

Thanks guys.

David, it's worth to learn whether topic="obs_documentCreated" is safe prior to a11y changes.

(In reply to comment #17) > They look right to me, expect that I'm not at all familiar with > topic="obs_documentCreated" Ehsan told me safety is not guaranteed here. Boris? > and topic = NS_XPCOM_SHUTDOWN_OBSERVER_ID might be actually safe. > XPCOM shutdown handling is documented in MDC. Olli, I'm not sure.

> Ehsan told me safety is not guaranteed here. Boris? Yep. It's called at unsafe times. Too bad it calls out into random chrome JS, in theory....

(In reply to comment #21) > > Ehsan told me safety is not guaranteed here. Boris? > > Yep. It's called at unsafe times. Too bad it calls out into random chrome JS, > in theory.... Ok. David, try-build, feedback from Marco, would be nice to have mochitest because it will allow you to describe what exactly Marco should test ;) and make me feel more happy :), r=me.

Comment on attachment 445390 [details] [diff] [review] trivial patch 3 Adding Ehsan, note more testing is required before landing.

Comment on attachment 445390 [details] [diff] [review] trivial patch 3 rs=me on the safety issue.

OK. How long before we can land this?

Marco, the try build you gave a spin over on bug 561932 included this patch. Can you try the same build against https://bug512059.bugzilla.mozilla.org/attachment.cgi?id=396056 and make sure when you tab to the editable document that it is reported as editable? I'm pretty confident this patch is low risk and can land, but value Alexander's happiness :)

(In reply to comment #26) > I'm pretty confident this patch is low risk and can land, but value Alexander's > happiness :) Please, it won't take much your time :) While you're here it's worth to add mochitests, at least this is the rule we are trying to follow.

Comment on attachment 448378 [details] [diff] [review] patch 3.1 (added test) >+ function makeEditable(aIdentifier) >+ { >+ this.DOMNode = getNode(aIdentifier); >+ >+ this.eventSeq = [ >+ new invokerChecker(EVENT_STATE_CHANGE, getAccessible(this.DOMNode)) instead of standard checker it makes sense to use own checker to ensure you get state change event for correct state and state has correct value.

(In reply to comment #29) > instead of standard checker it makes sense to use own checker to ensure you get > state change event for correct state and state has correct value. Done.

Comment on attachment 448397 [details] [diff] [review] patch 3.2 (test state as well) >diff --git a/accessible/tests/mochitest/events/test_doc.html b/accessible/tests/mochitest/events/test_doc.html just noticed, you know, I'm going to rename it to test_docload.html :) to emphasize it's about document handling only. It makes sense to have file test_statechange.html or something to keep these thing separately. >+ function makeEditableDoc(aDocNode) >+ >+ this.check = function editabledoc_check() { >+ testStates(aDocNode, 0, EXT_STATE_EDITABLE); >+ } this is good, however I meant to add check for nsIAccessibleStateChangeEvent. At least we'll be closer to testing of what we fire on linux.

Comment on attachment 448412 [details] [diff] [review] patch 3.3 > if (!nsCRT::strcmp(aTopic,"obs_documentCreated")) { > // State editable will now be set, readonly is now clear > nsRefPtr<nsAccEvent> event = > new nsAccStateChangeEvent(this, nsIAccessibleStates::EXT_STATE_EDITABLE, > PR_TRUE, PR_TRUE); >- nsEventShell::FireEvent(event); >+ FireDelayedAccessibleEvent(event); the same thing: don't we want to remove dupes? > } if we don't then we should explicitly pass eAllowDupes or something. What happens if you do document.designMode="on"; document.desginMode="off"; document.designMode="on"?

I don't think we need to worry about dupes here.

(In reply to comment #33) > if we don't then we should explicitly pass eAllowDupes or something. What > happens if you do > document.designMode="on"; > document.desginMode="off"; > document.designMode="on"? Two events for the "on" -- which I think is fine. Filed bug 569340 for the missing "off".

(In reply to comment #35) > Two events for the "on" -- which I think is fine. Filed bug 569340 for the > missing "off". Why do you think it is fine?

(In reply to comment #36) > (In reply to comment #35) > > > Two events for the "on" -- which I think is fine. Filed bug 569340 for the > > missing "off". > > Why do you think it is fine? Because we turned it on twice :) In general I don't think we should coalesce prematurely, but rather we should wait for real cases that show us we need to -- unless the benefit is obvious.

(In reply to comment #37) > (In reply to comment #36) > > (In reply to comment #35) > > > > > Two events for the "on" -- which I think is fine. Filed bug 569340 for the > > > missing "off". > > > > Why do you think it is fine? > > Because we turned it on twice :) We should report about current states and ignore stuffs in the middle of processing. > In general I don't think we should coalesce prematurely, but rather we should > wait for real cases that show us we need to -- unless the benefit is obvious. I wouldn't say it's good option to wait for a bug report than to fix it when we see it. On the another hand we should care about out-of-process clients when we can. Here you put event into queue and it's traversed any way therefore it makes compare nodes and coalesce if necessary. That's in theory. It's about why it's a bug and why it should be considered be fixed. In practice state change events are fired for different states and we can't coalesce them by nodes only.

Sure, fixing when we see it is fine with me but we probably don't agree on "when we see it" :) I'm actually not sure what we are discussing. I think this bug is just about making the event async -- we didn't coalesce this event before of course. We should probably capture coalescence discussion on a different bug.

I don't mind to have follow ups. I just need to be sure you do right things here.

Comment on attachment 448512 [details] [diff] [review] patch 3.4 Unrequested review to fix up comments

Filed Bug 569356 because our state change events coalescence is janky.

Comment on attachment 448530 [details] [diff] [review] patch 3.5 >+ <title>Accessible events testing for document</title> fix the title. >+ this.check = function editabledoc_check(aEvent) { >+ var event = null; >+ try { >+ var event = aEvent.QueryInterface(nsIAccessibleStateChangeEvent); >+ } catch (e) { >+ ok(false, "State change event was expected"); >+ } >+ >+ if (!event) { return; } >+ >+ ok(event.isExtraState(), "Extra state change was expected"); >+ is(event.state, EXT_STATE_EDITABLE, "Wrong state of statechange event"); >+ ok(event.isEnabled(), "Expected editable state to be enabled") add ';' in the end btw, I liked you tested state of accessible, it's worth to this since it's how windows screen readers work. >+ <div id="eventdump" src="about:blank"></div> what's @src about here? r=me with comments fixed

Comment on attachment 448530 [details] [diff] [review] patch 3.5 r=me with no additional nits. :)

(In reply to comment #44) > (From update of attachment 448530 [details] [diff] [review]) > > >+ <title>Accessible events testing for document</title> > > fix the title. > OK I made it more specific. > >+ ok(event.isEnabled(), "Expected editable state to be enabled") > > add ';' in the end Done. > > btw, I liked you tested state of accessible, it's worth to this since it's how > windows screen readers work. OK done. > > >+ <div id="eventdump" src="about:blank"></div> > > what's @src about here? Cruft - thanks for catching. > > r=me with comments fixed Thanks guys.

Landed http://hg.mozilla.org/mozilla-central/rev/60c10254881c