1.09 KB, application/xhtml+xml
1.15 KB, patch
Mike Schroepfer: approval1.8rc2+
|Details | Diff | Splinter Review|
1.07 KB, patch
|Details | Diff | Splinter Review|
See upcoming testcase. When clicking on the button in the testcase, I get to see values for attrName and prevValue, but not for newValue. This works fine in the 2005-01-24 build, but fails in the 2005-01-25 build: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-01-24+08%3A00%3A00&maxdate=2005-01-25+10%3A00%3A00&cvsroot=%2Fcvsroot I suspect bug 236476 might be the cause.
Created attachment 201294 [details] [diff] [review] patch, which backs out part of the patch from bug 236476 With this patch, it works again. so I guess aParsedValue.ToString(newValue) doesn't something unexpected here.
bz: this seems like your doing. Martin: That patch should not even apply since the function you're modifing no longer exists. Are you sure you're applying to, and testing with, a trunk tree?
err.. wait a minue. This regressed in january?
Adding blocking request since this is a bad regression. But I'll have to develop a patch to see if it'll be safe enough.
So if nothing else, calling SetAndTakeMappedAttr means that aParsedValue now holds the _old_ value, not the _new_ value, right? And this is a mapped attr, so the "old" value is likely to be rather weird, with all the mapped stuff we do. I suspect that martijn's patch is the right one for branch. For trunk, we should think about doing it the same way we do OnAttrChange.
Aw, crap! Actually, it holds an empty value since that is what SetAndTakeAttr does. We need to do this in more places then in html elements though. The same bug is probably in XUL and SVG. Martijn, care to come up with a patch for those too? Otherwise i will when i get home.
(In reply to comment #7) > Martijn, care to come up with a patch for those too? Otherwise i will when i > get home. Well, probably better if you do it. I don't really this code. XUL doesn't seem to have this problem (when looking at the code in nsXULElement.cpp)
Comment on attachment 201294 [details] [diff] [review] patch, which backs out part of the patch from bug 236476 Turns out GetAttr is called in both SVG and XUL so this is only an issue in SVG. Lets do this on trunk too for now so that we can get testing on this for the branch. Though it should be a very safe patch, partially because it's very simple, and partially because it lives in code that is ran very rarly.
> Lets do this on trunk too for now so that we can get testing on this for the > branch. That patch doesn't apply to trunk -- that code is in nsGenericElement now...
Created attachment 201414 [details] [diff] [review] Port to trunk Forwarding my r+. This is just a trunk port of Martijns patch.
We need to see this landed (in some form) on the trunk and it'd be nice if we could get jst to look here.
Fix checked in on trunk.
Jst, mind looking at the patch here and putting your approval stamp on it? The patch is really simple, but the more eyes the better. What happens is that I tried to get the old value by calling aParsedValue.ToString However that doesn't work since a bit higher up I did mAttrsAndChildren.SetAndTakeAttr(aParsedValue) which destroys aParsedValue (hence the 'and take'). So just calling GetAttr will get the value from the mAttrsAndChildArray instead. This only affects mutation events since this is mutation-event-firing code. So it doesn't matter that this is somewhat suboptimal perf-wise. I'll try to come up with samething faster for trunk in a different bug.
at the meeting today we decided this isn't something we would respin the release for and we're trying to hold the line to critical issues that would require a respin only. So I'm going to minus this now. Could be a good candidate for 1.8.1 release down the line.
copy and pasting some comments from bz to drivers for folks to consider: "I think this is a significant enough standards-support regression that we really should take it... Note that as far as I understand the bug was discovered because it broke some front-end code that Martijn was working on. And it just happens that we use this functionality in out mailnews UI. And Nvu uses this. Further, DOMAttrModified events are well documented for things like SVG and XBL (I found at least two sites in Russian documenting DOMAttrModified in the first few pages of Google hits, and both are using the exact functionality we've broken here). "
"Turns out GetAttr is called in both SVG and XUL so this is only an issue in SVG." This is really HTML and SVG though right?
That was a typo in sicking's comment, I believe. On branch, as things stand, newValue is broken for DOMAttrModified mutation events on HTML nodes and only HTML nodes. It works for XUL, SVG, and generic XML. The point of my mail is that this stuff is in fact being used, and even if the obvious uses are not overtly broken it's not really a good idea to have parts of our DOM working for some types of nodes but not others...
Comment on attachment 201294 [details] [diff] [review] patch, which backs out part of the patch from bug 236476 I'd like to strongly urge that we reconsider not fixing this regression for 1.8.
I'd like to echo Boris' concern here. Though the regression is in functionality that is not that often used, I think this is largly because IE does not supply similar functionality. For people using this feature we've broken it pretty badly. Additionally the patch is extreamly safe. Not only is it very simple in itself, the new code looks exactly like code we have elsewhere in the tree (in XUL and SVG code) so it known to be safe and correct. And even if this patch fudges things, it will only fudge things that are broken and rarely used anyway. So the risk accepting this patch is very low.
I should clarify sicking's comment, since there are several things in play here which might seem mutually contradictory: 1) The bug exists only for HTML, not SVG, XML, or XUL. 2) The fix is to make HTML do exactly what SVG and XUL already do. 3) This functionality is not commonly used on public web sites, because IE does not support it. This means that changes to this code won't suddenly break a top100 site. 4) This functionality _is_ used in various Firefox-based applications, including extensions, Nvu, and at least one intranet app I'm aware of. So the point is that not fixing this screws over the cases listed in point 4. Fixing this helps them, and should have minimal impact on common web sites, due to point 3. Just trying to explain how something can be both "rarely used" and important to fix... ;)
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051102 Firefox/1.6a1 Verfied on 11-2 trunk build
Comment on attachment 201294 [details] [diff] [review] patch, which backs out part of the patch from bug 236476 Per triage meeting today approving after verification.
Fixed on branch.