Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#12758 closed Bug (fixed)

[Webkit] Red tests on master after #12747

Reported by: Piotrek Koszuliński Owned by: Olek Nowodziński
Priority: Normal Milestone: CKEditor 4.4.7
Component: General Version: 4.4.7
Keywords: Cc:

Description

After #12747 there's one failing test in http://tests.ckeditor.dev:1030/tests/plugins/magicline/magicline

The first bad commit is git:5875b0cf. The change looked good, but I assumed that the code was wrong - that only one of scroll positions matter. Unfortunately, thanks to missing comments and that broken test I lost now this confidence. There's a small possibility that both scroll positions should be taken into account.

Change History (10)

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

It fails in Chrome (Webkit*) only. And this is why. We got to branch some code in element.getDocumentPostion() and write simple tests so that we immediately discover when they finally standardise it.

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

Status: newconfirmed
Summary: Red tests on master after #12747[Webkit] Red tests on master after #12747

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

Can we implement this in a way that when it is changed in the browser it will still work in CKEditor, so we avoid problems like Facebook? I mean - is it possible to guess which property should be used?

comment:4 in reply to:  3 Changed 5 years ago by Olek Nowodziński

Replying to Reinmar:

Can we implement this in a way that when it is changed in the browser it will still work in CKEditor, so we avoid problems like Facebook? I mean - is it possible to guess which property should be used?

Assuming that

  1. document.documentElement.scrollTop and document.body.scrollTop are either 0 or the right value (different than 0), depending on the browser.
  2. If both are 0 then it's really 0.
  3. One of the above is the right value.
  4. scrollTop is a positive integer.

Math.max( document.documentElement.scrollTop, document.body.scrollTop ) or ( document.documentElement.scrollTop || document.body.scrollTop ) should do the trick.

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

Owner: set to Olek Nowodziński
Status: confirmedassigned

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

Status: assignedreview

Put branch:t/12758 on review.

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

Status: reviewreview_failed

This patch cases regression in #12747. Check the manual test.

Please target the fix to Webkit/Blink browsers only.

Version 0, edited 5 years ago by Piotrek Koszuliński (next)

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

Status: review_failedreview

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

Status: reviewreview_failed

I pushed one more commit to branch:t/12758b. I think that we should use the value safely on Webkit/Blink, just like you proposed previously. I don't know why you reverted that change now.

However:

  • the new test fail on Firefox (rounding) and Safari (2px difference) - you can use assert.inNumberInRange.
  • I really miss a manual test.
Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

comment:10 Changed 5 years ago by Artur Delura

Resolution: fixed
Status: review_failedclosed

Modified existing test. Couldn't reproduce issue in manual test - already tried in ticket:12788. Merge commit.

Last edited 5 years ago by Artur Delura (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy