Opened 6 years ago

Closed 6 years ago

#11999 closed Bug (fixed)

Tests failing after update to Chrome 35

Reported by: Olek Nowodziński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.4.2
Component: General Version: 3.0
Keywords: Blink Cc:

Description

Two tests fail after upgrade to 35.0.1916.114 while they pass in 34.0.1847.131:

/dt/plugins/elementspath/click.html#test clicking typical strong element
/dt/plugins/enter/enterkey.html#test enterkey after invisible element

We must know the scope and possible impact of changes in v35 behind this failure. It may affect other aspects of the editor which are not tested or impossible to test.

Attachments (1)

11999a.zip (2.9 KB) - added by Olek Nowodziński 6 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Olek Nowodziński

Status: newconfirmed

comment:2 Changed 6 years ago by Olek Nowodziński

My research revealed that the difference between 34 and 35 is that window.getSelection().getRangeAt() works slightly different, which quite likely has been introduced in 169100.

Use attached sample to see that if clicked "Select natively" in Opera (Chrome/34.0.1847.132) the range is attached in the text node "foo", while in Chrome (Chrome/35.0.1916.114) it is <strong>. What happens next is just a snowball effect – algorithms enter wrong branches.

As for

/dt/plugins/elementspath/click.html#test clicking typical strong element

I suppose that elementspath plugin is safe because it is a matter of getHtmlWithSelection, which returns a "wrong" range in the assertion. In fact elementspath does not use selection.getRanges() (getRangeAt) at all.

However

/dt/plugins/enter/enterkey.html#test enterkey after invisible element

might be a problem because the algorithm relies on the output of selection.getRanges(). We got to find out how (if?) to fix it. In fact what worries me a lot is that we got 2 different behaviours for CKEDITOR.env.webkit, which means that Webkit and Blink go into different directions. We need to know how to deal with it.

Changed 6 years ago by Olek Nowodziński

Attachment: 11999a.zip added

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

cc

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

It seems that we were hit by https://code.google.com/p/chromium/issues/detail?id=346613 which is actually fixing one of the biggest problems with selection in Blink (hopefully Webkit will have this patch too). It's not fully fixed yet, so we still need the filling char, but I think that we'll be able to remove it soon :).

The biggest change right now was that getRangeAt() returns exactly the range set by addRange(), without moving it to nearest text node. It's perfectly visible in the sample for https://code.google.com/p/chromium/issues/detail?id=351819.

We need to start updating our own code too. Hopefully, these are only tests. Olek, could you handle this? We need to learn whether we don't have some logic depending on the old, incorrect behaviour of Blink. If so, we need to straighten it ASAP. Note that we already have env.safari and env.chrome.

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

Owner: set to Olek Nowodziński
Status: confirmedassigned

comment:6 Changed 6 years ago by Olek Nowodziński

I pushed fixed tests to t/11999. Note that several assertions will fail in Opera unti gets updated to latest Blink.

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

So these were only tests? What about getRanges() method? Does it seem to work ok? I think that you can verify this by controlling env.webkit checks in it.

comment:8 in reply to:  7 Changed 6 years ago by Olek Nowodziński

Replying to Reinmar:

So these were only tests? What about getRanges() method? Does it seem to work ok? I think that you can verify this by controlling env.webkit checks in it.

Working on it. That was only the 1st stage.

comment:9 Changed 6 years ago by Olek Nowodziński

Status: assignedreview

I checked the code of seleciton.getRanges() and tested the editor manually. I see no alarming behaviour other than what is fixed by t/11999 (tests). Still I insist that we keep an eye on the whole matter during test phase because there might be something that I missed.

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

Resolution: fixed
Status: reviewclosed

Merged to master with f612802. I made some manual tests and the affected cases seem to work.

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