Bug 201618 - Tap and hold on Facebook sometimes creates a tall empty selection
Summary: Tap and hold on Facebook sometimes creates a tall empty selection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Timothy Hatcher
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-09 16:36 PDT by Timothy Hatcher
Modified: 2019-09-09 20:50 PDT (History)
6 users (show)

See Also:

Attachments
Patch (10.50 KB, patch)
2019-09-09 16:45 PDT, Timothy Hatcher 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2019-09-09 16:36:58 PDT 
Most of Facebook has `-webkit-user-select: none`. The selection being created is spanning two of the only elements that are still user selectable — a user avatar icon in the story bar at the top, and the loading icon at the bottom.
Comment 1 Timothy Hatcher 2019-09-09 16:37:13 PDT Comment hidden (obsolete)
Comment 2 Timothy Hatcher 2019-09-09 16:37:13 PDT 
<rdar://53630145>
Comment 3 Timothy Hatcher 2019-09-09 16:45:47 PDT 
Created attachment 378417 [details]
Patch
Comment 4 Megan Gardner 2019-09-09 17:26:08 PDT 
Comment on attachment 378417 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +

Do we know why this is no longer needed? That is my only qualm about this patch, I'm not following why we don't need this anymore. What changed to make it not needed.
Comment 5 Alexey Proskuryakov 2019-09-09 18:15:25 PDT 
Comment on attachment 378417 [details]
Patch

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

> Tools/ChangeLog:10
> +        * TestWebKitAPI/Tests/ios/SelectionByWord.mm: Added.

What necessitates was sing an API tests? Those are less scalable and have weaker infrastructure around them, so layout tests are preferred whenever possible.
Comment 6 Timothy Hatcher 2019-09-09 19:13:54 PDT 
Comment on attachment 378417 [details]
Patch

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

>> Source/WebCore/ChangeLog:16
>> +
> 
> Do we know why this is no longer needed? That is my only qualm about this patch, I'm not following why we don't need this anymore. What changed to make it not needed.

This code dates back to the original iOS upstream in 2014. The code was originally added as-is in 2013 when wordRangeFromPosition() was introduced, not as part of a bug fix. It was landed without a test, so I don't know anymore details on why it was originally needed.

VisualPosition and Position changes since then have likely made it not necessary and broken in cases like we see today on Facebook.
Comment 7 Timothy Hatcher 2019-09-09 19:14:34 PDT 
Comment on attachment 378417 [details]
Patch

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

>> Tools/ChangeLog:10
>> +        * TestWebKitAPI/Tests/ios/SelectionByWord.mm: Added.
> 
> What necessitates was sing an API tests? Those are less scalable and have weaker infrastructure around them, so layout tests are preferred whenever possible.

I wasn't able to get this to reproduce as needed in a LayoutTest -- I tried.
Comment 8 Megan Gardner 2019-09-09 19:46:12 PDT 
With the additional comments, r+
Comment 9 WebKit Commit Bot 2019-09-09 20:50:10 PDT 
Comment on attachment 378417 [details]
Patch

Clearing flags on attachment: 378417

Committed r249701: <https://trac.webkit.org/changeset/249701>
Comment 10 WebKit Commit Bot 2019-09-09 20:50:12 PDT 
All reviewed patches have been landed.  Closing bug.