Bug 216656 – Selection API: Introduce LiveRangeSelectionEnabled, off by default

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

[Darin Adler]

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.

[Darin Adler]

Created attachment 409090 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

Created attachment 409093 [details]
Patch

[Darin Adler]

Committed r267220: <https://trac.webkit.org/changeset/267220>

[Radar WebKit Bug Importer]

<rdar://problem/69111988>

[Ryosuke Niwa]

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

[Darin Adler]

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.