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.
Created attachment 409090 [details] Patch
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.
Created attachment 409093 [details] Patch
Committed r267220: <https://trac.webkit.org/changeset/267220>
<rdar://problem/69111988>
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 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.