Bug 213564 – [iOS] -_requestTextInputContextsInRect cannot find empty Quip spreadsheet title

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Daniel Bates]

The spreadsheet title in a Quip spreadsheet is an editable inline element. When it has no content, which can happen if you delete it, then it has zero width and in this state it is not found by -_requestTextInputContextsInRect.

[Daniel Bates]

<rdar://problem/59355847>

[Daniel Bates]

Created attachment 402674 [details]
Patch

[Daniel Bates]

Comment on attachment 402674 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        and painting them. It's complicated to patch up all this up with a 100% correct

this up => this

[Daniel Bates]

Comment on attachment 402674 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        (They can only be found when focused using a keyboard or random tapping/clicking).

random tapping/clicking => tapping/clicking on them

[Simon Fraser (smfr)]

Comment on attachment 402674 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        solution without forcing more line box creation. It's clear to me that the customer
> +        experience will be significantly better if the focused editable no line box case
> +        is handled, but it's unclear whether that can be topped by patching everything to
> +        track all editable inlines without a line box because they are not discoverable.
> +        (They can only be found when focused using a keyboard or random tapping/clicking).

Maybe you don't need all these words.

> Source/WebCore/platform/graphics/FloatRect.cpp:57
> +bool FloatRect::intersectsEvenIfEmpty(const FloatRect& other) const

This is ambiguous about which rect might be empty. Is it |this| or other or both?

[Daniel Bates]

It's clear to me that the customer experience will be significantly better if the focused editable no line box case is handled, but it's unclear whether that can be topped by patching everything to track all editable inlines without a line box because they are not discoverable. (They can only be found when focused using a keyboard or tapping/clicking on them). So, I chose to fix this bug by adding a special case in the code.

[Daniel Bates]

Comment on attachment 402674 [details]
Patch

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

>>> Source/WebCore/ChangeLog:17
>>> +        (They can only be found when focused using a keyboard or random tapping/clicking).
>> 
>> random tapping/clicking => tapping/clicking on them
> 
> Maybe you don't need all these words.

Copied a bunch of ^^^ words into a bugzilla comment to capture them...I'll remove them from the change log

>> Source/WebCore/platform/graphics/FloatRect.cpp:57
>> +bool FloatRect::intersectsEvenIfEmpty(const FloatRect& other) const
> 
> This is ambiguous about which rect might be empty. Is it |this| or other or both?

Both could be empty. I just was following the naming convention of uniteIfEmpty(). I can't think of a better name, right now.

[Daniel Bates]

Comment on attachment 402674 [details]
Patch

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

>>> Source/WebCore/platform/graphics/FloatRect.cpp:57
>>> +bool FloatRect::intersectsEvenIfEmpty(const FloatRect& other) const
>> 
>> This is ambiguous about which rect might be empty. Is it |this| or other or both?
> 
> Both could be empty. I just was following the naming convention of uniteIfEmpty(). I can't think of a better name, right now.

Renaming inclusiveIntersection() on your private chat suggestion.

[Daniel Bates]

Thanks Simon!

[Daniel Bates]

Comment on attachment 402674 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/FloatRect.cpp:57
>>>> +bool FloatRect::intersectsEvenIfEmpty(const FloatRect& other) const
>>> 
>>> This is ambiguous about which rect might be empty. Is it |this| or other or both?
>> 
>> Both could be empty. I just was following the naming convention of uniteIfEmpty(). I can't think of a better name, right now.
> 
> Renaming inclusiveIntersection() on your private chat suggestion.

Actually renaming to inclusivelyIntersects

[Daniel Bates]

Created attachment 402815 [details]
To land

[Darin Adler]

Comment on attachment 402674 [details]
Patch

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

Heh, just realized I never pushed publish. My comment is consistent with what the two of you did!

> Source/WebCore/platform/graphics/FloatRect.h:165
> +    WEBCORE_EXPORT bool intersectsEvenIfEmpty(const FloatRect&) const;

Would be nice to find a more graceful name for this operation. I guess this is like uniteEvenIfEmpty. There have to be some better terms of art for this sort of thing since they come up in every single graphics library!

[Daniel Bates]

Created attachment 402816 [details]
To land

[Daniel Bates]

Created attachment 402818 [details]
To land

[Daniel Bates]

Committed r263531: <https://trac.webkit.org/changeset/263531>

[Daniel Bates]

Comment on attachment 402674 [details]
Patch

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

>> Source/WebCore/platform/graphics/FloatRect.h:165
>> +    WEBCORE_EXPORT bool intersectsEvenIfEmpty(const FloatRect&) const;
> 
> Would be nice to find a more graceful name for this operation. I guess this is like uniteEvenIfEmpty. There have to be some better terms of art for this sort of thing since they come up in every single graphics library!

I went with inclusivelyIntersects in the latest patch, just landed.