Opened 5 years ago

Closed 5 years ago

#11104 closed Bug (fixed)

[IE] Focusing widget will reset editor scrollbar position

Reported by: Marek Lewandowski Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.3.3
Component: General Version: 4.3 Beta
Keywords: IE9 IE10 Cc:

Description

since: 4.3 until major

If you will scroll your content all the way down, blur editor, and click directly to widget (to focus it) - editor scrollbar will jump to topmost position. It happens regardless of widget type.

  1. open image2 sample
  2. scroll editor to its very bottom
  3. blur editor (i.e. click on sample title)
  4. click directly on any image widget

Expected result:
Clicked widget should simply be focused.

Current result:
Scrollbar is moved to top, in addition to that widget is not being focused, because caret goes to begining of the document.

Attachments (1)

11104_ie11_unable_to_blur.mp4 (335.8 KB) - added by Olek Nowodziński 5 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 5 years ago by Marek Lewandowski

Summary: [IE10] Focusing scrolled widget will reset editor scrollbar position[IE] Focusing scrolled widget will reset editor scrollbar position

I've just checked IE9 and it turns out that it occurs there as well, so it's present at least in IE10, IE9.

comment:2 Changed 5 years ago by Marek Lewandowski

Summary: [IE] Focusing scrolled widget will reset editor scrollbar position[IE] Focusing widget will reset editor scrollbar position

comment:3 Changed 5 years ago by Jakub Ś

Keywords: IE9 IE10 added
Status: newconfirmed
Version: 4.3 (GitHub - major)4.3 Beta

I was able to reproduce this issue from CKEditor 4.3 beta in IE9 and IE10 (IE8 seems to work fine)

comment:4 Changed 5 years ago by Marek Lewandowski

Milestone: CKEditor 4.3.1

change milestone to 4.3.1 it would be good if we can make this issue into release since it may be really annoying for end user

comment:5 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

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

Milestone: CKEditor 4.3.1CKEditor 4.3.2
Owner: Marek Lewandowski deleted
Status: assignedconfirmed

comment:7 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:8 Changed 5 years ago by Marek Lewandowski

Owner: Marek Lewandowski deleted
Status: assignedconfirmed

Source of the problem is focusing editor itself in:
https://github.com/ckeditor/ckeditor-dev/blob/master/plugins/widget/plugin.js#L1040

However skipping that line will cause no focus on widget. For the time being we will skip this issue since it's time consuming.


