57307 - Remove moz-opacity property from the branch because it can result in crashes

Status: VERIFIED WONTFIX

(Core :: DOM: Editor, defect, P1)

Description

[Kevin McCluskey (gone)]

There are several bugs where setting moz-opacity causes crashes; bug 57223, bug
54631.

Rather, than trying to track down and fix all of the opacity problems for RTM,
lets just remove the property for RTM.

If we don't remove the property we have effectively killed the moz-opacity
property because content authors will avoid it the current and future versions
of the browser because it causes crashes.

No one is using the property.

Comment 1

[Kevin McCluskey (gone)]

Nominated for rtm. CC'ing attinasi, ekrock.

Comment 2

[Kevin McCluskey (gone)]

Setting priority to P2.

Comment 3

[karnaze (gone)]

Adding [rtm need info] and changing to P1 (there are several pages that crash 
using opacity).

Comment 4

[ekrock's old account (dead)]

Are those the only two known crashers? Do we have any idea how hard they would
be to fix? Hypothetically speaking, if there were only two crashers total and
they were easy to fix, I'd prefer to do that & keep the property than file one
bug to remove it.

However, if they are hard to fix or believed to be only the tip of the iceberg
(e.g. if we have evidence that opacity is an inherently fragile area of the
code), then pulling -moz-opacity seems to be the responsible thing to do. We
could fix by "ns601".

cc:ing mgalli for input about crash frequency based on his experience using
-moz-opacity experimentally at geckonnection.com.

Comment 5

[/\/\arcio Galli]

Usually online demostrations flavored with opacity are okay and running without 
crash at Geckonnection. However in my vision an important information is that:

Usually I have some crashes when dynamically changing opacity value in 
demonstrations that also combines with other operations: like using PNG+alpha, 
etc. My mission at geckonnection is to avoid the crashes by hidding piece of 
code that crashes. So published demos are only the very light demos. 

I believe the use of opacity feature must be polished and perfect and different 
kind of crazy demos should be running before we can say: go ahead web 
developers, use -moz-opacity. Web designers+developers are expecting this 
feature. If the feature is available to the use, all kind of JS codes using 
opacity will be done, so some new crashes that we are not expecting may result. 

I am afraid that fixing those two, we may have new ones. So a future fix with 
more tests is my suggestion for now. 

Comment 6

[Kevin McCluskey (gone)]

I fixed a crasher related to opacity a few-weeks ago. The two remaining crasher
bugs are difficult to reproduce. They crash on some machines and not others,
even though the machines have been set to the same display depth. I don't feel
confident that we can fix the two crashers plus test using opacity on all three
platforms at all of the available screen depths on multiple machines.

Comment 7

[ekrock's old account (dead)]

OK, let's drop -moz-opacity for the first release and put it in for the next NS 
release. Marking ns601.

Comment 8

[Pierre Saslawsky]

Removing ns601, this bug should be rtm or wontFix. The other 2 bugs should be 
ns601.

There is a little problem. The property is used in html.css for two pseudos 
(:table-outer and :-moz-anonymous-positioned-block), and in the different 
implementations of EdImageMapPage.css.  We can ignore the pseudos because they 
set "-moz-opacity: inherit" but I have some concern about the Editor files 
because they set the opacity to 0.99 instead of the default value of 1.0 and I 
have no idea why. I'll ask Ben and Pavlov who seem to be the original authors.

See http://lxr.mozilla.org/seamonkey/search?string=moz-opacity

Comment 9

[ekrock's old account (dead)]

Duh, I wasn't thinking straight. Thanks Pierre. Right, this should be rtm, and 
if this makes rtm, then we should create a new bug to restore moz-opacity and 
mark that ns601.

Comment 10

[Pierre Saslawsky]

CCD Ben and Pav. I sent them a mail yesterday asking about EdImageMapPage.css but 
I got no response.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/themes/blue/editor/
EdImageMapPage.css&rev= 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/themes/classic/editor/mac/
EdImageMapPage.css&rev= 

Comment 11

[Stuart Parmenter]

ask ben.  I simply checked "blue" in as a copy of modern at the time, I don't
know anything about this.

Comment 12

[Pierre Saslawsky]

Ok, nobody knows why we set the opacity to 0.99 in the different copies of 
EdImageMapPage.css. The code was sitting somewhere and it got 
copied over several times.  I assume it can be removed, I don't see any 
reason for having something 99% opaque instead of 100%.

Now, we still have a problem with the ImageMap Editor dialog because 
the highContrast() function changes the opacity. See the code at:
http://lxr.mozilla.org/seamonkey/source/editor/ui/dialogs/content/EdImage
Map.js#156

The interesting thing is that I tried to activate this code but it looks like the 
Contrast button in the ImageMap dialog doesn't work at all.  So apparently 
it wouldn't be a big loss to disable the property.

Reassigned to the Editor team to tell us if and how the Contrast function 
works, and if removing the button would be acceptable to them.

Please reassign to me after investigation.

Comment 13

[Pierre Saslawsky]

Marcio Galli sent me the following note:
----
I remember that in some of my old demos I was using .99 opacity instead 1/default 
value. I don't remember with accuracy but I think it was something related to the 
fact that when opacity was 1, it was not rendering correctly or crashing in demos 
that set the opacity dynamically.

Comment 14

[rubydoo123]

reassign to brade for review and determination as to rtm setting (should this be
rtm-?) and just get it in the trunk?

Comment 15

[Kathleen :Brade]

Pierre--you can go ahead with the changes you need to make to the image map
editor (removing the moz-opacity property from the css files). You may also
comment out the relevant lines within the JS function you mention
(highContrast()) which manipulates the opacity.  Dan, Brian and I agreed that
it's probably for the best of the product overall.

Reassign to Pierre per his request from way back...

Comment 16

[Pierre Saslawsky]

PDT doesn't take bug fixes anymore and since the crashes (bug 57223 and bug 
54631) will probably be fixed for the next release, we can close that one as 
WontFix.  And if they are not, we'll reopen that one.

I opened bug 58501 in relation to this bug.

Comment 17

[sujay]

verified in 10/30 build.