#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 10 years ago by
comment:2 Changed 10 years ago by
Status: | new → confirmed |
---|---|
Summary: | Red tests on master after #12747 → [Webkit] Red tests on master after #12747 |
comment:3 follow-up: 4 Changed 10 years ago by
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 Changed 10 years ago by
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
document.documentElement.scrollTop
anddocument.body.scrollTop
are either0
or the right value (different than0
), depending on the browser.- If both are
0
then it's really0
. - One of the above is the right value.
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 10 years ago by
Owner: | set to Olek Nowodziński |
---|---|
Status: | confirmed → assigned |
comment:7 Changed 10 years ago by
Status: | review → review_failed |
---|
This patch causes regression in #12747. Check the manual test.
Please target the fix to Webkit/Blink browsers only.
comment:9 Changed 10 years ago by
Status: | review → review_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.
comment:10 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | review_failed → closed |
Modified existing test. Couldn't reproduce issue in manual test - already tried in ticket:12788. Merge commit.
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.