Bug 60376 – selectstart event does not fire when selection is made via select all

• Component: UI Events
• Status: RESOLVED FIXED

[Tim Down]

Created attachment 92590 [details]
Test case

The selectstart event fires (many times) when making a mouse selection or a keyboard selection but does not fire at all when the selection changes using "Select all" options from context/edit menus or via Ctrl/Cmd-A keyboard shortcut.

[Alexey Proskuryakov]

Does it fire in IE?

[Tim Down]

Yes, it fires in IE.

[Ryosuke Niwa]

(In reply to comment #1)
> Does it fire in IE?

IE9 doesn't.

[Tim Down]

I only have Win XP at home so I can't test IE 9. It fires in IE 6 and 7. My test case isn't great: in IE you need to make sure you've got the focus on the body (eg by clicking on the text) before doing the select all.

[Alexey Proskuryakov]

If you need this to prevent selection, please note that WebKit supports a better way to do that (that is, -webkit-user-select CSS property).

[Tim Down]

Nothing to do with preventing selection. I do quite a bit of work with contenteditable/designMode and I'm interested in being notified whenever the selection is modified by any means: mouse, keyboard, menu option.

[Ryosuke Niwa]

(In reply to comment #6)
> Nothing to do with preventing selection. I do quite a bit of work with contenteditable/designMode and I'm interested in being notified whenever the selection is modified by any means: mouse, keyboard, menu option.

Does selectionchange work for your use case?  Since IE9 no longer fires selectstart when selecting all, I'm not sure we should change WebKit's behavior here.

[Ryosuke Niwa]

(In reply to comment #4)
> in IE you need to make sure you've got the focus on the body (eg by clicking on the text) before doing the select all.

Ah, thanks for pointing that out.  Now IE9 fires selectstart as well.

[Tim Down]

Frankly I don't care which event fires so long as one does, so selectionchange is fine by me. I assume this change is recent enough that I'll need to test against a nightly?

[Ryosuke Niwa]

Now that I know IE9 also fires we should definitely fix this bug.

(In reply to comment #9)
> Frankly I don't care which event fires so long as one does, so selectionchange is fine by me. I assume this change is recent enough that I'll need to test against a nightly?

It's shipped in Chrome 11.

[Ryosuke Niwa]

Created attachment 92681 [details]
fixes the bug

[Ryosuke Niwa]

Created attachment 92682 [details]
patch with tests

[Kent Tamura]

Comment on attachment 92682 [details]
patch with tests

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

> LayoutTests/fast/events/selectstart-on-selectall.html:13
> +var selectstartEvent = null;

This variable is not used.

> Source/WebCore/editing/FrameSelection.cpp:1403
> +            selectElement->selectAll();

Need "return" after this line?

[Ryosuke Niwa]

Created attachment 92732 [details]
Fixed per comments

[Ryosuke Niwa]

(In reply to comment #13)
> (From update of attachment 92682 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92682&action=review
> 
> > LayoutTests/fast/events/selectstart-on-selectall.html:13
> > +var selectstartEvent = null;
> 
> This variable is not used.

Removed.

> > Source/WebCore/editing/FrameSelection.cpp:1403
> > +            selectElement->selectAll();
> 
> Need "return" after this line?

Oops, a good catch!

[Kent Tamura]

Comment on attachment 92732 [details]
Fixed per comments

Looks good.

[Ryosuke Niwa]

(In reply to comment #16)
> (From update of attachment 92732 [details])
> Looks good.

Thanks for the review!

[Ryosuke Niwa]

Committed r86041: <http://trac.webkit.org/changeset/86041>

[WebKit Review Bot]

http://trac.webkit.org/changeset/86041 might have broken Leopard Intel Debug (Tests)
The following tests are not passing:
fast/workers/storage/use-same-database-in-page-and-workers.html
http/tests/appcache/fail-on-update-2.html
http/tests/appcache/fail-on-update.html
http/tests/misc/favicon-loads-with-icon-loading-override.html
storage/domstorage/localstorage/storagetracker/storage-tracker-2-create.html
storage/domstorage/localstorage/storagetracker/storage-tracker-3-delete-all.html
svg/animations/animate-path-nested-transforms.html