Bug 213564 - [iOS] -_requestTextInputContextsInRect cannot find empty Quip spreadsheet title
Summary: [iOS] -_requestTextInputContextsInRect cannot find empty Quip spreadsheet title
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-24 09:39 PDT by Daniel Bates
Modified: 2020-06-25 15:04 PDT (History)
5 users (show)

See Also:


Attachments
Patch (12.27 KB, patch)
2020-06-24 12:59 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To land (11.84 KB, patch)
2020-06-25 15:00 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To land (11.79 KB, patch)
2020-06-25 15:01 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
To land (11.80 KB, patch)
2020-06-25 15:02 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-06-24 09:39:25 PDT
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.
Comment 1 Daniel Bates 2020-06-24 09:39:40 PDT
<rdar://problem/59355847>
Comment 2 Daniel Bates 2020-06-24 12:59:12 PDT
Created attachment 402674 [details]
Patch
Comment 3 Daniel Bates 2020-06-25 14:27:32 PDT
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
Comment 4 Daniel Bates 2020-06-25 14:28:32 PDT
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
Comment 5 Simon Fraser (smfr) 2020-06-25 14:28:36 PDT
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?
Comment 6 Daniel Bates 2020-06-25 14:31:07 PDT
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.
Comment 7 Daniel Bates 2020-06-25 14:38:54 PDT
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.
Comment 8 Daniel Bates 2020-06-25 14:45:28 PDT
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.
Comment 9 Daniel Bates 2020-06-25 14:45:45 PDT
Thanks Simon!
Comment 10 Daniel Bates 2020-06-25 14:48:12 PDT
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
Comment 11 Daniel Bates 2020-06-25 15:00:04 PDT
Created attachment 402815 [details]
To land
Comment 12 Darin Adler 2020-06-25 15:00:30 PDT
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!
Comment 13 Daniel Bates 2020-06-25 15:01:48 PDT
Created attachment 402816 [details]
To land
Comment 14 Daniel Bates 2020-06-25 15:02:49 PDT
Created attachment 402818 [details]
To land
Comment 15 Daniel Bates 2020-06-25 15:03:28 PDT
Committed r263531: <https://trac.webkit.org/changeset/263531>
Comment 16 Daniel Bates 2020-06-25 15:04:55 PDT
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.