Modifying the opacity property of an element that had opacity: 1.0 when the page loaded doesn't work. Since the default opacity value is 1.0, this applies to all elements that don't have opacity < 1. If the element does have opacity < 1 when the page loads, updates to the opacity property are reflected correctly on the display. The value may even be changed to 1 and back. This is a drawing problem. The actual value of the opacity property changes but is not reflected on screen. This is most likely due to an important drawing optimization to only blend elements with opacity < 1. However, if the value changes from 1, the update must be realized.

The style contexts are getting updated correctly (Show Style Context in Viewer before and after changing). However, the opacity of the view is not getting update correctly (similarly, Show Views from Viewer). I think the frames need to handle the case where they have a view and the apacity is being changed; they need to update the view's opacity.

All of the mechanisms are in place to update the view's opacity, however it is not happening correctly. I've spent a couple of hours looking at this, how important is it? Nominating nsbeta2 to get the PDT's opinion. Note: setting the opacity statically works fine, it is modification of opacity via the DOM that is failing.

Putting on [nsbeta2-] radar. Not critical to beta2. Adding nsbeta3 keyword

[nsbeta3-] and Future.

Hmm, no progress! But the bug is now a little bit bigger! If you start the testcase, now the div id="h" doesn't get opacity 0.5 on startup. You can not click the button again (i think that's an other bug!) ... in http://www336.l4.xodox.com/radio/index.html the select box wouldn't redraw rightly but this is every time you try a not correct JavaScript! To get the button work again, select something else!

The name of the opacity property has changed to "-moz-opacity", that's why the testcase doesn't work anymore. I know what the problem is with this bug. I will take care of it. Marc, I'm assigning it to myself, if that's not OK, take it back :-).

Thanks Robert - is this fixed by your view manager rewrite? CC'ing self.

No. I think the problem is that we're not creating a view for element 'w' (the one that starts with opacity 1.0). When its opacity changes to less than 1.0, we need to create a view for it. But the style system just gives us a VISUAL change hint, so we don't get a chance to create a view (which can only happen on FRAMECHANGE). The fix is to make opacity changes from 1.0 to less than 1.0 cause FRAMECHANGE. We don't want to make ALL opacity changes cause FRAMECHANGE though, because that would make fade-in/fade-out effects hideously slow. We actually have a similar problem with background-attachment. Changing it to "fixed" in the DOM means we need a view for the frame, but we won't create one. It would be cool if we could have a style change hint that meant "FRAMECHANGE if the frame has no view, otherwise VISUAL". But we shouldn't worry about that for now.

Marc, can you review?

I think maybe a hint like 'VIEWCHANGE might be useful for cases where we need to create or destroy views but otherwise don't need to muck with the frame tree, but I agree that we could address that later. Anyway, the changes look OK. A couple of issues: In ComputeChangeHint there is a typo I believe: the comment should say 'Otherwise there must already be a view' not 'Otherwise there must already be a frame' I think. Additionally, in that new function shouldn't we check for the opacity going from less-than-one back to one, so we can eliminate the view? I admit is is not as important, since having an extra view around is probably not a big deal, but it struck me as asymmetrical and suboptimal memory-wise (but better for performance since it eliminates a reframe). I have a new implemenation of that method that causes a REFAME when the opacity changes to or from 1.0 instead of just from 1.0. What do you think? Here is a possible implementaion of the method that cleans up the view when it is no longer needed: static PRInt32 ComputeChangeHint(nsCSSProperty aPropID, const nsCSSValue& aOldValue, const nsCSSValue& aValue) { switch (aPropID) { case eCSSProperty_opacity: NS_ASSERTION(aOldValue != aValue, "ComputeChangeHint should not be called with equal values"); if(aOldValue.GetUnit() == eCSSUnit_Number && aValue.GetUnit() == eCSSUnit_Number) { // If the opacity is changing to or from 1.0, then reframe to cause a view to be created or eliminated // Otherwise we just need a visual change. This is important because opacity is frequently used // for fade effects, and we don't want to reframe for every step of the fade. if( (aOldValue.GetFloatValue() < 1.0 && aValue.GetFloatValue() == 1.0) || (aOldValue.GetFloatValue() == 1.0 && aValue.GetFloatValue() < 1.0) ){ // XXX: it would be better to pass out a hint NS_STYLE_HINT_VIEWCHANGE, but it does not exist return NS_STYLE_HINT_FRAMECHANGE; } else { return NS_STYLE_HINT_VISUAL; } } default: return nsCSSProps::kHintTable[aPropID]; } }

> In ComputeChangeHint there is a typo I believe Oops, you are right. > REFAME when the opacity changes to or from 1.0 instead of just from 1.0 It looks OK. I originally thought that was unnecessarily complex, but it probably does make slightly more sense to do it this way. I will attach a new patch.

Will -moz-opacity changed after the fix back to opacity ?? And what is with the other bug (stop redrawing by false JavaScript)? Should I make a new sample and Bug Report?

Note that I hoisted the assertion and removed the fall through to "default:". I also simplified the expression based on the assumption that the two values are not equal. (Of course, thing still work safely even if the assumption is violated, we just get a superfluous frame change.) Alexander, "-moz-opacity" is not going to be changed back to "opacity" because "opacity" is not a standard CSS property. It may become standard in the future, but obviously we don't know if or when that will happen. There are various bugs related to opacity and widgets. Please have a good look through Bugzilla and see if your one has already been reported.

Robert, your patch looks great (I like your version much better, btw.) Thanks, and r=attinasi

ok, it works with Mozilla/5.0 (Windows; U; Win98; en-US; m18) Gecko/2000110904 File updated: http://www336.l4.xodox.com/radio/test2.html

r=buster. the only thing I don't like is forcing a reframe when all we need is a manipulation of the view. This is horribly inefficient. It will generally cause a reframe of all children of the frame that is being targeted, which is potentially very expensive. Please submit a perf bug about this issue. We need to invent the NS_STYLE_HINT_VIEWCHANGE you hint at in the patch.

Implementing such a VIEWCHANGE thingy would be rather difficult. It would require inserting a view into the middle of the view hierarchy.

Darn, I droppped this on the floor. Marc, is your review still good?

Robert, my r= has no statute of limitations ;) Assuming it still works, that is...

