Bug 216656

Summary: Selection API: Introduce LiveRangeSelectionEnabled, off by default
Product: WebKit Reporter: Darin Adler <darin>
Component: HTML EditingAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, esprehn+autocc, ews-watchlist, kangil.han, kondapallykalyan, mifenton, rniwa, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=189164
Bug Depends on:    
Bug Blocks: 216325    
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch none

Description Darin Adler 2020-09-17 11:20:54 PDT 
Selection API as documented in https://www.w3.org/TR/selection-api/ calls for behavior where the range returned is a live range that both reflects changes to the selection and can be modified to change the selection. This is a significant first step in implementing that, with a few loose ends remaining.
Comment 1 Darin Adler 2020-09-17 17:01:11 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-09-17 17:02:02 PDT 
There are a few small changes from the patch that Sam reviewed in bug 216325, including things that addressed his feedback and Alex’s as well, and some other small tweaks.
Comment 3 Darin Adler 2020-09-17 17:35:45 PDT 
Created attachment 409093 [details]
Patch
Comment 4 Darin Adler 2020-09-17 18:45:06 PDT 
Committed r267220: <https://trac.webkit.org/changeset/267220>
Comment 5 Radar WebKit Bug Importer 2020-09-17 18:46:17 PDT 
<rdar://problem/69111988>
Comment 6 Ryosuke Niwa 2020-09-17 19:21:49 PDT 
Comment on attachment 409093 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:1522
> +LayoutUnit FrameSelection::lineDirectionPointForBlockDirectionNavigation(PositionType type)

Can we convert PositionType to enum class?

> Source/WebCore/editing/FrameSelection.cpp:2795
> +        }

Need a FIXME here to keep the JS wrapper of this range alive here too.

> Source/WebCore/editing/FrameSelection.cpp:2812
> +    updateFromAssociatedLiveRange();

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:2829
> +        disassociateLiveRange();

Like I commented in the other bug, I think it would be useful to leave a comment here explaining when this case will be hit.

> Source/WebCore/editing/FrameSelection.h:52
> +enum EUserTriggered : bool { NotUserTriggered, UserTriggered };
> +enum RevealExtentOption : bool { RevealExtent, DoNotRevealExtent };

Can we use enum class for these?

> Source/WebCore/editing/FrameSelection.h:313
> +    WeakPtr<Document> m_document;

Nice
Comment 7 Darin Adler 2020-09-18 08:58:27 PDT 
Comment on attachment 409093 [details]
Patch

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

>> Source/WebCore/editing/FrameSelection.cpp:1522
>> +LayoutUnit FrameSelection::lineDirectionPointForBlockDirectionNavigation(PositionType type)
> 
> Can we convert PositionType to enum class?

OK, I will do that in a follow-up.

>> Source/WebCore/editing/FrameSelection.cpp:2795
>> +        }
> 
> Need a FIXME here to keep the JS wrapper of this range alive here too.

I think we only need one FIXME, in associateLiveRange. Not three in three different functions. Not sure why I put it in disassociateLiveRange!

In any case, I am mostly done with the patch that resolves the problem now, so figuring out how many FIXMEs we want and where is a very short term issue.

>> Source/WebCore/editing/FrameSelection.cpp:2829
>> +        disassociateLiveRange();
> 
> Like I commented in the other bug, I think it would be useful to leave a comment here explaining when this case will be hit.

Yes, sorry, I forgot. Will add a comment.

>> Source/WebCore/editing/FrameSelection.h:52
>> +enum RevealExtentOption : bool { RevealExtent, DoNotRevealExtent };
> 
> Can we use enum class for these?

OK. Will do. Also rename EUserTriggered to remove the E.