Also i found another TC, might (but doesn't have to) be related to that ticked.

(using IE10 @ Win8)

  1. open sample with image2 plugin (/samples/plugins/image2/image2.html)
  2. scroll down editor content all the way down
  3. click at white space between editor and browsers scrollbar

As a result in addition also window will be scrolled.

comment:9 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:10 Changed 5 years ago by Marek Lewandowski

Owner: Marek Lewandowski deleted
Status: assignedconfirmed

Back to confirmed, since i got sick, and did not investigate that much at friday.

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

Milestone: CKEditor 4.3.2CKEditor 4.3.3

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

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

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

This was a nightmare and I found only a clean solution, not a perfect one. I also won't be able to explain all my findings in a understandable way, so just a couple of facts:

  1. On IE something weird happens when you click widget wrapper or its child directly - after mouseup control selection or something that resembles it is created. I couldn't understand if that's CKE's fault or purely IE thing. It's very fragile and depends on used elements, attributes etc.
  2. Preventing mousedown does not help for what happens after mouseup, although for the time between mousedown and mouseup the control selection sometimes (also depends on case and IE version) is not created, so UX is better.
  3. To fix this we need to wait after mouseup and focus widget then.
  4. Unfortunately this creates a weird situation in which mousedown happened, mouseup happened, but we blocked all that so selection wasn't unlocked.
  5. I checked controlselect and beforeeditfocus events, because I thought that I'll be able to avoid preventing default action on mousedown, what would made the situation more normal, but they are terribly implemented and not worth a penny.
  6. So, we are in a situation after mouseup, with selection still locked, because we prevented mousedown and we want to focus widget.
  7. First we fake selection on widget wrapper. We cannot unlock selection first because of git:f0c7b424. I didn't want to have code branching there, so the order has to be: set selection, focus.
  8. So there's lockedSel#fake -> lockedSel#reset -> hideSelection.
  9. hideSelection has to move selection to the hidden selection container. We cannot skip that even if selection is locked, because we'll not avoid issue explained in git:f0c7b424 which actually occurs also on IEs (vide this issue).
  10. So hideSelection calls editor.getSelection().selectRanges( [ hiddenSelConRange ] ). Since editor returns locked selection, sel#getRanges falls into the special condition for sel#isLocked. This code is a hack, but the only possible hack allowing to play with locked selection and assuring that all other properties of it will be updated accordingly. Unfortunately it is far from perfect and when trying to handle focus it actually fires many focus/blur events what leads locking selection again and... nvmnd :D. Horror.
  11. But what's also wrong is that to move selection to hidden container hideSelection uses locked selection when it should use the real one. This is proposed in t/11104.
  12. Hooooowever, the special condition in selectRanges assures that focus gets back to the place where it was originally. Since this code was the actual source of issue, we cannot use it and, since we don't have unlimited time, I want to leave this broken. No one should play with locked selection and I'm for removing the special condition from selectRanges in the future. But for now - we can just ignore this.
Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

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

Status: assignedreview

Ok, enough clicking - time for review. I didn't include any tests because I couldn't write them. Since we don't have a nice logging system (#9786) the only easily checkable thing is hidden. Moreover, I saw that in my TC the "Wrong selection instance resets fake selection." is still logged when it's not when manually testing this.

Pushed branch:t/11104. Note: this patch has to be thoroughly tested on every browser.

Known issues (not regressions):

  • [FF] widget has to be clicked twice to focus it when starting from blurred inline editor.

Fixed issues:

  • [IE] no more scrolling.
  • [IE] no "Wrong selection instance resets fake selection." when focusing editor by clicking on widget if widget was focused before editor was blurred.
  • [IE] no "Wrong selection instance resets fake selection." when doubleclicking widget.
Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

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

Issues specific for t/11104 found so far:

  • [IE11]
    1. go to plugins/image2/dev/image2.html
    2. select the last widget of "Inline sample"
    3. clicking outside of the editor doesn't blur the widget (repeat 2. and 3. until reproducible)

comment:16 in reply to:  15 ; Changed 5 years ago by Piotrek Koszuliński

Replying to a.nowodzinski:

Issues specific for t/11104 found so far:

  • [IE11]
    1. go to plugins/image2/dev/image2.html
    2. select the last widget of "Inline sample"
    3. clicking outside of the editor doesn't blur the widget (repeat 2. and 3. until reproducible)

I could not reproduce this. I repeated 2-3 many times.

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

Another TC:

  • [IE8]
    1. plugins/image2/dev/image2.html
    2. select any widget in any editor
    3. De-caption it and set align to none (make it truly inline)
    4. Editable is scrolled to top

Changed 5 years ago by Olek Nowodziński

comment:18 in reply to:  16 ; Changed 5 years ago by Olek Nowodziński

Replying to Reinmar:

Replying to a.nowodzinski:

Issues specific for t/11104 found so far:

  • [IE11]
    1. go to plugins/image2/dev/image2.html
    2. select the last widget of "Inline sample"
    3. clicking outside of the editor doesn't blur the widget (repeat 2. and 3. until reproducible)

I could not reproduce this. I repeated 2-3 many times.

Check this out attachment:11104_ie11_unable_to_blur.mp4

comment:19 in reply to:  18 Changed 5 years ago by Piotrek Koszuliński

Replying to a.nowodzinski:

Replying to Reinmar:

Replying to a.nowodzinski:

Issues specific for t/11104 found so far:

  • [IE11]
    1. go to plugins/image2/dev/image2.html
    2. select the last widget of "Inline sample"
    3. clicking outside of the editor doesn't blur the widget (repeat 2. and 3. until reproducible)

I could not reproduce this. I repeated 2-3 many times.

Check this out attachment:11104_ie11_unable_to_blur.mp4

Trying 100x I was able to reproduce this once. Totally not important because it's even hard to understand if it's t/11104 specific.

comment:20 in reply to:  17 Changed 5 years ago by Piotrek Koszuliński

Replying to a.nowodzinski:

Another TC:

  • [IE8]
    1. plugins/image2/dev/image2.html
    2. select any widget in any editor
    3. De-caption it and set align to none (make it truly inline)
    4. Editable is scrolled to top

I was able to reproduce it on t/11104 on IE8, but not on IE11. What's more - I was able to reproduce it on master on IE11, but not on IE8.

As long as t/11104 fixes more than breaks it's better than nothing.

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

Replying to Reinmar:

  • [FF] widget has to be clicked twice to focus it when starting from blurred inline editor.

The right, reproducible TC is as follows:

  1. Open plugins/image2/dev/image2.html
  2. Focus framed editor (click somewhere in contents).
  3. Scroll to inline editor.
  4. Focus a widget in inline editor (it's focusing).
  5. Focus framed editor (see: 2.).
  6. Try re-focusing the same widget in inline editor (now it requires double-click).

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

I checked that TC mentioned in comment:17 is IE8 specific. Little bit more important than comment:15, but still not a blocker.

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

Status: reviewreview_passed

Though not perfect, the solution in branch:t/11104 fixes more than it breaks. I'm OK with that — still this is a step forward. Tested:

  • IE[8...11]
  • FF
  • Chrome (Linux, OSX)
  • Safari

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

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:bb0ed1c.

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