OpenLayers.Element.getDimensions method should not set style.display property to none

[bbinet]

The bug appears when an OpenLayers.Element has a computed style display value set to none, and when this value comes from a css class and not from its own style attribute.

In this case, a call to OpenLayers.Element.getDimensions leads to setting the own style attribute display to none for the element.

[bbinet]

patch_2414-r9943.diff fixes the bug.

[bbinet]

This needs more work since els.display = ; does not enable the element because it does not change the computed style display value (still equals to none).

[ahocevar]

@bbinet: the patch fixes the reported bug, but it does not help us find the dimensions of the element. If this came up because Map::updateSize failed, then this is fixed already (see #2461).

Please commit.

[tschaub]

Bruno is not listed on CLA.

[bbinet]

Replying to ahocevar:

@bbinet: the patch fixes the reported bug, but it does not help us find the dimensions of the element. If this came up because Map::updateSize failed, then this is fixed already (see #2461). Please commit.

I'll see if #2461 fix the problem, and I'll report back.

[bbinet]

Replying to tschaub:

Bruno is not listed on CLA.

My name is now listed on CLA.

[bartvde]

Bruno, any news on this one?

[bbinet]

Replying to bartvde:

Bruno, any news on this one?

Sorry, I haven't taken the time to check this one. I'll do it tonight and report back.

[bbinet]

patch_2414-r10113.diff gives up the previous non completely working patch and remove the entire workaround which tries to guess the size of hidden elements (size of display:none elements is 0).

As explain in thread http://openlayers.org/pipermail/dev/2010-January/005612.html , this new patch makes the application responsible for manually calling updateSize when display:none is removed from the map div.

This new patch does not fix the reported bug, so I'm fine with creating a new ticket, if needed.

If you think we must stay with the current behavior and guess the size on display:none element, then I don't see any good solution. We could force display:block and then restore the original display property (but I don't like the idea of making temporary changes to the DOM only to get the size of an element).

[bbinet]

I forgot to remove the corresponding tests.

patch_2414-r10113.2.diff is the same as previous patch but remove also the tests.

[fredj]

Since Element.getDimensions is an API function, could you mark it as deprecated and keep the code ? Removing the test is fine.

[bbinet]

Yes, you're right Fred.

patch_2414-r10113.3.diff is same as previous but does not remove the getDimensions API function which is now marked as deprecated.

[fredj]

Thanks, please commit.

[bbinet]

I don't have rights to commit in openlayers trunk, so please commit ;)

[fredj]

(In [10125]) mark Element.getDimensions as deprecated and remove the workaround in Map.getCurrentSize. p=bbinet, r=me (closes #2414)

[ahocevar]

Possible regression. See http://openlayers.org/pipermail/dev/2010-May/006005.html

[ahocevar]

attachment: patch_2414-r9943.diff would have fixed the reported issue. I am in favor of reverting r10125 and committing the first patch instead. It may not fix the application issue of the ticket creator, but it fixes the issue described in the ticket summary.

[bartvde]

Andreas, your proposal makes sense to me, but I want to hear back from one of the C2C people first before setting the state to commit.

[elemoine]

2414.patch doesn't solve the issue. Andreas, we agree with your proposal. We'll open a new ticket for the remaining issues.

[ahocevar]

(In [10276]) reverted r10125, which does not fix the described issue, and store style.display instead (first patch attached to #2414). r=bartvde,elemoine (pullup #2414)

[ahocevar]

maybe there was a misunderstanding. quietriver is running more tests right now.

[quietriver]

Correct minimal test case, the previous test case was invalid as it did not include a maxExtent. This includes getCurrentSize to show the inclusion of 2414.patch fixing the issue described

[bartvde]

r10276 has already been pulled up to the 2.9 branch in r10277

[ahocevar]

(In [10278]) now don't use getDimensions any more, but use offsetWidth and offsetHeight before like getDimension does if style.display is not none. r=elemoine,quietriver (pullup #2414)

[bartvde]

(In [10279]) pullup r10278 to the 2.9 branch (closes #2414)