467144 - nsIMutationObserver::AttributeChanged should provide old attribute value

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[alexander :surkov (:asurkov)]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[alexander :surkov (:asurkov)]

(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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

(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?

Comment 5

[alexander :surkov (:asurkov)]

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.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

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?

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Olli what do you think about the AttributeWillChange approach?

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: wip

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: wip2

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

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 382150 [details] [diff] [review]
wip2

How's this looking? (Try server looked ok)

Comment 12

[Boris Zbarsky [:bzbarsky]]

>+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.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 1

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

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch 2

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?

Comment 17

[David Bolter [:davidb] (NeedInfo me for attention)]

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?

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

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

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Jonas, a friendly ping for your r? :)

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

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