Opened 6 years ago

Closed 5 years ago

#9816 closed Bug (fixed)

Floating toolbar does not reposition vertically

Reported by: Jeremie Miserez Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.2.1
Component: Core : Editable Version: 4.0 Beta
Keywords: Cc: jan_wloka@…, alexander.klyuchka@…, matti.jarvinen@…, peterpham@…, chris.ingham@…, cinger_nick@…, mindaugas.staponkus@…, cnj125@…, nw@…, roel.duijnhoven@…, clakaclak@…

Description

The floating toolbar is not repositioned correctly when the layout of the page changes, e.g. because of:

  • browser window resize
  • the content above the editable div gets smaller or bigger

Steps to reproduce:

  1. Go to http://nightly.ckeditor.com/4042/samples/inlineall.html
  2. Click on the two middle blocks to show the two toolbars
  3. Remove some text from the first block
  4. Click on the second block

Expected: The toolbar should have moved a bit upwards Observed: The toolbar is on top of the text and has not moved at all, even though the editable is now at a different position (see screenshot).

Note: Focus/Blur has no effect, the toolbar re-appears at the exact same wrong position.

Reproduced on all browsers tested (Windows: Chrome 23.0.1271.95m, Firefox 10.0.11 ESR, IE8) with CKEditor 4.0 "Full" Release and Nightly v4042.

Attachments (5)

toolbar_vertical_position.png (112.0 KB) - added by Jeremie Miserez 6 years ago.
Screenshot showing the nightly rev 4042 with the bug
9602.html (1.7 KB) - added by Jakub Ś 6 years ago.
Selection_111.png (24.6 KB) - added by Piotrek Koszuliński 5 years ago.
Selection_112.png (35.3 KB) - added by Piotrek Koszuliński 5 years ago.
Selection_114.png (90.0 KB) - added by Piotrek Koszuliński 5 years ago.

Download all attachments as: .zip

Change History (42)

Changed 6 years ago by Jeremie Miserez

Screenshot showing the nightly rev 4042 with the bug

comment:1 in reply to:  description Changed 6 years ago by Jeremie Miserez

Replying to jmiserez: Screenshot showing the nightly rev 4042 with the bug

comment:2 Changed 6 years ago by Jeremie Miserez

After looking through the positioning logic in the floatingspace plugin, I think that the mode-based approach might not be ideal. Correct positioning can be achieved with much more consistent results with something like this:

if (editorRect.bottom + dockedOffsetY <= pinnedOffsetY ) {
// outside window
updatePos( 'absolute', 'top', editorPos.y + editorHeight + dockedOffsetY );
} else if (editorRect.bottom - pinnedOffsetY - spaceHeight <= spaceHeight && !(editorRect.bottom <= pinnedOffsetY)) {
// docked bottom
updatePos( 'absolute', 'top', editorPos.y + editorHeight + dockedOffsetY );
} else if (editorRect.top - dockedOffsetY - spaceHeight <= pinnedOffsetY) {
// pinned top
updatePos( 'fixed', 'top', pinnedOffsetY );
} else {
// docked top (default)
updatePos( 'absolute', 'top', editorPos.y - spaceHeight - dockedOffsetY );
}                                                                                                                                     

In addition, the position should be recalculated on both scroll and window resize events, not just scroll events.

comment:3 Changed 6 years ago by Jan Wloka

Cc: jan_wloka@… added

Subscribed.

comment:4 Changed 6 years ago by Jakub Ś

Status: newconfirmed
Version: 4.04.0 Beta

Problem can be reproduced from CKEditor 4 beta.

To reproduce:

  1. Go to middle column
  2. Click in lower div ("Praesent wisi accumsan sit amet nibh")
  3. Click in uppder div and remove some text from it
  4. Click in lower div again

comment:5 Changed 6 years ago by Jakub Ś

Possible duplicate #9899.

There is also h-scroll problem #9903.

comment:6 Changed 6 years ago by Alexander

Cc: alexander.klyuchka@… added

Subscribed

comment:7 Changed 6 years ago by Jakub Ś

#9602 was marked as duplicate.


Please note that you get the same result when adding dynamic content to page (ckeck out 9602.html)

Changed 6 years ago by Jakub Ś

Attachment: 9602.html added

comment:8 Changed 6 years ago by Matti Järvinen

Cc: matti.jarvinen@… added

Added myself to cc

comment:9 Changed 6 years ago by Peter

Cc: peterpham@… added

cc to myself

comment:10 Changed 6 years ago by Chris Ingham

Cc: chris.ingham@… added

Adding myself to Cc:

comment:11 Changed 6 years ago by Jakub Ś

#10059 was marked as duplicate.

comment:12 Changed 6 years ago by Jakub Ś

#10075 was marked as duplicate.

comment:13 Changed 6 years ago by Nick

Cc: cinger_nick@… added

cc to myself

comment:14 Changed 6 years ago by Mike Burgh

following

comment:15 Changed 6 years ago by Mindaugas

Cc: mindaugas.staponkus@… added
Version: 4.0 Beta4.1

still not fixed in 4.1 cc to myself

