set opacity on layer divs, not on tiles and markers

[ahocevar]

Currently, the setOpacity method is quite expensive, because it sets the opacity on all tiles or markers. It is sufficient to set it on the layer div. This is another improvement factored out from #3061. It does not depend on any other open tickets.

[ahocevar]

All tests pass, opacity.html example works. Thanks for any review.

[fredj]

handle initial opacity == 0.0

[fredj]

attachment:"openlayers-3431.1.patch" changes the initial opacity test to handle opacity == 0.0 (if (this.opacity != null)) instead of if (this.opacity).

[fredj]

tested on several projects using chrome 12 and FF5, please commit if you agree with my changes.

[ahocevar]

Thanks @fredj for the improvements. I fully agree.

[ahocevar]

(In [12185]) Set opacity on layer div, not on tiles and markers. r=fredj (closes #3431)

[fredj]

In OpenLayers.Tile.Image and OpenLayers.Tile.Image.IFrame, layer.opacity is still used to initialize the image div.

[ahocevar]

True. But it won't be any more after #3419 is in :-).

[ahocevar]

Created a new ticket for this (#3455). Closing this one.

[fredj]

The opacity example no longer works with IE8:  http://www.openlayers.org/dev/examples/layer-opacity.html

I don't think this is related to #3455

[erilem]

fredj, is this a regression? Should we change Milestone to 2.11?

[ahocevar]

bring opacity back on IE8

[ahocevar]

We can either revert r12185, or apply opacity.patch . Please review and/or let me know what to do.

[ahocevar]

Setting milestone to 2.11 because this is a regression from 2.10.

[ahocevar]

Maybe an explanation about opacity.patch is in order: in IE8, the filter is not applied to positioned child elements (like in our case, where style.position is set to absolute). And in addition, the filter needs to be applied after an image is loaded. So we use a loadend listener and just set the filter again, to make sure that it gets applied. In css, setting a width and height in pixels for the layer div is also necessary, because otherwise the children won't render at all. The whole workaround is only necessary for IE8.

[erilem]

Replying to ahocevar:

Maybe an explanation about opacity.patch is in order: in IE8, the filter is not applied to positioned child elements (like in our case, where style.position is set to absolute). And in addition, the filter needs to be applied after an image is loaded. So we use a loadend listener and just set the filter again, to make sure that it gets applied. In css, setting a width and height in pixels for the layer div is also necessary, because otherwise the children won't render at all. The whole workaround is only necessary for IE8.

Thanks Andreas for researching the issue, and providing an explanation for it. I'm in favor of reverting r12185, to avoid introducing some IE8-specific hackish code.

[ahocevar]

like above, but only sets css class when opacity is set

[ahocevar]

Eric, are you sure? This doesn't look like a big loss now, because the opacity handling code is still in Tile.js and its subclasses. But the patch for #3419 would get bloated again significantly by bringing opacity handling back to the tile level. I'm just thinking a hack that does exactly what's needed for a specific browser bug is better than unnecessary dom manipulation in all browsers. But if you say once more that you want me to revert, I'll go ahead and do it, and update the patch for #3419.

[cmoullet]

Hi, Thanks for this patch. I just applied it in our projects and it solves the issue with IE8. I can understand the concerns of Eric but we face here a bug in IE8 and I also prefer to make specific workaround instead of risking to affect other browsers. I'm wondering if it's still needed to set the opacity on the img level:  http://trac.osgeo.org/openlayers/browser/trunk/openlayers/lib/OpenLayers/Tile/Image.js#L394 I just tested to deactivate this part and unfortunately IE8 opacity doesn't work anymore. But keeping this code means that we don't have a real benefit to set the opacity on the "div layer level". Cédric

[erilem]

Replying to cmoullet:

Hi, Thanks for this patch. I just applied it in our projects and it solves the issue with IE8. I can understand the concerns of Eric but we face here a bug in IE8 and I also prefer to make specific workaround instead of risking to affect other browsers.

Don't get me wrong, I haven't said that I don't want the issue fixed. I'm just wondering whether we should go back to the previous way of setting the layer opacity, to avoid introducing IE8-specific code in OpenLayers.

[erilem]

Replying to ahocevar:

Eric, are you sure? This doesn't look like a big loss now, because the opacity handling code is still in Tile.js and its subclasses. But the patch for #3419 would get bloated again significantly by bringing opacity handling back to the tile level. I'm just thinking a hack that does exactly what's needed for a specific browser bug is better than unnecessary dom manipulation in all browsers. But if you say once more that you want me to revert, I'll go ahead and do it, and update the patch for #3419.

Andreas, I'm failing to understand why we'd bloat the backbuffer code by handling opacity at the tile level. Before [12185] we had a loop to set the opacity on child elements, if we go back to tile-level opacity setting wouldn't we use the same exact code as before? (I'm probably missing something here.) If you still think tile-level opacity setting would bloat the tile/backbuffer code feel free to commit your patch. Thanks.

[ahocevar]

@erilem: Thanks for the review. My concern is not about bloating the backBuffer code. It's about re-adding a setOpacity interface to Tile.js and an implementation of it for all subclasses after #3419. I'm going to commit opacity.2.patch now (with a comment that it can be removed when we drop support for IE8 :-) ). If this turns out to cause problems, we can always revisit and bring the setOpacity methods back to the tile classes after #3419.

[erilem]

Replying to ahocevar:

@erilem: Thanks for the review. My concern is not about bloating the backBuffer code. It's about re-adding a setOpacity interface to Tile.js and an implementation of it for all subclasses after #3419.

AFAICT [12185] did not remove any setOpacity function from the Tile prototype. That is why I don't understand why you're talking about re-adding it.

[ahocevar]

You're right. But #3419 removes it.

[ahocevar]

(In [12228]) IE8 specific fix for opacity. r=erilem (closes #3431)

[ahocevar]

r12228 needs to be pulled up into the 2.11 branch.

[ahocevar]

@erilem: sorry, my mistake. You're right, there is no setOpacity in Tile.js, not even before #3419. There is only this code here in Tile.Image::initImgDiv:

if(this.layer.opacity != null) {

    OpenLayers.Util.modifyDOMElement(this.imgDiv, null, null, null,
                                     null, null, null, 
                                     this.layer.opacity);
}

[erilem]

Replying to ahocevar:

@erilem: sorry, my mistake. You're right, there is no setOpacity in Tile.js, not even before #3419. There is only this code here in Tile.Image::initImgDiv: {{{ if(this.layer.opacity != null) { OpenLayers.Util.modifyDOMElement(this.imgDiv, null, null, null, null, null, null, this.layer.opacity); } }}}

Which just sets the initial opacity of the tile. Cédric just said that setting the layer opacity doesn't work for him in IE8 if this code is removed, so I'm wondering if #3419 will break IE8 again. Cédric told me he's going to do more tests with #3419 applied.

[ahocevar]

I think I'm just going to revert r12228 and r12185 - vector layers and marker layers also don't work after r12228 in IE8. Sad how one single browser can make things more complicated than they need to be.

[ahocevar]

(In [12230]) reverting r12185 (closes #3431)

[ahocevar]

Something went wrong reverting this. Fixing right now.

[ahocevar]

(In [12231]) reverting r12228 and r12185 (closes #3431)

[ahocevar]

I am going to pull this up into the 2.11 branch right now.

[ahocevar]

Everything is now back to the way it was before r12185, and r12185 was also reverted on the 2.11 branch.

I still think that this ticket is valid, so I'm reopening it. Maybe we can address this later and find a better workaround for IE8.