338679 - DOMAttrModified event reports new value as prevValue for style changes

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Aiko]

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

Comment 1

[Aiko]

Attachment: testcase

Comment 2

[Robert Sayre]

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 }

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Robert Sayre]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Robert Sayre]

Attachment: add argument to SetInlineStyle

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Robert Sayre]

(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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Robert Sayre]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[sgl4kn]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[sgl4kn]

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

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[timeless]

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.

Comment 17

[sgl4kn]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[sgl4kn]

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?

Comment 20

[Zack Weinberg (:zwol)]

Attachment: new patch (depends on bug 569719)

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 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 459916 [details] [diff] [review]
new patch (depends on bug 569719)

r=bzbarsky.  The test looks good.

Comment 22

[Zack Weinberg (:zwol)]

Attachment: revised for changes in 569719

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 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 462229 [details] [diff] [review]
revised for changes in 569719

Ah, yes.  Good catch!

Comment 24

[Zack Weinberg (:zwol)]

Attachment: minor corrections

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

Comment 25

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/79d6ab339484

Comment 26

[Boris Zbarsky [:bzbarsky]]

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