Bug 199741

Summary: [iOS WK2] Can't place caret or select in content that overflows a contenteditable element
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: HTML EditingAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: megan_gardner, sam, simon.fraser, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch wenson_hsieh: review+

Description Simon Fraser (smfr) 2019-07-11 17:51:13 PDT
[iOS WK2] Can't place caret or select in content that overflows a contenteditable element
Comment 1 Simon Fraser (smfr) 2019-07-11 17:57:27 PDT
Created attachment 373985 [details]
Patch
Comment 2 Simon Fraser (smfr) 2019-07-11 17:57:29 PDT
<rdar://problem/50545233>
Comment 3 Simon Fraser (smfr) 2019-07-11 17:59:19 PDT
Created attachment 373986 [details]
Patch
Comment 4 Wenson Hsieh 2019-07-11 21:34:37 PDT
> 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.
Comment 5 Simon Fraser (smfr) 2019-07-12 12:58:23 PDT
Created attachment 374028 [details]
Patch
Comment 6 Wenson Hsieh 2019-07-12 13:08:15 PDT
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.
Comment 7 Simon Fraser (smfr) 2019-07-12 14:42:34 PDT
https://trac.webkit.org/r247398
Comment 8 Sam Weinig 2019-07-12 14:46:19 PDT
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.
Comment 9 Simon Fraser (smfr) 2019-07-12 14:48:32 PDT
(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.
Comment 10 Sam Weinig 2019-07-12 15:02:35 PDT
(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.
Comment 11 Simon Fraser (smfr) 2019-07-12 15:13:22 PDT
Not right now.
Comment 12 Sam Weinig 2019-07-14 17:26:38 PDT
(In reply to Simon Fraser (smfr) from comment #11)
> Not right now.

Bummer.