Bug 60376 - selectstart event does not fire when selection is made via select all
Summary: selectstart event does not fire when selection is made via select all
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 60424
  Show dependency treegraph
 
Reported: 2011-05-06 09:42 PDT by Tim Down
Modified: 2011-05-09 03:41 PDT (History)
11 users (show)

See Also:


Attachments
Test case (278 bytes, text/html)
2011-05-06 09:42 PDT, Tim Down
no flags Details
fixes the bug (6.11 KB, patch)
2011-05-06 19:54 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
patch with tests (11.72 KB, patch)
2011-05-06 19:58 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per comments (11.37 KB, patch)
2011-05-07 23:52 PDT, Ryosuke Niwa
tkent: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Down 2011-05-06 09:42:26 PDT
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.
Comment 1 Alexey Proskuryakov 2011-05-06 17:19:48 PDT
Does it fire in IE?
Comment 2 Tim Down 2011-05-06 17:30:45 PDT
Yes, it fires in IE.
Comment 3 Ryosuke Niwa 2011-05-06 17:31:02 PDT
(In reply to comment #1)
> Does it fire in IE?

IE9 doesn't.
Comment 4 Tim Down 2011-05-06 17:34:56 PDT
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.
Comment 5 Alexey Proskuryakov 2011-05-06 17:40:50 PDT
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).
Comment 6 Tim Down 2011-05-06 17:43:16 PDT
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.
Comment 7 Ryosuke Niwa 2011-05-06 17:46:55 PDT
(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.
Comment 8 Ryosuke Niwa 2011-05-06 17:48:27 PDT
(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.
Comment 9 Tim Down 2011-05-06 17:51:01 PDT
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?
Comment 10 Ryosuke Niwa 2011-05-06 18:03:13 PDT
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.
Comment 11 Ryosuke Niwa 2011-05-06 19:54:43 PDT
Created attachment 92681 [details]
fixes the bug
Comment 12 Ryosuke Niwa 2011-05-06 19:58:32 PDT
Created attachment 92682 [details]
patch with tests
Comment 13 Kent Tamura 2011-05-07 20:46:28 PDT
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?
Comment 14 Ryosuke Niwa 2011-05-07 23:52:02 PDT
Created attachment 92732 [details]
Fixed per comments
Comment 15 Ryosuke Niwa 2011-05-07 23:52:54 PDT
(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!
Comment 16 Kent Tamura 2011-05-08 17:43:08 PDT
Comment on attachment 92732 [details]
Fixed per comments

Looks good.
Comment 17 Ryosuke Niwa 2011-05-08 19:15:26 PDT
(In reply to comment #16)
> (From update of attachment 92732 [details])
> Looks good.

Thanks for the review!
Comment 18 Ryosuke Niwa 2011-05-08 20:29:48 PDT
Committed r86041: <http://trac.webkit.org/changeset/86041>
Comment 19 WebKit Review Bot 2011-05-09 03:41:08 PDT
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