Bug 213746 – [iOS 14] A couple of tests in editing/selection/ios fail after <rdar://problem/60978283>

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Wenson Hsieh]

More work towards <rdar://problem/64808138>

These two tests are:
• editing/selection/ios/select-text-in-existing-selection.html
• editing/selection/ios/selection-extends-into-overflow-area.html

[Wenson Hsieh]

Created attachment 403116 [details]
Patch

[Devin Rousso]

Comment on attachment 403116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403116&action=review

r=me, with some NITs

> LayoutTests/editing/selection/ios/select-text-in-existing-selection.html:28
> +editor = document.querySelector("div[contenteditable]");
> +target = document.getElementById("target");

NIT: `let`?

> LayoutTests/resources/ui-helper.js:135
> +        const x = element.offsetLeft + element.offsetWidth / 2;
> +        const y = element.offsetTop + element.offsetHeight / 2;

NIT: I'd add parenthesis around the `/ 2` to avoid ambiguity/complexity while reading

> LayoutTests/resources/ui-helper.js:1103
> +                new Promise(resolve => target.addEventListener(eventName, e => {

NIT: for readability, I usually avoid using inlined arrow functions unless I am intentionally using the implicit return value
```
    new Promise((eventListenerResolve) => {
        target.addEventListener(eventName, (e) => {
            event = e;
            eventListenerResolve();
        }, {once: true})
    }),
```

NIT: I'd also recommend using a different variable name other than `resolve` to avoid shadowing

[Wenson Hsieh]

Comment on attachment 403116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=403116&action=review

>> LayoutTests/editing/selection/ios/select-text-in-existing-selection.html:28
>> +target = document.getElementById("target");
> 
> NIT: `let`?

👍🏻

>> LayoutTests/resources/ui-helper.js:135
>> +        const y = element.offsetTop + element.offsetHeight / 2;
> 
> NIT: I'd add parenthesis around the `/ 2` to avoid ambiguity/complexity while reading

👍🏻

>> LayoutTests/resources/ui-helper.js:1103
>> +                new Promise(resolve => target.addEventListener(eventName, e => {
> 
> NIT: for readability, I usually avoid using inlined arrow functions unless I am intentionally using the implicit return value
> ```
>     new Promise((eventListenerResolve) => {
>         target.addEventListener(eventName, (e) => {
>             event = e;
>             eventListenerResolve();
>         }, {once: true})
>     }),
> ```
> 
> NIT: I'd also recommend using a different variable name other than `resolve` to avoid shadowing

Sounds good! Changed from inline => to => { … }, and also renamed the two inner `resolve`s to avoid shadowing.

[Wenson Hsieh]

Created attachment 403130 [details]
Patch for landing

[EWS]

Committed r263708: <https://trac.webkit.org/changeset/263708>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403130 [details].