[macOS/Safari] Test "EditorView.movePos / properly handles line motion when not focused" #84

New issue

Closed

opened 2019-03-13 19:25:10 +01:00 by FrankReh · 1 comment

FrankReh commented

2019-03-13 19:25:10 +01:00

(Migrated from github.com)

Error: 11 != 10 [snip]

Note: the same test code passed when the editor had focus. It is only the focus==false case that isn’t passing for Safari. And the telling code that seems to be introducing the difference takes a different branch for Safari compared to Chrome and Firefox.

function testLineMotion(focus) {
    var cm = tempEditor("one two\nthree\nتممين");
    if (focus)
        requireFocus(cm);
    else
        cm.contentDOM.blur();
    ist_1(cm.movePos(0, "forward", "line"), 8);
    ist_1(cm.movePos(1, "forward", "line"), 9);
    ist_1(cm.movePos(7, "forward", "line"), 13);
    var last = cm.movePos(10, "forward", "line");
    ist_1(last, 19, "<");
    ist_1(last, 14, ">");
    cm.dispatch(cm.state.t().setSelection(EditorSelection.single(1))); // Clear goal columns
    ist_1(cm.movePos(last, "backward", "line"), 10); // <-- When focus==false, this movePos returns 11 on my macOS/Safari.
    ist_1(cm.movePos(12, "backward", "line"), 4);
    ist_1(cm.movePos(13, "backward", "line"), 5);
    ist_1(cm.movePos(8, "backward", "line"), 0);
}

At a lower level, textCoords() is coming up with a different result on Safari.

Failing Safari computes startCoords.left==28 and then posAtCoords comes up with offset 3.
Both passing Chrome and Firefox compute startCoords.left==20 (or 19.6 which is close enough) and then posAtCoords comes up with offset 2.

The value of 28 (failing case) or 20 (working cases) is coming from the call to textCoords when pos=3.

function textCoords(text, pos) {
    // For all three browsers, pos=3.
    var range = document.createRange();
    if (browser.chrome || browser.gecko) {
        // These browsers reliably return valid rectangles for empty ranges
        range.setEnd(text, pos);
        range.setStart(text, pos);
        return range.getBoundingClientRect();
    }
    else {
        // Otherwise, get the rectangle around a character and take one side
        var extend = pos == 0 ? 1 : -1;
        range.setEnd(text, pos + (extend > 0 ? 1 : 0));
        range.setStart(text, pos - (extend < 0 ? 1 : 0));
        var rect = range.getBoundingClientRect();
        var x = extend < 0 ? rect.right : rect.left;
        // In Safari failing case:
        // extend      = -1,
        // rect.right  = 28,
        // rect.left   = 19,
        // therefore x = 28.
        return { left: x, right: x, top: rect.top, bottom: rect.bottom };
    }
}

The working cases follow the if branch while the failing case follows the else branch. I added some comments to show what the variable values are at the time x is being set to 28.

Also interesting I think: if the code in textCoords is changed so all three browsers use the manual method, they all fail.

The reason the focus==true version of the test passes seems to be related to a routine higher in the call stack, movPos. The first if block in movPos is only executed for the offset==true case and only for Safari and Chrome. The little bit of logic outlined above only comes into play if the first block is not entered.

Trying to find the right level of description for the mix of browser and function and branch and result is challenging in such an analysis. Sorry if it is too long.

> Error: 11 != 10 [snip] Note: the same test code passed when the editor had focus. It is only the focus==false case that isn’t passing for Safari. And the telling code that seems to be introducing the difference takes a different branch for Safari compared to Chrome and Firefox. ``` function testLineMotion(focus) { var cm = tempEditor("one two\nthree\nتممين"); if (focus) requireFocus(cm); else cm.contentDOM.blur(); ist_1(cm.movePos(0, "forward", "line"), 8); ist_1(cm.movePos(1, "forward", "line"), 9); ist_1(cm.movePos(7, "forward", "line"), 13); var last = cm.movePos(10, "forward", "line"); ist_1(last, 19, "<"); ist_1(last, 14, ">"); cm.dispatch(cm.state.t().setSelection(EditorSelection.single(1))); // Clear goal columns ist_1(cm.movePos(last, "backward", "line"), 10); // <-- When focus==false, this movePos returns 11 on my macOS/Safari. ist_1(cm.movePos(12, "backward", "line"), 4); ist_1(cm.movePos(13, "backward", "line"), 5); ist_1(cm.movePos(8, "backward", "line"), 0); } ``` At a lower level, textCoords() is coming up with a different result on Safari. Failing Safari computes startCoords.left==28 and then posAtCoords comes up with offset 3. Both passing Chrome and Firefox compute startCoords.left==20 (or 19.6 which is close enough) and then posAtCoords comes up with offset 2. The value of 28 (failing case) or 20 (working cases) is coming from the call to textCoords when pos=3. ``` function textCoords(text, pos) { // For all three browsers, pos=3. var range = document.createRange(); if (browser.chrome || browser.gecko) { // These browsers reliably return valid rectangles for empty ranges range.setEnd(text, pos); range.setStart(text, pos); return range.getBoundingClientRect(); } else { // Otherwise, get the rectangle around a character and take one side var extend = pos == 0 ? 1 : -1; range.setEnd(text, pos + (extend > 0 ? 1 : 0)); range.setStart(text, pos - (extend < 0 ? 1 : 0)); var rect = range.getBoundingClientRect(); var x = extend < 0 ? rect.right : rect.left; // In Safari failing case: // extend = -1, // rect.right = 28, // rect.left = 19, // therefore x = 28. return { left: x, right: x, top: rect.top, bottom: rect.bottom }; } } ``` The working cases follow the _if_ branch while the failing case follows the _else_ branch. I added some comments to show what the variable values are at the time x is being set to 28. Also interesting I think: if the code in textCoords is changed so all three browsers use the manual method, they all fail. The reason the focus==true version of the test passes seems to be related to a routine higher in the call stack, movPos. The first _if_ block in movPos is only executed for the offset==true case and only for Safari and Chrome. The little bit of logic outlined above only comes into play if the first block is not entered. Trying to find the right level of description for the mix of browser and function and branch and result is challenging in such an analysis. Sorry if it is too long.

marijnh commented

2020-06-29 16:27:13 +02:00

(Migrated from github.com)

As of 57d0310433 the tests all run again on Safari

As of 57d031043383 the tests all run again on Safari

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

codemirror/dev#84

No description provided.

Rows
Columns