I will attach a new patch, retargeted to the files in 'content' but otherwise unchanged. Given that Marc's r= is still valid, I'd like sr= from waterson and then I'll check it in :-).

*** Bug 92282 has been marked as a duplicate of this bug. ***

Kevin/Waterson: please s/r and comment on [buster 2000-11-16 12:22].

So, ah..., there's a patch and it has been there since 2001-03-27 with an r=attinasi and a request for s/r=waterson. What's the holdup? I suppose if we are going to sit of this patch indefinitely, the workaround is instead of using an upper value of 100% to, rather, use an upper value no greater than exactly 99.9999961853% for DOM modifications of -moz-opacity (style.MozOpacity) to work. Jake

I might've missed this: sometimes I go through bug mail pretty quickly. Anyway, is the patch still current? If so, sr=waterson.

The patch was obseleted by one or more style system reorgs. New patch coming up.

Please re-r/sr. Thanks!

Actually, I'd feel more comfortable if dbaron or hyatt could r= this one (to go along with attinasi's sr=).

OK, I'll ping them.

Comment on attachment 65730 [details] [diff] [review] Updated patch r=dbaron, although it would be good to put a comment in nsCSSPropList.h (at both changes) that you really mean VIEWCHANGE. Also, nsStyleVisibility::CalcDifference could be written a little more cleanly if you do an if/else inside the first part that you changed (which would mean you wouldn't have to change the second part you changed at all)

The first test in CalcDifference() tests more than just mOpacity != aOther.mOpacity, so I don't think it suffices to put the rest of the function into an else clause.

MISFIRE sorry

Oops. Never mind the CalcDifference comment.

Chris, since Marc is apparently out of town I would be grateful if you could sr or at least rs this.

Comment on attachment 65730 [details] [diff] [review] Updated patch sr=waterson

dbaron, didn't you mean "avoid else after return and invert the if condition to avoid a gratuitous cvs diff line"? I.e., instead of - return NS_STYLE_HINT_NONE; + if (mOpacity == aOther.mOpacity) + return NS_STYLE_HINT_NONE; + else + return NS_STYLE_HINT_VISUAL; write it this way: + if (mOpacity != aOther.mOpacity) + return NS_STYLE_HINT_VISUAL; return NS_STYLE_HINT_NONE; /be

Checked in.

Marking FIXED

ComputeChangeHint resulted in: roc+ (188 warnings) 1-188. content/html/style/src/nsCSSParser.cpp:3356 (See 1st of 188 warnings in build log)Enumeration value `eCSSProperty_UNKNOWN' not handled in switch It appears you're using a switch (foo){case bar: {/*...*/}} where a simple if would suffice: if (foo == bar) {{/*...*/}}. If you expect to use the switch for other cases, please add a default, otherwise can we switch to if ()?

Add a default. You write the patch, I'll review it :-).

sorry, there's an else after return in the neighborhood and if brendan saw me patching that he'd probably ask me why i left it alone (after asking why i bothered spending any time on this at all)

Comment on attachment 68370 [details] [diff] [review] add default Personally I think "else after return" is Just Fine; it promotes a more functional style of programming. But if it makes Brendan happy :-)

Comment on attachment 68370 [details] [diff] [review] add default sr=alecf

Comment on attachment 68370 [details] [diff] [review] add default I checked this in. Within two cycles the warning count dropped by 182. The odd thing is that in the interim cycle, I gained the blame for those warnings. By the second cycle I couldn't find them anymore.

Should this bug be reopened? The reason is that bug 66022 was reopened with this comment from Rick Potts ( bug 66022, comment 25 ). I'm reopening this bug :-( Currently, the attached test case causes mozilla to hang because the onLoad handler for the image is fired *each* time the 'MozOpacity' style is changed. What's happening is that image onLoad handlers are being incorrectly fired each time the associated image frame is recreated (due to style changes)... -- rick It really would suck if, after we spent so long waiting for a fix and finally getting one, MozOpacity didn't work in Mozilla 1.0! Jake

Bug not yet reopened -- what's the scoop? (roc, C isn't functional, neither is C++, but small lapses in else-after-return intolerance are ok -- the usual bad pattern, however, leads to massively overindented Pascal-ese with the common case buried 9 else-if's down and off the right margin :-). /be

Today I realized the modification of MozOpacity property is not working for elements that are inline and with style defiintion without position: attribute. When I add position:relative when it works. I am wondering if you know about this regression... I see the regression in asa's slide presentations and I am helping him creating the workaround for now.

Note. the element is a span. Does not work via markup of via DOM. I created this simple testcase from asa's presentation. If you believe this is another regression not related with this bug, please let me know. This is an important regression at this point in the game and it's branch.

opacity is fundamentally broken and is not a priority for 1.0.

Robert, do you think this one is a regression? or is another bug? (since opacity does not work with simple markup).

I'm not sure that opacity on inline elements has ever really worked properly, so I'm not sure if this is a regression or an old bug.

Unless I'm missing something, this is definitely a regression. It worked fine after the patch for this bug was committed. The reason why I brought it up is that testcases that the patch for this bug fixed are no longer working. How is that not a regression? Jake

see bug 202028, to see behaviour of '-moz-opacity'