Opened 7 years ago

Closed 7 years ago

#9800 closed Bug (fixed)

Float Panel for Combo Box doesnt listen to resize event

Reported by: Gary Gilbert Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.0.1
Component: UI : Floating Panel Version: 4.0
Keywords: Cc:

Description

reproduceable in 3.6.2 also

with style or format dropdowns open, resize the window. The position of the dropdown (float panel) is not updated.

This problem is annoying when ckeditor is initialized in a jquery window that enables repositioning, the panel remains floating in the middle of no-where.

suggest closing dropdown on any window/document click event the same as a "normal" combo-box would.

Attachments (3)

chrome.jpg (166.6 KB) - added by Gary Gilbert 7 years ago.
Reproducable on Chrome
firefox.jpg (110.4 KB) - added by Gary Gilbert 7 years ago.
Reproducable on FF 17.0.1
winFFwtfRandomPanelResisting.ogv (1.4 MB) - added by Olek Nowodziński 7 years ago.

Download all attachments as: .zip

Change History (16)

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

Milestone: CKEditor 4.0.1
Status: newconfirmed

I couldn't reproduce this on Chrome on FF. Reproducible on IEs.

comment:2 Changed 7 years ago by Piotrek Koszuliński

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

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

Milestone: CKEditor 4.0.1CKEditor 4.0.2

Initially I thought that this issue is caused by some easily fixable bug in editor blur/resize listener or new listener on window resize.

Unfortunately it's more complex. New listener on window resize is indeed needed, because IEs don't blur editor when resizing window and don't fire any mouse event. But this isn't of course enough - we need to handle cases that could result in editor changing its position according to document.

  1. Editor position changed programmatically - impossible to handle in decent way (other than setInterval). Perhaps we should implement editor's method for notifying about that.
  2. Editor position changed by resizing by mouse widget like jQuery UI dialog (what should blur editor, but doesn't do that on any browser, perhaps due to prevDef() called on handler).
  3. When editor is inside scrollable box its position can be changed when scrolling this box - like 1.
  4. Perhaps more cases are possible.

So handling all of that is basically impossible/very hard. Thus, I'm postponing this ticket, because it doesn't fit in 4.0.1 milestone which is already set.

Changed 7 years ago by Gary Gilbert

Attachment: chrome.jpg added

Reproducable on Chrome

Changed 7 years ago by Gary Gilbert

Attachment: firefox.jpg added

Reproducable on FF 17.0.1

comment:4 Changed 7 years ago by Gary Gilbert

As shown in my two attachments, this problem is not restricted to MSIE. The issue is that the float panel is attached to the end of document body, or according to the PanelDefinition the parent, which never seems to be anything but document.body

The position of the element is not changed in relation to the change of editor (which in my case does not change) but in relation to the document body.

The "Simplest" fix, in my opinion, is to close the dropdown when the element loses focus instead of trying to do any fancy repositioning, besides closing on lost focus is a "normal" behavior of a combo box.

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

Milestone: CKEditor 4.0.2CKEditor 4.0.1

Hmmm. I was testing this on Linux and it turns out that on Linux, in contrast to Windows and MacOS, blur is fired on float panel when resizing window by dragging border. That's why it was working for me. On other OSes (and on Linux when maximizing window) element doesn't lose focus when window is resized. So listening on blur is not enough (we do this already https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/floatpanel/plugin.js#L191).

So back to 4.0.1. We'll add window resize listener to target most common cases. More complex cases, which I mentioned in comment:3 need new ticket.

comment:6 Changed 7 years ago by Piotrek Koszuliński

Status: assignedreview

Pushed git:9a47c5d on review.

Continuation on #9803.

comment:7 Changed 7 years ago by Olek Nowodziński

Status: reviewreview_failed

1: CKEDITOR.document.getWindow().equals( doc.getWindow() ); // true

2: bar.on( 'foo', this.hide, this ); is straightforward for other people and your method doesn't save much bytes (if any).

Another thing is that this feature can be tested by opening a panel and observing if it's gone on several events. Regarding editor mode and resize: piece of cake to trigger. However, to create synthetic window resize event, document.createEvent can be used (cross-browser code in url). Even if it doesn't work with old IEs (research needed), it's enough to have it tested in modern browsers.

comment:8 Changed 7 years ago by Piotrek Koszuliński

Status: review_failedreview

Pushed code simplification and tests for opening and closing float panel.

Changed 7 years ago by Olek Nowodziński

comment:9 Changed 7 years ago by Olek Nowodziński

Status: reviewreview_failed

Tests are awesome. However I spotted a random issue that I'm able to reproduce on Win only with IE9, latest FF and IE7. See: attachment:winFFwtfRandomPanelResisting.ogv

comment:10 Changed 7 years ago by Piotrek Koszuliński

It's not random and not Win only. Second opened float panel isn't properly closed :(.

comment:11 Changed 7 years ago by Piotrek Koszuliński

Status: review_failedreview

Ha! :) My first version was correct, but I forgot to comment why. We need that floatPanel#hide wrapper due to limitations of events implementation. It's not possible to attach one function twice on the same event to the same object. And floatPanel#hide is in prototype chain so it's shared between all instances.

Pushed revert and test.

comment:12 in reply to:  11 Changed 7 years ago by Olek Nowodziński

Status: reviewreview_passed

Replying to Reinmar:

Ha! :) My first version was correct, but I forgot to comment why. We need that floatPanel#hide wrapper due to limitations of events implementation. It's not possible to attach one function twice on the same event to the same object. And floatPanel#hide is in prototype chain so it's shared between all instances.

Pushed revert and test.

Well done.

comment:13 Changed 7 years ago by Piotrek Koszuliński

Resolution: fixed
Status: review_passedclosed

Masterised git:979136f.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy