314286 - Further improve SetAttr

Status: RESOLVED INACTIVE

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

Description

[Boris Zbarsky [:bzbarsky]]

This is a followup to bug 308270.  Two things came up there:

1)  The three boolean args to SetAttrAndNotify should probably be combined into a single flags argument.
2)  We should rework the hooks system as follows:  Instead of having a BeforeSetAttr hook and an AfterSetAttr hook, just have a single OnAttrChange hook, called from both SetAttr and UnsetAttr.  This would get called after we set the new value in mAttrsAndChildren but before we send document observer notifications about the attr change or fire mutation events.  The OnAttrChange hook would get both the old and the new attr value; the old one will end up in our nsAttrValue in SetAttrAndNotify as a natural consequence of SetAndTakeAttr (and similar for mapped attrs) swapping values, while to get at the new one we might need to change SetAndTakeAttr return the pointer to the new attr value.  Or something.  With this change, it should be possible, sicking and I think, to remove all current BeforeSetAttr and AfterSetAttr functions, and remove most of the existing SetAttr/UnsetAttr overrides (all of them except nsHTMLImageElement, possibly).

sicking, were you interested in doing this?  Or should I take a shot at it?

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I can have a whack at it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Jonas, do you think you'll be able to do this for 1.9?  If not, I'm still offering to take a stab at it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  So one issue is that actually getting a pointer to the new value out of mapped attributes is a bit of a pain.  In particular, the process of making a nsMappedAttributes unique means that a pointer from nsMappedAttributes::SetAndTakeAttr is not usable...

Options are to either just pass the old value to OnAttrChange or to pass some boolean indicating that the new value wasn't easy to get hold of and should be regotten.

Another question that I have is whether we want to pass the new/old value as nsAttrValue& or nsAttrValue*.  The latter lets us use null for "wasn't set before" and "is being unset", while if we do the former we might need to pass around some booleans if subclasses care about this sort of thing.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So I've discovered a BeforeSetAttr/AfterSetAttr that actually acts differently depending on whether the attr was set to empty string or not set at all.  <input type="radio"> with no name at all is not added to radio groups, while <input type="radio" name=""> is added to a radio group.

I tried Opera, Konqueror, and had someone on IRC try IE.  Opera puts nameless radios and radios with name="" in the same radio group.  Konqueror doesn't put either nameless radios nor radios with name="" in radio groups at all.  IE doesn't even allow selecting a radio that has no name attribute or has name="".

So sounds like compat is just not an issue here; I'd like to go with either Opera's or Konqueror's behavior.  Opera's is probably a tad less code (one less IsEmpty() check).

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

All other <input> types don't submit if name is missing, but do submit if name="". This would make it weird to put those two cases in the same radio group. We could of course make an exception for radios and never or always submit them, independent of if name exists.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Or leave submission as-is and do what Konqueror does as far as radio groups. Or pass around the "attr used to be set" state, I suppose....

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Testcase

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK, so the plan is to do what Opera does here.  Put all nameless and name="" radios in one group, and don't submit that group.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I had Hixie test the attached testcase in Opera. Seems like name="" and nameless controls are considered the same group, and controls in that group never submits.

I think that's the behaviour we should have. The downside is that it'll make <input name=""> (which does submit) different from <inpyt type=radio name=""> (which wouldn't submit). However Hixie pointed out that we should consider changing that. I'll file a followup on that.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So there are several use cases of SetAttr that would be pretty tough to convert to OnAttrChange.  The various callers of ForgetLink(), in particular....  They could have used BeforeSetAttr if we did that, but doing what they want from OnAttrChange would require a lot of code refactoring.  Perhaps we just shouldn't worry about them for the time being...

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think the question is, would they be better with such refactoring in the end? If we can't use OnAttrChange then we can't, we shouldn't try to squeeze things in there that doesn't really fit.

But I don't think we should hold OnAttrChange if it's a good idea in the end but would take some coding to get to everywhere.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: checkpoint

> I think the question is, would they be better with such refactoring in the
> end?

I'm not really sure.  We'd need to refactor all the "get the link URI code" to take more than just a content node, basically.

This is my current content/ diff for the relevant tree, if you're interested.  I ran into several SetAttr impls so far that couldn't really use OnAttrChange for various reasons; those are documented (search for "can't" in the diff).  If OnAttrChange could return a callback to run once it's "safe", most of them could use it, I think....

I do think that a lot of stuff does become cleaner with OnAttrChange, fwiw.  Just not everything.  ;)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Checkpoint merged to trunk

I'm not going to have time to work on this, so I figured I should post what I have...  I need this tree for other work.  :(