User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a1) Gecko/20060517 Minefield/3.0a1 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a1) Gecko/20060517 Minefield/3.0a1 If the DOMAttrModified event is fired because of a change to its style object, event.prevValue isn't the old style value but the same as event.newValue. The attached testcase only tests "width" but as far as I can tell all style properties are affected. If the style is modified via setAttribute("style", newValue) instead then prevValue is reported correctly. Reproducible: Always

This particular test case seems to be a known issue, at least in the source code. The last place we have access to the old value is nsDOMCSSDeclaration.cpp#244, which then calls the subclass' implementation of DeclarationChanged(). I'm not sure we can fix this without changing the signature of nsGenericHTMLElement::SetInlineStyleRule. 80 nsDOMCSSAttributeDeclaration::DeclarationChanged() 81 { 82 NS_ASSERTION(mContent, "Must have content node to set the decl!"); 83 nsICSSStyleRule* oldRule = mContent->GetInlineStyleRule(); 84 NS_ASSERTION(oldRule, "content must have rule"); 85 86 nsCOMPtr<nsICSSStyleRule> newRule = oldRule->DeclarationChanged(PR_FALSE); ... 90 91 return mContent->SetInlineStyleRule(newRule, PR_TRUE); 92 } ------------------------------ 1755 nsGenericHTMLElement::SetInlineStyleRule(nsICSSStyleRule* aStyleRule, 1756 PRBool aNotify) 1757 { ... 1767 // There's no point in comparing the stylerule pointers since we're always 1768 // getting a new stylerule here. And we can't compare the stringvalues of 1769 // the old and the new rules since both will point to the same declaration 1770 // and thus will be the same. 1771 if (hasListeners) { 1772 // save the old attribute so we can set up the mutation event properly 1773 // XXXbz if the old rule points to the same declaration as the new one, 1774 // this is getting the new attr value, not the old one.... 1775 modification = GetAttr(kNameSpaceID_None, nsHTMLAtoms::style, 1776 oldValueStr); 1777 } 1778 else if (aNotify) { 1779 modification = !!mAttrsAndChildren.GetAttr(nsHTMLAtoms::style); 1780 } ... 1786 }

Yeah, you'd want to change the signature of SetInlineStyleRule to take the old value, and change various other stuff to propagate the old value through the system as needed. Or some other similar refactoring. This is about as low-priority as things get for me, frankly, but I'd be willing to review a patch that doesn't regress perf in the usual (no mutation listeners) case if someone writes one.

