Bug 199741 – [iOS WK2] Can't place caret or select in content that overflows a contenteditable element

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Simon Fraser (smfr)]

[iOS WK2] Can't place caret or select in content that overflows a contenteditable element

[Simon Fraser (smfr)]

Created attachment 373985 [details]
Patch

[Simon Fraser (smfr)]

<rdar://problem/50545233>

[Simon Fraser (smfr)]

Created attachment 373986 [details]
Patch

[Wenson Hsieh]

> Test suite failed
> Failed
>     TestWebKitAPI.KeyboardInputTests.SelectionClipRectsWhenPresentingInputView
>         
>         /Volumes/Data/worker/iOS-12-Simulator-Build-EWS/build/Tools/TestWebKitAPI/Tests/ios/KeyboardInputTestsIOS.mm:524
>         Expected equality of these values:
>           10
>           selectionClipRect.origin.x
>             Which is: 11

It looks like this can just be rebaselined.

[Simon Fraser (smfr)]

Created attachment 374028 [details]
Patch

[Wenson Hsieh]

Comment on attachment 374028 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:1610
> +    boundingBox.setWidth(boundingBox.width() - style.borderLeftWidth() - style.borderRightWidth());
> +    boundingBox.setHeight(boundingBox.height() - style.borderBottomWidth() - style.borderTopWidth());

Nit - could consider using contract instead.

> LayoutTests/editing/caret/ios/caret-in-overflow-area.html:47
> +        window.addEventListener('load', async () => {
> +            runTest();
> +        }, false);

Nit - addEventListener(‘load’, runTest);?

> LayoutTests/editing/selection/ios/place-selection-in-overflow-area.html:53
> +        window.addEventListener('load', async () => {
> +            runTest();
> +        }, false);

Ditto.

> LayoutTests/editing/selection/ios/selection-extends-into-overflow-area.html:61
> +        window.addEventListener('load', async () => {
> +            runTest();
> +        }, false);

Ditto.

[Simon Fraser (smfr)]

https://trac.webkit.org/r247398

[Sam Weinig]

Comment on attachment 374028 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebPage.h:1212
> +#if PLATFORM(IOS_FAMILY)
> +    // This excludes layout overflow, includes borders.
> +    static WebCore::IntRect rootViewBoundsForElement(const WebCore::Element&);
> +    // These include layout overflow for overflow:visible elements, but exclude borders.
> +    static WebCore::IntRect absoluteInteractionBoundsForElement(const WebCore::Element&);
> +    static WebCore::IntRect rootViewInteractionBoundsForElement(const WebCore::Element&);
> +#endif // PLATFORM(IOS_FAMILY)

Do these really need to be iOS specific? They seem pretty general. We really should be aiming to have less platform specific code like this.

[Simon Fraser (smfr)]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 374028 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374028&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.h:1212
> > +#if PLATFORM(IOS_FAMILY)
> > +    // This excludes layout overflow, includes borders.
> > +    static WebCore::IntRect rootViewBoundsForElement(const WebCore::Element&);
> > +    // These include layout overflow for overflow:visible elements, but exclude borders.
> > +    static WebCore::IntRect absoluteInteractionBoundsForElement(const WebCore::Element&);
> > +    static WebCore::IntRect rootViewInteractionBoundsForElement(const WebCore::Element&);
> > +#endif // PLATFORM(IOS_FAMILY)
> 
> Do these really need to be iOS specific? They seem pretty general. We really
> should be aiming to have less platform specific code like this.

Agreed. I started to clean things up and it got messy; it's unclear if all the callers want to include borders or not, and which want the interaction rect vs. the content box rect.

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #9)
> (In reply to Sam Weinig from comment #8)
> > Comment on attachment 374028 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=374028&action=review
> > 
> > > Source/WebKit/WebProcess/WebPage/WebPage.h:1212
> > > +#if PLATFORM(IOS_FAMILY)
> > > +    // This excludes layout overflow, includes borders.
> > > +    static WebCore::IntRect rootViewBoundsForElement(const WebCore::Element&);
> > > +    // These include layout overflow for overflow:visible elements, but exclude borders.
> > > +    static WebCore::IntRect absoluteInteractionBoundsForElement(const WebCore::Element&);
> > > +    static WebCore::IntRect rootViewInteractionBoundsForElement(const WebCore::Element&);
> > > +#endif // PLATFORM(IOS_FAMILY)
> > 
> > Do these really need to be iOS specific? They seem pretty general. We really
> > should be aiming to have less platform specific code like this.
> 
> Agreed. I started to clean things up and it got messy; it's unclear if all
> the callers want to include borders or not, and which want the interaction
> rect vs. the content box rect.

Will you finish :) ? Since these are static, and don't rely on the Page at all, it might also be better to move them to their own ElementGeometry.h/cpp (or whatever) file. And/Or move them to WebCore.

[Simon Fraser (smfr)]

Not right now.

[Sam Weinig]

(In reply to Simon Fraser (smfr) from comment #11)
> Not right now.

Bummer.