Last Comment Bug 314343 - DOMAttrModified event doesn't show newValue anymore
: DOMAttrModified event doesn't show newValue anymore
Status: VERIFIED FIXED
: fixed1.8, regression, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Martijn Wargers [:mwargers] (not working for Mozilla)
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 236476
  Show dependency treegraph
 
Reported: 2005-10-29 11:35 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2005-11-02 20:51 PST (History)
4 users (show)
mscott: blocking1.8rc2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.09 KB, application/xhtml+xml)
2005-10-29 11:36 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
patch, which backs out part of the patch from bug 236476 (1.15 KB, patch)
2005-10-29 14:31 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
jonas: review+
bzbarsky: superreview+
mtschrep: approval1.8rc2+
Details | Diff | Splinter Review
Port to trunk (1.07 KB, patch)
2005-10-31 03:30 PST, Jonas Sicking (:sicking) PTO Until July 5th
jonas: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2005-10-29 11:35:39 PDT
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.
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-10-29 11:36:46 PDT
Created attachment 201280 [details]
testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-10-29 14:31:32 PDT
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.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2005-10-30 09:57:34 PST
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?
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2005-10-30 09:59:15 PST
err.. wait a minue. This regressed in january?
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2005-10-30 10:01:26 PST
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.
Comment 6 Boris Zbarsky [:bz] 2005-10-30 10:18:46 PST
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.
Comment 7 Jonas Sicking (:sicking) PTO Until July 5th 2005-10-30 12:16:49 PST
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.
Comment 8 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-10-30 12:53:46 PST
(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 9 Jonas Sicking (:sicking) PTO Until July 5th 2005-10-30 17:59:05 PST
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.
Comment 10 Boris Zbarsky [:bz] 2005-10-30 18:30:39 PST
> 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...
Comment 11 Jonas Sicking (:sicking) PTO Until July 5th 2005-10-31 03:30:35 PST
Created attachment 201414 [details] [diff] [review]
Port to trunk

Forwarding my r+. This is just a trunk port of Martijns patch.
Comment 12 Asa Dotzler [:asa] 2005-10-31 16:21:57 PST
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. 
Comment 13 Boris Zbarsky [:bz] 2005-10-31 18:30:14 PST
Fix checked in on trunk.
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2005-11-01 02:31:49 PST
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.
Comment 15 Scott MacGregor 2005-11-01 17:40:17 PST
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. 
Comment 16 Scott MacGregor 2005-11-01 18:31:19 PST
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). "

Comment 17 Scott MacGregor 2005-11-01 19:00:14 PST
"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?
Comment 18 Boris Zbarsky [:bz] 2005-11-01 19:08:55 PST
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 19 Boris Zbarsky [:bz] 2005-11-01 19:36:16 PST
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.
Comment 20 Jonas Sicking (:sicking) PTO Until July 5th 2005-11-02 04:46:52 PST
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.
Comment 21 Boris Zbarsky [:bz] 2005-11-02 08:03:40 PST
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... ;)
Comment 22 Mike Schroepfer 2005-11-02 18:02:09 PST
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 23 Mike Schroepfer 2005-11-02 18:29:00 PST
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.
Comment 24 Boris Zbarsky [:bz] 2005-11-02 20:51:08 PST
Fixed on branch.

Note You need to log in before you can comment on or make changes to this bug.