Bug 195178 – Native text selection UI is incorrectly suppressed in Microsoft Visio

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

[Wenson Hsieh]

1. Install iOS 12.
2. Request desktop site and create a new diagram in Microsoft Visio online.
3. Create a new shape and try to edit text in that shape

Expected: native text editing caret should appear, along with callout bar, etc.
Observed: selection UI is suppressed.

[Radar WebKit Bug Importer]

<rdar://problem/48519394>

[Wenson Hsieh]

Created attachment 363401 [details]
WIP

[Tim Horton]

I'm gonna leave this one for zalan/smfr

[Darin Adler]

Comment on attachment 363401 [details]
WIP

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

> Source/WebCore/rendering/RenderObject.cpp:1558
> +    auto* container = &enclosingBox();
> +    while (container) {

Nicer to write as a for loop. If we can’t make the third clause work we can still write:

    for (auto* container = &enclosingBox(); container; ) {

> Source/WebCore/rendering/RenderObject.cpp:1566
> +            auto& frame = container->frame();

I think this logic about walking out to the enclosing frame's renderer should be put in a helper function. Writing it out as a sequence of if statements makes the thing look too complicated and it’s really a single concept. Not sure exactly which helper function to write. Here’s one:

    static RenderBox* enclosingFrameRenderBox(RenderBox& container)
    {
        auto* owner = container.frame().ownerElement();
        if (!owner)
            return nullptr;
        auto* renderer = owner->renderer();
        return is<RenderBox>(renderer) ? &downcast<RenderBox>(*renderer) : nullptr;
    }

Then:

    static RenderBox* containingBlockCrossingFrameBoundaries(RenderBox& container)
    {
        auto* nextContainer = container.containingBlock();
        if (!nextContainer)
            nextContainer = enclosingFrameRenderBox(container);
        return nextContainer ? nextContainer : enclosingFrameRenderBox(container);
    }

Then:

    for (auto* container = &enclosingBox(); container; container = containingBlockCrossingFrameBoundaries(*container))

But I think there are other interesting factorings too that might allow sharing even more code with other call sites.

> Source/WebCore/rendering/RenderObject.cpp:1572
> +            if (frame.isMainFrame())
> +                break;
> +
> +            auto* owner = frame.ownerElement();
> +            if (!owner)
> +                break;

It’s unnecessary to check both isMainFrame and check ownerElement for null. I suggest skipping the isMainFrame check.

> Source/WebCore/rendering/RenderObject.cpp:1576
> +            if (!frameRenderer || !frameRenderer->isBox())
> +                break;

if (!is<RenderBox>(frameRenderer))
        break;

[Simon Fraser (smfr)]

Comment on attachment 363401 [details]
WIP

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

> Source/WebCore/ChangeLog:9
> +        Currently, our heuristics for detecting hidden editable areas attempts to search for empty parent renderers with

heuristics...attempts -> heuristics...attempt

> Source/WebCore/rendering/RenderObject.cpp:1563
> +        bool isOverflowHidden = style.overflowX() == Overflow::Hidden || style.overflowY() == Overflow::Hidden;
> +        if (isOverflowHidden && !container->isDocumentElementRenderer() && !container->isBody() && container->contentSize().isEmpty())
> +            return true;
> +

I think this code needs to be rewritten in terms of RenderLayer::clipRectRelativeToAncestor() or something similar. These are questions you ask of the RenderLayer rather than trying to compute yourself, which is fraught with danger.

[Wenson Hsieh]

Comment on attachment 363401 [details]
WIP

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

>> Source/WebCore/ChangeLog:9
>> +        Currently, our heuristics for detecting hidden editable areas attempts to search for empty parent renderers with
> 
> heuristics...attempts -> heuristics...attempt

Good catch!

>> Source/WebCore/rendering/RenderObject.cpp:1558
>> +    while (container) {
> 
> Nicer to write as a for loop. If we can’t make the third clause work we can still write:
> 
>     for (auto* container = &enclosingBox(); container; ) {

Sounds good — I rewrote both this and the while loop above as for loops, using static helpers to advance each iteration.

>> Source/WebCore/rendering/RenderObject.cpp:1563
>> +
> 
> I think this code needs to be rewritten in terms of RenderLayer::clipRectRelativeToAncestor() or something similar. These are questions you ask of the RenderLayer rather than trying to compute yourself, which is fraught with danger.

It wasn't clear how to make use of this; one approach that seems to work reasonably well is to check whether the enclosing layer's selfClipRect is empty, but this doesn't account for the scenario in which the enclosing layer's clippingRootForPainting() is itself (for instance, when a text field has "transform: translateZ(0);"). I was able to fix this by introducing a new helper on RenderLayer to the effect of hasEmptyClipRectRelativeToDocumentView() that computes the clip rect relative to the document view's layer instead of the clipping root for painting. But this affects no known sites and I'm not sure it's worth the additional code on RenderLayer, so I omitted it in the new patch (forthcoming).

>> Source/WebCore/rendering/RenderObject.cpp:1566
>> +            auto& frame = container->frame();
> 
> I think this logic about walking out to the enclosing frame's renderer should be put in a helper function. Writing it out as a sequence of if statements makes the thing look too complicated and it’s really a single concept. Not sure exactly which helper function to write. Here’s one:
> 
>     static RenderBox* enclosingFrameRenderBox(RenderBox& container)
>     {
>         auto* owner = container.frame().ownerElement();
>         if (!owner)
>             return nullptr;
>         auto* renderer = owner->renderer();
>         return is<RenderBox>(renderer) ? &downcast<RenderBox>(*renderer) : nullptr;
>     }
> 
> Then:
> 
>     static RenderBox* containingBlockCrossingFrameBoundaries(RenderBox& container)
>     {
>         auto* nextContainer = container.containingBlock();
>         if (!nextContainer)
>             nextContainer = enclosingFrameRenderBox(container);
>         return nextContainer ? nextContainer : enclosingFrameRenderBox(container);
>     }
> 
> Then:
> 
>     for (auto* container = &enclosingBox(); container; container = containingBlockCrossingFrameBoundaries(*container))
> 
> But I think there are other interesting factorings too that might allow sharing even more code with other call sites.

Ok! I pulled out some common logic into static helpers, and used these to rewrite the two while loops here as for loops.

>> Source/WebCore/rendering/RenderObject.cpp:1572
>> +                break;
> 
> It’s unnecessary to check both isMainFrame and check ownerElement for null. I suggest skipping the isMainFrame check.

Fixed!

>> Source/WebCore/rendering/RenderObject.cpp:1576
>> +                break;
> 
> if (!is<RenderBox>(frameRenderer))
>         break;

I ended up removing this part.

[Wenson Hsieh]

Created attachment 363518 [details]
Take two

[Darin Adler]

Comment on attachment 363518 [details]
Take two

Wow, so much better in both algorithm and coding style too!

[Wenson Hsieh]

Comment on attachment 363518 [details]
Take two

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 363518 [details]
Take two

Clearing flags on attachment: 363518

Committed r242401: <https://trac.webkit.org/changeset/242401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.