Closed Bug 33601 Opened 24 years ago Closed 23 years ago

DOM modification of opacity property doesn't work

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Platform:
x86
Windows 98
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future

People

(Reporter: Chris244, Assigned: roc)

References

Details

(Keywords: polish)

Attachments

(5 files, 4 obsolete files)

Example of opacity change failing to update display
519 bytes, text/html
Details
Fixed test case to use -moz-opacity
568 bytes, text/html
Details
Testcase for "background-attachment"
2.28 KB, text/html
Details
Updated patch
5.20 KB, patch
dbaron
: review+
waterson
: superreview+
Details | Diff | Splinter Review
Simple testcase demonstrating opacity not working.
307 bytes, text/html
Details 
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.
Assignee: pierre → attinasi
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Target Milestone: --- → M17
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.
Keywords: nsbeta2
Putting on [nsbeta2-] radar. Not critical to beta2. Adding nsbeta3 keyword
Keywords: nsbeta3
Whiteboard: [nsbeta2-]
Target Milestone: M17 → M20
Keywords: polish
[nsbeta3-] and Future.
Whiteboard: [nsbeta2-] → [nsbeta2-][nsbeta3-]
Target Milestone: M20 → 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!
Assignee: attinasi → roc+moz
Status: ASSIGNED → NEW
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.
Status: NEW → ASSIGNED
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.
Keywords: mozilla1.0
QA Contact: chrisd → ian
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 :-).
Priority: P3 → P2
Whiteboard: [nsbeta2-][nsbeta3-]
*** 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.
Attached patch Updated patch — — Splinter Review 
Please re-r/sr. Thanks!
Attachment #19009 - Attachment is obsolete: true
Attachment #19021 - Attachment is obsolete: true
Attachment #29000 - Attachment is obsolete: true
Actually, I'd feel more comfortable if dbaron or hyatt could r= this one (to go
along with attinasi's sr=).
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)
Attachment #65730 - Flags: review+
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.
Assignee: roc+moz → dbaron
Status: ASSIGNED → NEW
MISFIRE sorry
Assignee: dbaron → roc+moz
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
Attachment #65730 - Flags: superreview+
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
Marking FIXED
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → 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 :-).
Attached patch add default (obsolete) — — Splinter Review 
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 :-)
Attachment #68370 - Flags: review+
Comment on attachment 68370 [details] [diff] [review]
add default

sr=alecf
Attachment #68370 - Flags: superreview+
Attachment #68370 - Attachment is obsolete: true
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'
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: