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
Created attachment 403116 [details] Patch
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 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.
Created attachment 403130 [details] Patch for landing
Committed r263708: <https://trac.webkit.org/changeset/263708> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403130 [details].