Opened 6 years ago

Closed 6 years ago

#9499 closed Bug (fixed)

Fix selection change event on data reload

Reported by: Garry Yao Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 4.0
Component: Core : Selection Version: 4.0
Keywords: Cc:

Description

Selection change event is not always fired when data reload happens, this happens consistency for IE, and it breaks FF in the following test as well:

http://ckeditor4.t/dt/core/selection/editor.html?name=test-selectionChange-fired-on-data-loaded-

Change History (9)

comment:1 Changed 6 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

comment:2 Changed 6 years ago by Garry Yao

Status: assignedreview

Opened t/9499 for review:

An initial "virtual" selection is presented on the editor upon data loaded, this selection will be placed at the start of the document, it will make all editor states that relies on the "selectionChange" event to work. But it doesn't impose any actual editor focus.

Additionally, this initial selection will be consolidated, once editable gain focus, it helps to guarantee a consistent range position when editor is focused for the first time.

comment:3 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

I think a much simpler solution can come out to solve this problem, which is basically having that test passing (that's our main goal now).

In any case, the code is already there, so I've checked it. While generally ok with the change, there are two things that make me worried:

  • "8fd5e5d: Avoid making real DOM selection, when locked, it will scroll IE unexpectively." - This one gives me the sensation that we'll have lots of troubles in the future and that we cannot predict them now.
  • "4d98e64: Allow selection to be locked even if empty." - CKEDITOR.SELECTION_NONE is not restored (to an empty selection). Sounds wrong.

Well... another thing that makes me even more worried is that I'm having 2 reds on that branch.

In any case, after some research, the culprit for that red has been found: 77afd3d.

Knowing the breaking point, I'm sure that a much simpler solution could be found for this, avoiding us spending days to solve an issue that is not causing major consequences.

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

Regarding red tests - I believe that you Fred had them on Chrome, when I had all green on Chrome and 3 reds on Opera. Strange as always :).

First bad commit is https://github.com/ckeditor/ckeditor-dev/commit/a13e678 and since #9470 was only Opera's issue maybe we can target this patch only for it?

One more thing makes me wonder - how does this change affect selectionchange? :|

Last edited 6 years ago by Piotrek Koszuliński (previous) (diff)

comment:5 Changed 6 years ago by Garry Yao

Status: review_failedreview

Since there exists some controversy commits in the above branch, I simplified the branch on t/9499b with only the change to fix this issue, leaved the rest of features to be checked in another dedicated ticket.

comment:6 Changed 6 years ago by Garry Yao

Status: reviewreview_failed

I figured out t/9499b is still falling IE, even the test shows PASSED, being it's an issue not revealed by the test:

  1. Load the replacebyclass in IE;
  2. Check the toolbar status without focusing the document;
  • Actual: the toolbar is not properly given the initial states as in other browsers.

comment:7 Changed 6 years ago by Garry Yao

Status: review_failedreview

I was difficult to propose a fix that relies on making an initial selection, so I would propose for a simple solution that slightly changes the selectionCheck function - when selection is of type NONE, use a synthetic selection path (anchored at the start of document) in the event, it should be enough to bootstrap most of the commands.

Opened t/9499c for review.

comment:8 in reply to:  6 Changed 6 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

I figured out t/9499b is still falling IE, even the test shows PASSED, being it's an issue not revealed by the test:

  1. Load the replacebyclass in IE;
  2. Check the toolbar status without focusing the document;
  • Actual: the toolbar is not properly given the initial states as in other browsers.

I'm not reviewing this ticket. I just want to say that this is something we can live with. In fact, that's the v3 behavior. So, if we can keep things simple, let's do it.

comment:9 Changed 6 years ago by Garry Yao

Resolution: fixed
Status: reviewclosed

Reverted part of a13e678 on master with 73645.

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