Opened 13 years ago
Closed 10 years ago
#6058 closed bug (fixed)
Dialog overlays are not properly reused when multiple instances of a Dialog exist
Reported by: | emeerson | Owned by: | emeerson |
---|---|---|---|
Priority: | minor | Milestone: | 1.10.0 |
Component: | ui.dialog | Version: | 1.8.2 |
Keywords: | overlay | Cc: | [email protected]…, [email protected]… |
Blocked by: | Blocking: |
Description
When multiple dialog instances exist at the same time (but one may be hidden from view), the internal overlay oldInstances stack attempts to reuse a previous overlay but does not make the overlay properly visible.
Removing oldInstances.pop() from line 6143 alleviates this issue. Since it is implemented for addressing a browser-specific deficiency (memory-leak, no less), should this be implemented for all browser(s) & versions (while I understand the mantra of feature checking, not browser checking, this is not an issue of Browser standards or feature support, but a straight-up memory leak in IE).
Change History (12)
comment:1 Changed 12 years ago by
Component: | ui.core → ui.dialog |
---|
comment:2 Changed 12 years ago by
Cc: | [email protected]; [email protected] → [email protected], [email protected] |
---|---|
Keywords: | overlay added |
comment:3 Changed 10 years ago by
Milestone: | TBD → 1.10.0 |
---|
comment:4 Changed 10 years ago by
Resolution: | → duplicate |
---|---|
Status: | new → closed |
comment:5 Changed 10 years ago by
Resolution: | duplicate |
---|---|
Status: | closed → reopened |
I don't think these are related. One is a feature request and one is a memory leak bug.
comment:6 Changed 10 years ago by
Status: | reopened → open |
---|
comment:7 Changed 10 years ago by
Owner: | set to emeerson |
---|---|
Status: | open → pending |
Needs to be reproduced, state should be "new", but can't set that since it was reopened. I don't know what "(but one may be hidden from view)" means here - closed dialog, manually hidden?
comment:8 Changed 10 years ago by
The reuse was added to address the memory leak in IE 6/7/8 reported in #5185. I'm wondering if we should just get rid of the reuse now that we're using fixed positioning for overlays, so they should be much smaller and therefore use less memory.
comment:9 Changed 10 years ago by
It the memory usage is still a problem, which we'd need to verify, we could remove the opacity from the overlay, for IE<=8.
comment:10 Changed 10 years ago by
It seems like people are willing to deal with some level of memory leak from this. I'd actually be fine with just getting rid of the workaround, leaving the opacity in the theme and telling people to override the styles if they care about the leak.
comment:11 Changed 10 years ago by
Status: | pending → open |
---|
Now would be a good time to challenge a few assumptions. Removing the workaround will surely address this issue, whatever it actually is.
comment:12 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | open → closed |
Dialog: Remove the instance-storing for the overlay, just create one whenever it is needed. Heavily simplifies the code, while the memorly leak should be hardly an issue anymore, since fixed positioning restricts the overlay size to the window dimensions. Fixes #6058 - Dialog overlays are not properly reused when multiple instances of a Dialog exist.
Changeset: 1e8baf568365f8edc833439315f76e5efe1ba9b6
Duplicate of #5901 .
Seems to be a duplicate/related issue to #5901