Bug 213746 - [iOS 14] A couple of tests in editing/selection/ios fail after <rdar://problem/60978283>
Summary: [iOS 14] A couple of tests in editing/selection/ios fail after <rdar://proble...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-29 11:57 PDT by Wenson Hsieh
Modified: 2020-06-29 16:59 PDT (History)
4 users (show)

See Also:


Attachments
Patch (8.95 KB, patch)
2020-06-29 14:41 PDT, Wenson Hsieh
drousso: review+
Details | Formatted Diff | Diff
Patch for landing (9.12 KB, patch)
2020-06-29 16:09 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2020-06-29 11:57:15 PDT
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
Comment 1 Wenson Hsieh 2020-06-29 14:41:43 PDT
Created attachment 403116 [details]
Patch
Comment 2 Devin Rousso 2020-06-29 15:12:33 PDT
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
Comment 3 Wenson Hsieh 2020-06-29 16:05:30 PDT
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.
Comment 4 Wenson Hsieh 2020-06-29 16:09:28 PDT
Created attachment 403130 [details]
Patch for landing
Comment 5 EWS 2020-06-29 16:59:04 PDT
Committed r263708: <https://trac.webkit.org/changeset/263708>

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