Opened 6 years ago

Closed 6 years ago

#10865 closed Bug (fixed)

Widgets: copybin is not hidden

Reported by: Olek Nowodziński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.3
Component: Core : Pasting Version: 4.3 Beta
Keywords: Cc:

Description

Because of that, some flickering can be observed while copying widgets. For example:

  1. In IE9 open samples/plugins/image2/image2.html
  2. Select a widget.
  3. C-c.
  4. You've just been teleported to the bottom of editable.

This may depend on the size of selected element etc.

Change History (13)

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

Status: newconfirmed

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

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

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

Summary: Widgets: pastebin is not hiddenWidgets: copybin is not hidden

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

Owner: Piotrek Koszuliński deleted
Status: assignedconfirmed

comment:5 Changed 6 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

Issue is reproducable on all IE with exception of 11th (where it only "jumps").

Last edited 6 years ago by Marek Lewandowski (previous) (diff)

comment:6 Changed 6 years ago by Marek Lewandowski

Status: assignedreview

the best test for this copying is image2 plugin test

i've created 2 solutions:

1st is more diffrentiated
+ imho gives better ux
- is more complicated

2nd is more cross-browser
+ straight forward code
- relies on manual scrollTop reset, after very small timeout

1st solution t/10865:
make use of the fact that ie9 and higher allows to copy content of hidden elements, whereas other browsers don't pros: it inserts hidden element to dom so _theoretically_ there should be no reflow, and no jumping anywhere is visible

if the browser does not support hidden markup copying it will apply absolute positioning

2nd solution t/10865b:
uses the same absolute positioning for all browsers, but ies below 11th does hiccup on that pros: clearer code

please take a look on both solutions and put your thoughts on this subject


notes how solution 2 behaves on IEs:

IE8: adjusts its position by few pixels before first paste, it jumps when pressing ctrl+c (it does not after some pastes though)

errors: on paste undo, throws error: call .blur() on null/undefined

IE9: scrollbar jumps from time to time (not always) rarely content jump can be seen

IE10: scrollbar + content jump from time to time (approx 1/4 cases)

IE11: everything looks fine


general issue with this approach is that from times to times they only copy inner content of widget (that is caption for image2), but it's quite boundary case. I have no idea what's the case at this moment.

In meanwhile i will do some extra research on that topic, but meanwhile we can decide which conception do we want to follow.

comment:7 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewassigned

Let's go with absolute position everywhere for now. I find this a more straightforward way and we need to close this issue faster.

Later on we can do more research, especially considering that the issue is specially annoying in IE10.

comment:8 Changed 6 years ago by Marek Lewandowski

Status: assignedreview

In this situation we're going to progress with branch t/10865b.

Note that regex was changed in the way that it got duplicated (?:\s*style\=\"[^"]+")?, that's because IE9 moves style attribute as the first attribute, while other versions place it after id attr.

I've also moved regex to pasteReplaceRegex variable (which was not joined to var statement @ line 14, since it's messy, and is only important for setupDataProcessing() function ). That way we will avoid regex recompilation everytime setupDataProcessing() will be called.

During that ticket i also found out that:

  • there quite often there is issue (cross browser) that only caption text of image2 will be copied instead of whole wrapper
  • undo on ie8 causes exception

These issues will be reported in separate ticket.

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

Owner: changed from Marek Lewandowski to Piotrek Koszuliński

I pushed t/10865b on dev and tests.

Additional fixes:

  • Longer timeout is used so Chrome on Mac will be able to grab the content.
  • Double spans on IEs are used to avoid expanding editable's height.
  • Inverted order of focusing widget and removing copybin to avoid scroll jump on IEs. Previously copybin was removed first, so for a short period selection was reset to the beginning of editable.
  • Since the timeout is now pretty long (100ms), copying is aborted if previous is still going on. It prevents from error being thrown and we've got the same stopper in clipboard.

comment:10 Changed 6 years ago by Marek Lewandowski

Current solution works very well with non ie8 browsers, and there is no longer any jumping, here are results of my testing:

ie9 - fine
ie10 - fine
ie11 - fine
opera - fine
firefox - fine
chrome - fine
ie8 - browser always scrolls to the bottom

Seems that only ie8 left to fix. Quickfix which WFM is setting editor.document.$.documentElement.scrollTop after range.select() to its initial value, stored during function initialization.

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

Thanks, I pushed a fix for IE8 to t/10865b and rebased it on latest major. Waiting for code review now.

comment:12 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

Well... it works great. Nice job!

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

Resolution: fixed
Status: review_passedclosed

Fixed on major with git:cc0310f on dev and 0c6414a on tests.

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