comment:16 Changed 6 years ago by Jakub Ś

Version: 4.14.0 Beta

Version number is used to indicate when problem stated occurring.

@minds and all other users - stop changing version numbers to show us that issue is still reproducible in latest CKEditor. We are aware of that!!!

comment:17 Changed 6 years ago by Ben Ng

Cc: cnj125@… added

Add to cc

comment:18 Changed 5 years ago by Eduro

Cc: nw@… added

Add to cc

comment:19 Changed 5 years ago by roelvd

Cc: roel.duijnhoven@… added

Add to cc

comment:20 Changed 5 years ago by roelvd

Anyone who got a temporary work around / hack?

comment:21 Changed 5 years ago by Nick

The workaround I'm using is to reload CKEditor whenever the position needs to change.

comment:22 Changed 5 years ago by roelvd

But how do you know that beforehand? Or are you listening to all sorts of events in order to do so?

comment:23 Changed 5 years ago by Nick

For me, the position changes when some divs are moved around. When this happens, I reload ALL the editors in there. Depends on your situation. Keep in mind that this reloading tends to take a second or two, depending on the number of editors.

comment:24 Changed 5 years ago by Eduro

Any news on this issue? Reloading all boxes is a pretty heavy performance hit and is not an acceptable solution :|

comment:25 Changed 5 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.2.1

comment:26 Changed 5 years ago by Claire Martinez

Cc: clakaclak@… added

added to cc

comment:27 Changed 5 years ago by Olek Nowodziński

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:28 Changed 5 years ago by Olek Nowodziński

Status: assignedreview

Created branch t/9816 with a fix:

  • Revisited vertical space positioning.
  • Replaced comments with ASCII images of the cases.
  • Re-position the space on editor's change event.
  • To simplify conditions, re-positioning is performed on each window scroll/resize and editor focus/blur.

comment:29 Changed 5 years ago by Olek Nowodziński

NOTE: #9899 is a totally different ticket, touching focus manager. It's because clicking other editor's resizer doesn't trigger blur in the first editor.

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

Status: reviewreview_failed

Repositioning on every #change is IMO too (unnecessarily) heavy. I'd add eventsBuffer (implemented in #9764) to reduce frequency of repositioning.

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

Just to know about what numbers we're talking - I made a test.

IE9 - on master I've got ~12% CPU core usage, on t/9816 it reaches even ~20% for the same typing speed. Chrome. 3% -> 5%. FF 5% -> 7%.

comment:32 Changed 5 years ago by Olek Nowodziński

Status: review_failedreview
  • Ported CKEDITOR.tools.eventsBuffer from #9764.
  • Created t/9816 branch in tests: ported tests eventsBuffer tests from #9764.
  • Used two separate eventsBuffers: .5s for change event and .1s for other events to avoid delayed re-positioning.
    • Reduced CPU load.
  • Re-factorized the entire code of floatingspace:
    • removed awkward order of variable declarations
    • improved variable/function scoping
    • improved performance by moving function definitions out of layout() body
    • fixed code style issues

If the last change seems too drastic, it can be discarded (the last commit in the branch).

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

TODO: Add @since tag to eventsBuffer doc.

Changed 5 years ago by Piotrek Koszuliński

Attachment: Selection_111.png added

Changed 5 years ago by Piotrek Koszuliński

Attachment: Selection_112.png added

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

Status: reviewreview_failed

I pushed two commits:

  1. Improvements of eventsBuffer docs.
  2. Prevent caching editable. Currently inline editor's editable is not dynamic like framed's one, but since it may change, it's safer take this into account now.

While testing I found that floating space behaves now differently than on master.

Case 1.

On master toolbar would be positioned below editable, on t/9816 toolbar is never positioned below editable.

However, I've never liked the moment when toolbar is repositioned below editable. The condition said - if visible part of editable is lower than toolbar, place toolbar under the editable. It's not optimal in cases like:

There's a lot more available space below editable, than above, so it would make sense to place toolbar below editable.

Case 2.

Toolbar is always visible, even if editable is completely hidden. On master it was pinned to editable.

I think that in this case previous behaviour was correct, because it was less confusing. Toolbar shouldn't be visible, when it cannot be associated with focused editable.

Conclusion

  • Case 2. should be fixed.
  • We can discuss what to do with case 1., but t/9816's behaviour is not correct.

Changed 5 years ago by Piotrek Koszuliński

Attachment: Selection_114.png added

comment:35 Changed 5 years ago by Olek Nowodziński

Status: review_failedreview

Updated the branch:

  • Case 1: Indeed, this is cumbersome. Now the space is always moved to the bottom of editable when it's possible, e.g. when the space would float over the editable and there's enough space below the editable to have it there.
  • Case 2: Reverted master's behavior.

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

Status: reviewreview_passed

I have tested t/9816 and I liked changes very much. Now toolbar is more often positioned below editable (what makes a lot of sense) and thanks to that it is also visible that fix for the toolbar positioning on editor#change works.

I asked also Wiktor to check new behaviour, but for now I'm R+ing.

comment:37 Changed 5 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged branch into master, dev (​git:711cec9), tests (8e4f9dd).

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