Bug 48151 – Add a new version of setSelection that fires selectionchange event

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

[Ryosuke Niwa]

Add a wrapper for setSelection that also fires selectionchange event.  We intend to migrate most of calls to setSelection to this new version.

[Alexey Proskuryakov]

The feature probably needs to be under an ifdef during development, so that no one accidentally ships selectionchange that _sometimes_ works.

[Ryosuke Niwa]

(In reply to comment #1)
> The feature probably needs to be under an ifdef during development, so that no one accidentally ships selectionchange that _sometimes_ works.

Yes, that is the plan.  I'm currently working on that.  I have wrapped all the code I've added so far by ENABLE(SELECTIONCHANGE_EVENT) but I can't figure out how to enable it locally.  Is there a command-line option to build-webkit?  Or do I define the constant in XCode?

[Alexey Proskuryakov]

The default is in WebCore/Configurations/FeatureDefines.xcconfig. Command line options can be added to build-webkit script.

[Ryosuke Niwa]

Created attachment 71785 [details]
Patch

[Ryosuke Niwa]

Created attachment 73159 [details]
implements setSelectionFiringEvent with a test

[Alexey Proskuryakov]

Comment on attachment 73159 [details]
implements setSelectionFiringEvent with a test

+void SelectionController::setSelectionFiringEvent(const VisibleSelection& s, bool closeTyping, bool shouldClearTypingStyle, bool userTriggered, CursorAlignOnScroll align, TextGranularity granularity, DirectionalityPolicy directionalityPolicy)

I've been reading this as "set SelectionFiring event" until I saw the implementation. Maybe setSelectionWithEvent()? Or combine the options into a structure, and add that to it?


+#if ENABLE(SELECTIONCHANGE_EVENT)
+    m_frame->document()->dispatchEvent(Event::create(eventNames().selectionchangeEvent, false, false));
+#endif

Can m_frame be null? SelectionController::setSelection() has a null check for it.


-            m_frame->selection()->setSelection(newSelection, granularity, MakeNonDirectionalSelection);
+            m_frame->selection()->setSelectionFiringEvent(newSelection, granularity, MakeNonDirectionalSelection);

I don't know how to review such changes for security impact.

+FAIL: double clicking first word of "<a href="javascript:false">hello</a>" yielded 0 selectionchange events but expected 2 events

The test is not enabled on any platforms, why check in a FAIL result? That should be a at least explained in ChangeLog.

[Darin Adler]

Comment on attachment 73159 [details]
implements setSelectionFiringEvent with a test

The naming here is not great. The normal version of the function is the one that sends events. The current version of the function which does not send events is the “buggy” one. I think first step here should be renaming the current one to include the word “deprecated” or “without sending events” or both.

We don’t want to leave a rename as the last step if we can help it.

[Ryosuke Niwa]

(In reply to comment #6)
> (From update of attachment 73159 [details])
> I've been reading this as "set SelectionFiring event" until I saw the implementation. Maybe setSelectionWithEvent()? Or combine the options into a structure, and add that to it?

Oh yeah, that's a good point now that you pointed out.  I can't step reading it that way.  Will fix.

> +#if ENABLE(SELECTIONCHANGE_EVENT)
> +    m_frame->document()->dispatchEvent(Event::create(eventNames().selectionchangeEvent, false, false));
> +#endif
> 
> Can m_frame be null? SelectionController::setSelection() has a null check for it.

Will fix.

> -            m_frame->selection()->setSelection(newSelection, granularity, MakeNonDirectionalSelection);
> +            m_frame->selection()->setSelectionFiringEvent(newSelection, granularity, MakeNonDirectionalSelection);
> 
> I don't know how to review such changes for security impact.

I added a comment to each test case indicating which case corresponds to which.  The only function I haven't been able to test is selectClosestWordOrLinkFromMouseEvent.  I ran all layout tests with ASSERT_NOT_REACHED but it never hit the assert.  It's quite possible that it's dead code.

> +FAIL: double clicking first word of "<a href="javascript:false">hello</a>" yielded 0 selectionchange events but expected 2 events
> 
> The test is not enabled on any platforms, why check in a FAIL result? That should be a at least explained in ChangeLog.

I'll just remove this test.

(In reply to comment #7)
> (From update of attachment 73159 [details])
> The naming here is not great. The normal version of the function is the one that sends events. The current version of the function which does not send events is the “buggy” one. I think first step here should be renaming the current one to include the word “deprecated” or “without sending events” or both.

Will rename setSelection to deprecatedSetSelection.  I'd like to keep the name setSelectionWithEvent for this very first patch.  Because if I had used setSelection in this patch, reviewers need to go through every single file to make sure I caught them all.  Calling it setSelectionWithEvent will avoid this because it'll show up as a compilation error if I had forgotten any.

[Ryosuke Niwa]

Created attachment 73295 [details]
fixed per comments

[Eric Seidel (no email)]

Attachment 73295 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5431038