nsIMutationObserver::AttributeChanged should provide old attribute value. This will allow to send accessibility events correctly (bug 467143 and bug 452388) when ARIA attributes are changed. Does this sound well enough to start the work on patch?

1) This would impose a noticeable performance penalty on some common operations (e.g. setting foo.style.whatever). 2) This _could_ probably be done in most cases without too much performance penalty if bug 314286 is fixed. 3) Is a AttributeWillChange which provides the new value (and the element, so you can get the old value) a reasonable solution here? See also bug 231676.

(In reply to comment #1) > 3) Is a AttributeWillChange which provides the new value (and the element, so > you > can get the old value) a reasonable solution here? Almost all we need is to notify AT. We could use delayed events in the case of AttributeWillChange. Therefore I think it should be good way if the change of AttributeChanged method hits performance.

Could a11y cache the attribute values it needs? Maybe there isn't need to cache the string value, but just some enum or integer?

(In reply to comment #3) > Could a11y cache the attribute values it needs? Maybe there isn't need to > cache the string value, but just some enum or integer? In theory I think we could, the ones we really care about have NMTOKEN values. Surkov? Thoughts?

I don't think caching will work until we create accessible tree on demand. In the meantime if attribute is changed then may create accessible for DOM element attribute is changed for. That means we don't have proper place to cache attribute values only if we don't want to run through whole DOM to prepare initial caching. As well we aren't restricted by NMTOKEN values. If we want to implement inverse relations properly then we need to have previous IDRefs values to avoid relation recreation.

Yeah, after running a few laps, I think Boris's AttributeWillChange approach from comment #1 is a good candidate. (I've read over the related bugs) Do we want to start exploring this, perhaps via #ifdef ACCESSIBILITY for now? And by "we", I mean probably not me (yet). Volunteers?

Olli what do you think about the AttributeWillChange approach?

Okay, Smaug mentioned on IRC that the approach sounded right. Posting this early WIP to see if we are all on the same page.

You don't need a state mask there. And there are indent issues. And you probably want to send that notification even if there is no current document on the node. And nix nsIDocument::AttributeWillChange and use this notification instead. With those nits, looks about right.

Tweaked slightly since last try server run (which passed fine with a couple of known orange, perf was green). Trying this one now.

Comment on attachment 382150 [details] [diff] [review] wip2 How's this looking? (Try server looked ok)

>+nsDocument::AttributeWillChange(nsIDocument* aDocument, Fix the indent of the function arguments here. >+ And don't add the blank line? >+ nsIContent* content = static_cast<nsIContent*>(this); You shouldn't need the cast. Just pass |this| to nsNodeUtils::AttributeWillChange. This applies to both cases where you do that. And fix the indent on the first caller too? > +nsXULDocument::AttributeWillChange(nsIDocument* aDocument, Again, fix the indent. >nsHTMLFramesetFrame::FrameResizePrefCallback(const char* aPref, void* aClosure) >+ nsNodeUtils::AttributeWillChange(frame->mContent, kNameSpaceID_None, Fix the indent here too? And this call should be conditioned on |doc|, no? With those nits looks ok; I'd like Jonas to look over the nsGenericElement and nsXULElement changes just to make sure that the point where we're making this call is early enough (so that we still have our original value) and that those are all the places where we need to make the call.

Okay thanks, I think I fixed the nits here. Jonas, as per comment #12, your thoughts?

That patch didn't fix the nsXULDocument indent, still has the unnecessary nsIContent* temporaries when calling into nodeutils, and still has the weird indent in FrameResizePrefCallback.

Agreed. There is no need for the nsIContent* temporaries -- fixing. (I could have sworn I saw this elsewhere in the same file and was trusting the precedent). I'm also going to win this indent battle with Eclipse; sorry.

OK I think the indent battle is won, and the unnecessary temp vars are removed. Note I moved the document->ForgetLink call after the AttributeWillChange notification in nsGenericElement because it seems safer?

While we are here... We're going to need to know when nodes are going to be removed. Perhaps the existing NodeWillBeDestroyed would suffice? Are nodes ever removed and not destroyed?

Actually a quick chat with Smaug I realize ContentRemoved should be sufficient. Please disregard comment #17 (at least for now).

Jonas, a friendly ping for your r? :)

Pushed on 1.9.2: http://hg.mozilla.org/mozilla-central/rev/9c23b293dc4e