Opened 9 years ago
Last modified 7 years ago
#13604 review Bug
Notifications are not visible when editor is placed in a absolutely positioned div with high z-index
| Reported by: | Jonathan | Owned by: | Tade0 |
|---|---|---|---|
| Priority: | Nice to have (we want to work on it) | Milestone: | |
| Component: | General | Version: | 4.5.0 |
| Keywords: | Cc: |
Description
We have some popups (absolutely positioned divs) that have some high z-indexes (1001). The notifications that display in the editor when uploading a file appear to display behind the divs. What's more is that the if I move the absolutely positioned div, the notification does not follow (see attached image).
Attachments (1)
Change History (25)
Changed 9 years ago by
| Attachment: | Notification.png added |
|---|
comment:1 Changed 9 years ago by
| Milestone: | → CKEditor 4.5.3 |
|---|---|
| Status: | new → confirmed |
| Version: | 4.5.1 → 4.5.0 |
comment:2 Changed 9 years ago by
| Owner: | set to Tade0 |
|---|---|
| Status: | confirmed → assigned |
comment:3 Changed 9 years ago by
Proposed solution: Attaching the notification area to the editor's parent instead of the body.
Works on all major browsers and doesn't seem to break anything.
Changes pushed to branch:t/13604.
comment:4 Changed 9 years ago by
| Status: | assigned → review |
|---|
comment:5 Changed 9 years ago by
| Status: | review → review_failed |
|---|
What if editor is moved from one place in the DOM to another place? It's a popular scenario that editor can be DnDed.
Another thing - what if editor's container has some specific styles, like position, overflow, etc. What if the editor is positioned absolutely or relatively itself? What if editor is maximised? Etc. There are a lot of cases to check. Of course, they would need to be checked in both solutions. We would need more manual tests to assure that this solution will work.
comment:6 Changed 9 years ago by
PS. I know that none of the solutions will be perfect and will consider all possible cases as there are millions of them. However, one may be safer than another, so I would like to know which is it. I pinged @a.nowodzinski too.
comment:7 Changed 9 years ago by
I pushed branch:t/13604b, which slightly extends manual test from branch:t/13604.
I see the following, separate cases:
- Absolutely (relatively) positioned popup of high
z-index. - Notifications don't follow an absolutely positioned popup (any parent of
editor.container), while it's moving. - Editor (
editor.container) is positioned absolutely or relatively (could also be moving). - Editor is (gets) maximized when combined with above.
As for 1. baseFloatZIndex is the right answer.
As for 2. we got to provide some config option (like config.notification_container = {String|DOM object}) to let developers control the location of notifications in DOM. At the moment it is <body>, but some people might want to use a different container, i.e. so their notifications follow the popup, which is absolutely positioned. Unfortunately it means radical (?) changes in positioning algorithm so it dynamically adapts to different configurations.
As for 3. -> see 2. We could implement pre–defined values for config.notification_duration like body (default) and inner (inside editor.container).
As for 4. it needs research. I checked manual tests for notification plugin and it worked with maximize (didn't look under the hood) but I noticed that the integration is broken for static instances in branch:t/13604b (http://tests.ckeditor.dev:1030/tests/tickets/13604/1).
As for
what if editor's container has some specific styles, like position, overflow, etc.
there's nothing we can do except to give the freedom of choice to developers (2.). And if we do so, I consider most of the real–life problems with notifications solved. Surely, there will be edge cases but for most of the integrations the desired UX level will be preserved.
WDYT? Let's discuss it first to avoid bad implementations.
comment:8 Changed 9 years ago by
| Milestone: | CKEditor 4.5.3 → CKEditor 4.5.4 |
|---|
comment:9 Changed 9 years ago by
As for 2. I think this config option should be described as a selector, because this way it's possible to avoid modifying it at runtime.
comment:10 Changed 9 years ago by
Additionally note that notifications are placed in the body the same way floating toolbar is. So it is very likely that these issues are more generic and more generic changes will be needed.
comment:11 Changed 9 years ago by
I've done a search and found that there are at least four plugins that do this.
comment:12 Changed 9 years ago by
| Status: | review_failed → review |
|---|
I've created a draft implementation that seems to work(tried it on Chrome on all manual tests that mention notifications).
Changes pushed to branch:t/13604a.
comment:13 Changed 9 years ago by
As for 2. we got to provide some config option (like config.notification_container = {String|DOM object}) to let developers control the location of notifications in DOM. At the moment it is <body>, but some people might want to use a different container, i.e. so their notifications follow the popup, which is absolutely positioned. Unfortunately it means radical (?) changes in positioning algorithm so it dynamically adapts to different configurations.
Additionally note that notifications are placed in the body the same way floating toolbar is. So it is very likely that these issues are more generic and more generic changes will be needed.
Instead of trying to be too smart we should allow other developers tinkering with this logic. If someone needs super custom placement and styling he/she can use shared spaces (for the toolbar) or reimplement the notification system using the events that we exposed.
So KISS. We're doing fine here and we should not try to complicate this too much.
comment:14 Changed 9 years ago by
True. See "Notification Integration" section in: http://docs.ckeditor.com/#!/guide/dev_notifications
comment:15 Changed 9 years ago by
| Milestone: | CKEditor 4.5.4 → CKEditor 4.5.5 |
|---|
comment:16 Changed 8 years ago by
| Milestone: | CKEditor 4.5.5 → CKEditor 4.5.6 |
|---|
comment:17 Changed 8 years ago by
| Milestone: | CKEditor 4.5.6 → CKEditor 4.5.7 |
|---|
comment:18 Changed 8 years ago by
| Milestone: | CKEditor 4.5.7 → CKEditor 4.5.8 |
|---|
comment:19 Changed 8 years ago by
| Milestone: | CKEditor 4.5.8 → CKEditor 4.5.9 |
|---|
comment:20 Changed 8 years ago by
| Milestone: | CKEditor 4.5.9 → CKEditor 4.5.10 |
|---|
comment:21 Changed 8 years ago by
| Milestone: | CKEditor 4.5.10 → CKEditor 4.5.11 |
|---|
Moving tickets to the next milestone.
comment:22 Changed 8 years ago by
| Milestone: | CKEditor 4.5.11 → CKEditor 4.6.1 |
|---|
comment:23 Changed 7 years ago by
| Milestone: | CKEditor 4.6.1 → CKEditor 4.6.2 |
|---|
Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.
comment:24 Changed 7 years ago by
| Milestone: | CKEditor 4.6.2 |
|---|---|
| Priority: | Normal → Nice to have (we want to work on it) |
I haven't checked, but I'll confirm this ticket because it's very likely that such issue can exist.
As for the z-index, I think notification should use http://docs.ckeditor.com/#!/api/CKEDITOR.config-cfg-baseFloatZIndex
As for positioning... we'll see. This may be very tricky.