Opened 4 years ago

Last modified 2 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)

Notification.png (16.1 KB) - added by Jonathan 4 years ago.

Download all attachments as: .zip

Change History (25)

Changed 4 years ago by Jonathan

Attachment: Notification.png added

comment:1 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.3
Status: newconfirmed
Version: 4.5.14.5.0

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.

comment:2 Changed 4 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:3 Changed 4 years ago by Tade0

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 4 years ago by Tade0

Status: assignedreview

comment:5 Changed 4 years ago by Piotrek Koszuliński

Status: reviewreview_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 4 years ago by Piotrek Koszuliński

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 4 years ago by Olek Nowodziński

I pushed branch:t/13604b, which slightly extends manual test from branch:t/13604.

I see the following, separate cases:

  1. Absolutely (relatively) positioned popup of high z-index.
  2. Notifications don't follow an absolutely positioned popup (any parent of editor.container), while it's moving.
  3. Editor (editor.container) is positioned absolutely or relatively (could also be moving).
  4. 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.

Last edited 4 years ago by Olek Nowodziński (previous) (diff)

comment:8 Changed 4 years ago by Olek Nowodziński

Milestone: CKEditor 4.5.3CKEditor 4.5.4

comment:9 Changed 4 years ago by Tade0

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 4 years ago by Piotr Jasiun

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 4 years ago by Tade0

I've done a search and found that there are at least four plugins that do this.

comment:12 Changed 4 years ago by Tade0

Status: review_failedreview

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 3 years ago by Piotrek Koszuliński

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 3 years ago by Piotr Jasiun

True. See "Notification Integration" section in: http://docs.ckeditor.com/#!/guide/dev_notifications

comment:15 Changed 3 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.4CKEditor 4.5.5

comment:16 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5CKEditor 4.5.6

comment:17 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.6CKEditor 4.5.7

comment:18 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:19 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:20 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:21 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:22 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.11CKEditor 4.6.1

comment:23 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.

comment:24 Changed 2 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.2
Priority: NormalNice to have (we want to work on it)
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy