Opened 7 years ago

Closed 7 years ago

#10115 closed Bug (fixed)

[Webkit] Invisible caret after newpage

Reported by: Olek Nowodziński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.0.2
Component: Core : Selection Version: 4.0.2
Keywords: Cc:

Description

  1. Open replacebycode sample.
  2. Click "New Page" button.
  3. Content is removed, typing is possible but the caret is visually gone.
  • Works in 4.0.1
  • Tested in Chrome & Safari@Win.

Change History (11)

comment:1 Changed 7 years ago by Frederico Caldeira Knabben

Component: Core : EditableCore : Selection
Status: newconfirmed

Introduced with git:a9d4df7.

comment:2 Changed 7 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: confirmedassigned

comment:3 Changed 7 years ago by Frederico Caldeira Knabben

Status: assignedreview

Safe fix pushed into t/10115@cksource.

comment:4 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewassigned

Test results of my "safe fix":
6 failed / 1057 passed / 00m 43s 895ms - 100%

While the fix may be enough for the real world usage, it doesn't work well with tests.

comment:5 Changed 7 years ago by Frederico Caldeira Knabben

Status: assignedreview

Another workaround is now proposed on t/10115b@cksource.

I tried to find the root of the issue, but it looks that is is related to an internal race condition of WebKit itself when dealing with the contents replacement and the selection change. So, an ugly fix was the only clear solution.

I've also fixed the order of the events, which were matching another "-1" priority used somewhere else in the code.

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

Pushed t/10115c on dev and tests.

Few important things:

  • New tests doesn't verify this issue, they can't. But they check if everything works fine from on the code level. I added them to ensure that there are no regressions introduced.
  • This patch targets #9507 more precisely. Only Firefox has problems with the position of initial selection so hack works on gecko only.
  • And most important thing - what was the root of this issue?
    • On editor#dataReady selection is checked (here).
    • To do that, editor creates selection instance.
    • There's a hack for Webkits inside selection constructor (here). If incorrectly initialized selection is discovered it's moved to the right place.
    • And here's the tricky part - addRange() causes focus to be fired on editable.
    • Then #9507's hack is executed. It sees that selection is directly in editable. This is because fixDom hasn't been called yet, because it's called on selectionChange (here) and selectionChange will be fired when editor will check it.
    • Thus - hack for Webkit caused that focus is fired before DOM was fixed. On Firefox this order is correct - setData -> selectionChange -> fixDom -> focus.
    • Therefore I decided to cancel focus event if it was fired exactly on addRange() in Webkit's hack. Execution flow is fixed, focus listener is executed on fixed DOM so as Webkit fixes selection by itself, it is correct so it is not moved. As a result caret is visible.

I was also thinking whether FF's case couldn't be fixed in selection constructor (as Webkit's case), but first we'd have to figure out correct condition, so it's safer now to stay with the fix we tested.

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

Status: reviewreview_failed

Replying to Reinmar:

Pushed t/10115c on dev and tests.

  • This patch targets #9507 more precisely. Only Firefox has problems with the position of initial selection so hack works on gecko only.

I'm now unsure that that fix is FF only. I have this issue with t/10115c and Chrome:

  • Open replacebyclass.
  • Click on the page, right above the editor.
  • TAB to move the focus to the editor.

The editor takes the focus, but the caret is not visible. master also has an issue with the TC, but at least the caret is visible.

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

Owner: Frederico Caldeira Knabben deleted
Status: review_failedconfirmed

Sorry, it was a wrong observation. The described issue was introduced with git:7afa822 on master.

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

Owner: set to Piotrek Koszuliński
Status: confirmedreview

The caret is visible on both branches - master and t/10115c, but it blinks next to image as this is the first element in editor. The fact that caret was visible in other place than that in fact proves that moveToElementEditStart() works incorrectly.

comment:10 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

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

Resolution: fixed
Status: review_passedclosed

Fixed on master with git:81d6123 and tests with afa31d0.

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