(In reply to comment #3) > > This is about as low-priority as things get for me, frankly, but I'd be willing > to review a patch... OK, I'll take it. I have my head in the code already. Is it ok to pass the old value as a string?

You'd have to; there's not much else you can pass it as (short of an atom).

Comment on attachment 223659 [details] [diff] [review] add argument to SetInlineStyle I'd need to see numbers to convince me that this doesn't regress perf... Based on everything I know about this code, this would slow down setting inline style a good bit.

(In reply to comment #7) > (From update of attachment 223659 [details] [diff] [review] [edit]) > I'd need to see numbers to convince me that this doesn't regress perf... Based > on everything I know about this code, this would slow down setting inline style > a good bit. > Hmm, tdhtml seems down in the noise (see below). what's a better test? without patch: Test Median Average Data ============================================================ colorfade: 1513 1516 1566,1481,1513,1475,1544 diagball: 1810 1823 1864,1765,1897,1810,1780 fadespacing: 2632 2651 2816,2568,2555,2632,2685 imageslide: 426 414 363,429,426,424,426 layers1: 4288 4486 5547,4288,4141,4319,4136 layers2: 108 109 108,114,108,108,108 layers4: 108 108 108,109,107,108,107 layers5: 2878 2868 2813,2879,2878,2875,2895 layers6: 254 254 256,254,253,252,256 meter: 1320 1324 1378,1284,1320,1321,1319 movingtext: 1164 1162 1154,1165,1164,1164,1165 mozilla: 2957 2959 2988,2957,2999,2944,2907 replaceimages: 3083 3183 3585,3076,3083,3075,3098 scrolling: 31581 31564 31490,31548,31581,31619,31582 slidein: 2692 2731 2898,2696,2692,2686,2682 slidingballs: 1718 1707 1763,1665,1722,1718,1669 zoom: 587 591 490,706,623,587,550 _x_x_mozilla_dhtml,1334 with patch: Test Median Average Data ============================================================ colorfade: 1516 1498 1480,1407,1551,1536,1516 diagball: 1878 1850 1738,1849,1890,1878,1893 fadespacing: 2575 2568 2575,2540,2575,2562,2587 imageslide: 423 419 391,435,422,423,426 layers1: 4563 4674 4884,4562,4563,4800,4563 layers2: 125 125 125,124,125,126,124 layers4: 124 125 127,124,124,124,124 layers5: 3339 3339 3292,3374,3339,3352,3337 layers6: 310 310 312,310,310,307,310 meter: 1272 1293 1393,1265,1275,1261,1272 movingtext: 1181 1184 1195,1181,1204,1178,1163 mozilla: 3022 3002 2920,3022,3010,3025,3034 replaceimages: 3076 3076 3076,3076,3086,3045,3097 scrolling: 31701 31684 31559,31672,31701,31753,31736 slidein: 2682 2740 2990,2670,2682,2672,2686 slidingballs: 1892 1904 1904,1855,2016,1892,1852 zoom: 576 638 893,604,553,563,576 _x_x_mozilla_dhtml,1397

That's not down in the noise; that's a 5% regression.

(In reply to comment #9) > That's not down in the noise; that's a 5% regression. Ok, I was looking at argo-vm.mozilla.org and saw the tests fluctuating in a 20% range. Other machines look more stable, though. I changed the switch in SetInlineStyle back to the old way, and that seemed to slow things down. Throw me a bone... what made you think this would be a regression?

> what made you think this would be a regression? With this change, on every single inline style set we serialize the current inline style to a string first. This doubles the amount of time we spend compressing and decompressing the css data blocks (which is a noticeable part of the total time in profiles of pages like the one in bug 229391), adds the time to actually perform the serialization, etc. As in, we do more work, by a good amount. So I would expect us to get slower. How much slower is an interesting question; at the very least it depends on the exact styles used (the more properties set in the inline style, the bigger the slowdown). In all cases, I'd need to see data indicating that the slowdown is negligible before we make this change. The other option, of course, is to only serialize when we really need to -- when we know we're going to fire the mutation event. I'd really prefer that, if possible.

It is impossible to compare prevValue and newValue. https://bugzilla.mozilla.org/attachment.cgi?id=222747 Why is this still not fixed and since 2 months still "new"?

> Why is this still not fixed Because there are finite resources and other bugs seem to be higher priority. The best way to get it fixed is to pay someone to fix it, if you can find someone with the time to do so... > and since 2 months still "new"? That's its status. It's a bug, it's not assigned to anyone. Note that it's not like you can depend on either newValue or prevValue reflecting anything sane about the state of the node at any point in time, since DOM events can be received in the opposite order from the changes actually occurring...

>Because there are finite resources and other bugs seem to be higher priority. It´s not that a feature is not working properly, an implemented feature is not working completely. IMHO only a crash can be more important than an old DOM 2 property that is not supported and should be as specs. >since DOM events can be received in the opposite order from the changes actually occurring... I don´t know why the ability of an event to bubble or not should be of any interest here. In Opera the event process is impremented this way: 1. the browser gets the command to change the attribut of a div 2. the browser saves the state of the div 3. the div gets altered 4. the new state of the div is saved 5. DOMAttrModified gets fired 6. the event bubbles or is captured by target´s childs the whole process is not cancelable and it´s unimportant if the bubbles at point 6. I don´t think that there´s another sequence possible. BTW: I´m surely not going to pay anyone, because I primary use Opera. However which source code/form is affected?

> IMHO only a crash can be more important than an old DOM 2 property I don't think others agree on this with you, especially for something like mutation events (which are of limited usefulness anyway, since they can be reentered, as I said). Frankly, if people suddenly decide it's important to have a 100% correct implementation of mutation events, my vote would be for removing support for them altogether. > I don´t know why the ability of an event to bubble or not Bubbling has nothing to do with this. Capturing has a lot more to do with it. Consider a capturing DOMAttrModified listener that changes the same attribute and think about what an at-target or bubbling listener sees. > In Opera the event process is impremented this way: I'm aware of how one would implement this in general, yes. ;) > 2. the browser saves the state of the div The point is that for the style attribute this is actually pretty expensive. Doing it unconditionally (i.e. when there are no mutation listeners) would be a pretty major performance penalty in many cases. So a correct solution would only do this if needed. > However which source code/form is affected? See attachment 223659 [details] [diff] [review]. That touches all the relevant code, I think.

i'd vote for removing mutation event support entirely :) of course, it's in my interests for browsers to be smaller and the web less pathological.

@Boris >Doing it unconditionally (i.e. when there are no mutation listeners) would be a pretty major performance penalty in many cases. I´m pretty sure that Opera always fires a mutation event. I made a few tests in the past and my mutation event script was only ~1% slower at 10000 runs (even with a RegExp) than normal rendering. Because Opera is written in C++/Qt there should be no performance issues in FF, too. >I'm aware of how one would implement this in general, yes. ;) The main point is that the whole thing is senseless when there are performance issues. It should be no problem to check the string of an attribute. If a div is changed by setAttribute, setAttributeNode, replaceData... only the string "style=''" is checked. If a script access the div via div.style.example the attribute string gets modified, or "style='example'" is add. String is saved as prevValue, changes are made and the new string is saved as newValue. Event is fired. There should be no performance loss. However I´m a little bit disappointed, Mozilla is a big company with potent sponsors and should have the ressources to implement standards. Instead of implementing new Core level 3 dom functions level 2 should be supported. >See attachment 223659 [details] [diff] [review] [edit]. Thanks.

> there should be no performance issues in FF If you can produce profiling data backing this up, great. See comment 7. > String is saved as prevValue The data is not stored as a string in this case. Getting it into string form is slow. > Mozilla is a big company Actually, it's a pretty small company. For example, as of end of 2005, Opera Software had 254 employees (see http://www.opera.com/company/investors/finance/2005/ann_rep_numbers.pdf). The Mozilla corporation has around 60 employees. Even the number of regular code contributors to the Mozilla code (many of whom are doing it very much in their spare time) is smaller than the number of Opera employees. Just figured I'd set the record straight here and all. > and should have the ressources to implement standards If all wishes came true... ;) > Instead of implementing new Core level 3 dom functions level 2 should be > supported. Parts of DOM 3 that are very useful tend to have priority over less-useful corner cases of DOM 2 in people's minds, for better or for worse.

Over two years later: >The other option, of course, is to only serialize when we really need to -- >when we know we're going to fire the mutation event. It's a pity that Mozilla took option 3: Ignoring everything and hoping that no one recognizes it. What a fortune that Acid3 just tests mutation events in general and does not compare prevValue and newValue. And yes I totally agree, supporting standards that nobody uses is totally useless. Let's just do it like MS with IE and totally ignore all SVG, events, formats and everything else that is not used very often. This would speed up FF quite some more. Perhaps implementing some CSS3 functions (while CSS2 could be dropped generously, IE works this way, too) and completing the Acid3 will do for marketing and satisfaction of everyone, or not?

With the changes in bug 569719 we can fix this a lot more cheaply. I discovered this quite by accident when those changes made the existing, xfailed test case start passing. This patch beefs up the test case -- bz, please let me know if it doesn't cover all the possibilities you can think of -- and closes the gap bz thought might still exist. Which it did, according to the revised test case.

Comment on attachment 459916 [details] [diff] [review] new patch (depends on bug 569719) r=bzbarsky. The test looks good.

Had to revise this a little after some changes requested by dbaron in bug 569719. There's a subtlety here: at the end of 569719, RuleMatched is the only place that calls SetImmutable on declarations, so we can use IsMutable on the declaration to detect whether RuleMatched has been called; but after this change there is another caller of SetImmutable, so we need to track RuleMatched having been called explicitly. Asking for re-review because of that. (bz: you might want to read the new part 16 of bug 569719 first.)

Comment on attachment 462229 [details] [diff] [review] revised for changes in 569719 Ah, yes. Good catch!

I forgot to initialize the new CSSStyleRuleImpl member variable in the other two constructors, so it blew up horribly on the try server.

This didn't quite fix all cases of the problem. See bug 724128.