Last Comment Bug 496275 - Expose JS API for modifying page's selection (window.getSelection().modify())
: Expose JS API for modifying page's selection (window.getSelection().modify())
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.9.3a5
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 512295 581051 642823
Blocks: 716141
  Show dependency treegraph
 
Reported: 2009-06-04 00:30 PDT by Mark Tsui
Modified: 2013-04-04 13:53 PDT (History)
14 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
selection.modify example (867 bytes, text/html)
2009-08-20 20:49 PDT, Mark Tsui
no flags Details
Work in progress 0.1.1 (9.52 KB, patch)
2009-10-17 15:00 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch for review v1 (18.93 KB, patch)
2009-11-06 18:27 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch for review v2 (18.14 KB, patch)
2009-11-21 00:38 PST, Justin Lebar (not reading bugmail)
mats: review-
Details | Diff | Splinter Review
Testcase #2 (1.73 KB, text/html)
2010-01-05 10:43 PST, Mats Palmgren (:mats)
no flags Details
Testcase #3 (1.45 KB, text/html; charset=windows-1255)
2010-01-05 10:45 PST, Mats Palmgren (:mats)
no flags Details
Patch v3 (22.41 KB, patch)
2010-02-10 23:17 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v4 (23.30 KB, patch)
2010-03-11 19:40 PST, Justin Lebar (not reading bugmail)
mats: review+
Details | Diff | Splinter Review
Patch v5 (23.62 KB, patch)
2010-03-23 13:22 PDT, Justin Lebar (not reading bugmail)
jst: superreview+
Details | Diff | Splinter Review
Patch for checkin (24.05 KB, patch)
2010-03-24 13:57 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review

Description Mark Tsui 2009-06-04 00:30:49 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_6; en-us) AppleWebKit/525.27.1 (KHTML, like Gecko) Version/3.2.1 Safari/525.27.1
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090521 Minefield/3.6a1pre

We can get the access to the selectionController in extensions (via nsISelectionController), but not in webpages. Specifically, I'd like to be able to programmatically move the selection inside contentEditable, i.e. wordMove, lineMove etc..

Webkit exposes something similar (but not published) via window.getSelection.modify("move", "forward", "word") etc.. 

It'd be great if firefox can expose programmatic selection movement, and even better if it can be speced out in html5 for a browser-independent solution.

Reproducible: Always
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-06-04 02:55:49 PDT
This would be pretty easy to do. However, I don't think exposing nsISelectionController is the way to go. A browser-independent selection and caret navigation API that fits as gracefully as possible into the existing editor commands should be added to HTML5, then implementing it would be pretty easy for us I think. Do you have a proposal for that?
Comment 2 Mark Tsui 2009-06-11 01:08:34 PDT
I tried using Webkit's extension to the selection interface, and it seems quite flexible.

Webkit adds this method to their Selection interface:

void modify(in DOMString alter, in DOMString direction, in DOMString granularity);

This method modifies the Selection object, but not the position of the actual caret. To move the caret, the user needs to getSelection(), modify the selection object, and apply the new selection via removeAllRanges and addRange.

alter can be { "move", "extend" }
- "move" collapses the selection to the end of the selection and applies the movement direction/granularity to the collapsed selection.
- "extend" leaves the start of the selection unchanged, and applies movement direction/granularity to the end of the selection.

direction can be { "forward", "backward", "left", "right" }
- "left" and "right" are self-explanatory
- "forward" and "backward" describes movement relative to the ordering of content in the DOM. This is important in RTL languages where "forward" moves the caret right, and "backward" moves the caret left

granularity can be { "character", "word", "sentence", "line", "paragraph", "lineboundary", "sentenceboundary", "paragraphboundary", "documentboundary" }
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2009-08-20 17:14:40 PDT
Confirming, and cc:ing Justin who will start investigating this.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2009-08-20 17:15:52 PDT
And I think I'd rather see us emulate what WebKit does here and advocate for including a specification for that in HTML5 than seeing us expose our internal selection controller objects to webpages.
Comment 5 Justin Lebar (not reading bugmail) 2009-08-20 18:15:23 PDT
I'm sure I'm doing something wrong here, but I haven't been able to get this interface to work in Chrome 2.0.172.39 on Windows.  Here's what I'm doing:

 * Load a page, select some text
 * From the JS console, type
     var sel = window.getSelection()
     sel.modify("move", "forward", "word")
   This clears the visible selection.
 * I've tried a bunch of things after here and nothing seems to bring the selection back.  My best guess is:
     var range = sel.getRangeAt(0)
     sel.removeAllRanges()
     sel.addRange(range)

At this point, there's no visible selection on the page.

What am I doing wrong here?
Comment 6 Mark Tsui 2009-08-20 20:49:05 PDT
Created attachment 395769 [details]
selection.modify example
Comment 7 Mark Tsui 2009-08-20 21:18:11 PDT
Turned out I was wrong about the selection.modify not moving the caret.

I've uploaded an example that I've tested with Chrome linux 3.0.198.1
It should move the caret every 2 seconds according to the parameters.
Comment 8 Justin Lebar (not reading bugmail) 2009-08-21 10:12:51 PDT
Works on Chrome 2.0.172.39 on Windows.  Thanks, Mark.
Comment 9 Justin Lebar (not reading bugmail) 2009-10-14 18:35:55 PDT
To those of you who will be using this feature: It'll be much more likely that I can complete this in a timely manner if we can reduce the API's size to the bare minimum required and if we can nail down exactly what the remaining pieces of the spec mean.

For instance, which of the granularities are must-haves?  If we keep "sentence" and "paragraph", I'd like to define exactly what they mean, but I'd also love to ignore them.  (I'm not a web developer, but since "paragraph" and "sentence" mean different things to different people, I imagine developers might implement those in terms of the other primitives themselves.)

Which parts of this are essential, and which parts (if any) can we throw out?
Comment 10 Mark Tsui 2009-10-14 19:46:55 PDT
"character" and "word" are must-haves. I'd really like "sentence", but I understand the difficulty in specifying exactly what that means.

One reason for pushing this functionality as part of the API is that there is significant logic involved for making word/sentence boundaries work for different languages. The browser can leverage external libraries for this. This behaviour is "best-effort" as suitable libraries may not be available on all platforms and there may also be inconsistent behaviour between these libraries. I think this would be much better than implementing an ICU library in javascript.
Comment 11 Justin Lebar (not reading bugmail) 2009-10-14 19:57:02 PDT
(In reply to comment #10)

Sounds good.  I'll concentrate on character and word for now, and we can revisit the other granularities once I make those work.
Comment 12 Ojan Vafai 2009-10-15 13:23:22 PDT
I believe the primary use-case here is to be able to mimic the behavior of user-initiated keyboard-based selections (e.g. ctrl+shift+right-arrow). In that context, "character", "word" and "lineboundary" are must haves. "line" and "documentboundary" are nice to haves.

I don't see any use-case for "sentence", "paragraph", "sentenceboundary" or "paragraphboundary". Mark, what was your use-case for "sentence"?
Comment 13 Daniel Danilatos 2009-10-15 15:37:29 PDT
Yep, that's right. Basically any form of movement corresponding to movements possible with the keyboard, that would be complicated to re-implement with javascript. Just to be clear, "character" (probably ill-named) doesn't map exactly to characters - two UTF16 surrogate pairs should count as a 'character' for such purposes, as should some pairs of unicode code points where one is a combining character, such as in arabic "تَ". I think there's a separate bug in FF where you can get your cursor between the two code points in that arabic example, but what's important is that the behaviour of selection.modify matches the behaviour of left/right arrow keys.

Another thing though: I believe (and Ojan agrees) that this API should be on text ranges and not on the selection object. It's more general, and for what it's worth is what IE does.
Comment 14 Mark Tsui 2009-10-15 16:57:12 PDT
Suppose, we want to provide a navigation mechanism to jump to the next sentence, that would be difficult to implement in javascript. However, I agree that it is not an important requirement right now.
Comment 15 Justin Lebar (not reading bugmail) 2009-10-17 15:00:30 PDT
Created attachment 406888 [details] [diff] [review]
Work in progress 0.1.1

Work in progress patch.  Implements "character", "word", "line", and "lineboundary" granularities.

Remaining to be done is RTL support.
Comment 16 Justin Lebar (not reading bugmail) 2009-11-06 18:27:50 PST
Created attachment 410924 [details] [diff] [review]
Patch for review v1

Patch for review.

There's some platform-specific behavior in the mochitest that I've only been able to test on Linux; tryserver isn't cooperating right now.  The test should also work on Mac.  Once I get my hands on a Windows build, I'll make sure the test works on there too.

I'm also generating some new warnings, but I'm not sure what's going on:

layout/generic/nsSelection.cpp:6407: warning: enumeral mismatch in conditional expression: ‘nsIDOMKeyEvent::<anonymous enum>’ vs ‘nsIDOMKeyEvent::<anonymous  enum>’
Comment 17 Justin Lebar (not reading bugmail) 2009-11-09 19:15:30 PST
Comment on attachment 410924 [details] [diff] [review]
Patch for review v1

Hm...  Try is exposing some problems.  Taking down the review request for now.
Comment 18 Justin Lebar (not reading bugmail) 2009-11-21 00:38:19 PST
Created attachment 413790 [details] [diff] [review]
Patch for review v2

Patch for review.  This still has the compile warnings from comment 16 that I'd like to get rid of.
Comment 19 Mats Palmgren (:mats) 2009-11-30 20:17:28 PST
> content/base/public/nsISelection.idl
>  * @status FROZEN

Uhm, doesn't that mean we can't change it? (ever)
Comment 20 Mats Palmgren (:mats) 2010-01-05 10:43:06 PST
Created attachment 420111 [details]
Testcase #2
Comment 21 Mats Palmgren (:mats) 2010-01-05 10:45:16 PST
Created attachment 420113 [details]
Testcase #3
Comment 22 Mats Palmgren (:mats) 2010-01-05 10:50:30 PST
Comment on attachment 413790 [details] [diff] [review]
Patch for review v2

> layout/generic/nsSelection.cpp

+  if(aGranularity == CHARACTER) {

Please add a space after 'if' (in a few places).

+  // line moves are always absolute

The term "absolute" isn't previously used in this file.
I think you mean "visual".

+  PRBool absolute = (aDirection == LEFT ...
+  PRBool forward = (aDirection == FORWARD ...
+  PRBool extend = (aAlter == EXTEND);

The parenthesis are redundant.


+NS_IMETHODIMP
+nsTypedSelection::Modify(const nsAString& aAlter, const nsAString& aDirection,
+                         const nsAString& aGranularity) {
+

Move the brace to the next line.

The code looks good otherwise, but there are a number of differences when
comparing with Safari (4.0.4)...

For example,
1. when extending the selection forward to "lineboundary" twice, Safari
   selects the next line whereas we stop at end of the first line.
2. when extending a selection forward where the anchor offset > focus offset,
   Safari first swaps them so that the current range is still selected.
   (and vice versa when selecting backwards)
See Testcase #2.

3. in many cases when moving or extending the selection forward at the end
  (or backward at the beginning) of the document, we throw an exception
  (NS_ERROR_FAILURE), whereas Safari is silent and doesn't modify the
  current selection/position.  We also throw when there is no selection,
  e.g. selection.removeAllRanges() or selection.getRangeAt(0).detach(),
  Safari is silent in those cases.

4. modify("extend","forward","line") on the last line results in the
   selection being extended to the end of the line in Safari,
   we throw NS_ERROR_FAILURE.

5. in RTL text, Safari moves/extends in the logical direction for
   forward/backward and visual direction for left/right, we should do
   the same.
See Testcase #3


r- because I suspect some of the Safari parity is unintentional.
Comment 23 Justin Lebar (not reading bugmail) 2010-01-05 11:13:10 PST
Thanks for the review, Mats.

I agree that we should resolve 2-5 to match Safari.  But I'm not sure that we want to duplicate Webkit's behavior in case 1.  In Chrome and Safari on my Windows box, pressing <end> twice in a textbox leaves me at the end of the current line, while pressing <shift>+<end> twice selects two lines.  In Firefox, <end> and <shift>+<end> always stop at the current line.

The Webkit behavior here seems pretty strange to me.
Comment 24 Ojan Vafai 2010-01-08 18:10:52 PST
I think 1 is likely a WebKit bug. I've filed https://bugs.webkit.org/show_bug.cgi?id=33413 for it.

2 is a Mac-ism. I intend to soon post a WebKit patch making that behavior only happen on the Mac. It's a little weird to me that these APIs would do something different on different platforms, but they're supposed to match the platform selection behavior, right?

3-5 I agree should be fixed to match Safari.
Comment 25 Justin Lebar (not reading bugmail) 2010-01-09 12:14:41 PST
(In response to comment 22)
> We also throw when there is no selection, e.g. selection.removeAllRanges() or 
> selection.getRangeAt(0).detach(), Safari is silent in those cases.

Do we really want to silently ignore this case?  I don't really care either way, but this seems perhaps more appropriate than throwing an exception when a selection exists but we can't move in the specified direction.

Also WebKit silently ignores invalid parameters to Selection.modify() (try opening testcase 3 and changing one of the params), but I think we should continue throwing an exception in this case.
Comment 26 Justin Lebar (not reading bugmail) 2010-01-09 15:37:49 PST
Another WebKit bug: If the cursor is in an RTL div, modify("move", "right", "lineboundary") actually moves the cursor left.  https://bugs.webkit.org/show_bug.cgi?id=33435

I actually made the same mistake.  :)
Comment 27 Justin Lebar (not reading bugmail) 2010-01-09 16:59:40 PST
(re comment 22)
> 4. modify("extend","forward","line") on the last line results in the
>    selection being extended to the end of the line in Safari,
>    we throw NS_ERROR_FAILURE.

I think bug 512295 may block this.  If you're at the top line of a textbox and press <up>, the cursor goes to the first character of the textbox.  If you're at the top line of a content-editable div and press <up>, the cursor gets stuck.
Comment 28 Mats Palmgren (:mats) 2010-01-11 10:30:02 PST
(In reply to comment #24)
> 2 ... they're supposed to match the platform selection behavior, right?

I think modify() should do whatever the corresponding keyboard command
does on the platform, so yes.

(In reply to comment #25)
For the errors:
1. invalid parameters
2. valid but unimplemented parameters ("sentence" "paragraph" etc)
3. can't move/extend in the requested direction
4. Selection does not have a Range or it's detached
I would prefer 1:NS_ERROR_INVALID_ARG, 2:NS_ERROR_NOT_IMPLEMENTED,
3:no exception, 4:NS_ERROR_DOM_INVALID_STATE_ERR

Please file a WebKit bug to see if we can make them throw an exception
for 1 and 4.  If not, then I think we should be silent too, since
compatibility is more important.
Comment 29 Justin Lebar (not reading bugmail) 2010-01-11 19:57:58 PST
Filed: 
  https://bugs.webkit.org/show_bug.cgi?id=33509
  https://bugs.webkit.org/show_bug.cgi?id=33510
Comment 30 Justin Lebar (not reading bugmail) 2010-02-10 23:17:20 PST
Created attachment 426463 [details] [diff] [review]
Patch v3

To summarize what's changed:

From comment 22, #3, 4, and 5 are fixed.  I don't have access to a Mac right now, so I'm not sure what we do in #2, although I suspect we don't match the desired behavior.  I could add platform-specific code if necessary.

From comment 28:
> 1. invalid parameters
> 2. valid but unimplemented parameters ("sentence" "paragraph" etc)
> 3. can't move/extend in the requested direction
NS_ERROR_INVALID_ARG, NS_ERROR_NOT_IMPLEMENTED, and no error, per the comment

> 4. Selection does not have a Range or it's detached
It doesn't look like Webkit is going to throw an error in this case (https://bugs.webkit.org/show_bug.cgi?id=33510) so I silently fail here too.
Comment 31 Justin Lebar (not reading bugmail) 2010-03-10 22:34:31 PST
Hm...Webkit's arguments are case-insensitive.  Presumably we want to match them?
Comment 32 :Ehsan Akhgari 2010-03-11 09:08:24 PST
This looks great, Justin!  Do you have try-server builds?
Comment 33 :Ehsan Akhgari 2010-03-11 09:13:50 PST
I specifically want to make sure that the character granularity works correctly with combining characters (see comment 13):

ت + َ

which will be:

تَ

I think we specifically need tests for this case as well.

Also interesting would be handling of ligatures, such as:

ل + ا

which will be:

لا
Comment 34 :Ehsan Akhgari 2010-03-11 09:14:27 PST
CCing Johnathan because he may have more ideas along the lines of comment 33.
Comment 35 Justin Lebar (not reading bugmail) 2010-03-11 10:57:22 PST
Just to be clear, we should treat تَ as one character and لا as two, right?  It that's how they seem to behave in a regular text box.

I'm spinning up some tryserver builds and will post a link here when I'm done.
Comment 36 Ojan Vafai 2010-03-11 11:25:26 PST
(In reply to comment #12)
> I don't see any use-case for "sentence", "paragraph", "sentenceboundary" or
> "paragraphboundary". Mark, what was your use-case for "sentence"?

I've grown more familiar with webkit's use of these values. sentence and sentenceboundary are only used for OS X accessibility. paragraph and paragraphboundary do actually correspond to user-actions (triple-click, ctrl+shift+a, ctrl+shift+e). In Gecko, ctrl+shift+a and ctrl+shift+e correspond to lineboundary though.

In either case, it seems like implementing paragraph, paragraphboundary and documentboundary would be worthwhile followup patches.
Comment 37 Jonathan Kew (:jfkthame) 2010-03-11 11:42:44 PST
(In reply to comment #35)
> Just to be clear, we should treat تَ as one character and لا as two, right?

لا is definitely two characters; although it's a single glyph (in most Arabic fonts), it is clearly understood to be a ligature, like "fi" in (some) Latin fonts.

تَ is a more difficult case; it is a consonant + vowel sequence, where the vowel is a diacritic. Whether they are best treated as a single unit may depend on how it's being used. When selecting text with a GUI, it's not possible to select the diacritic separately in most systems; however, it does make sense to be able to address it as a separate unit for operations such as deleting or replacing a character in the text.
Comment 38 Justin Lebar (not reading bugmail) 2010-03-11 12:26:21 PST
Windows and Linux builds of the current patch (v3) are up at https://build.mozilla.org/tryserver-builds/justin.lebar@gmail.com-selection-8/  Mac builds should be done soon.
Comment 39 :Ehsan Akhgari 2010-03-11 14:40:27 PST
(In reply to comment #37)
> (In reply to comment #35)
> > Just to be clear, we should treat تَ as one character and لا as two, right?
> 
> لا is definitely two characters; although it's a single glyph (in most Arabic
> fonts), it is clearly understood to be a ligature, like "fi" in (some) Latin
> fonts.

Yes, that's right.

> تَ is a more difficult case; it is a consonant + vowel sequence, where the
> vowel is a diacritic. Whether they are best treated as a single unit may depend
> on how it's being used. When selecting text with a GUI, it's not possible to
> select the diacritic separately in most systems; however, it does make sense to
> be able to address it as a separate unit for operations such as deleting or
> replacing a character in the text.

Well, IINM this patch is all about manipulating selections and caret navigation.  For those purposes, تَ should be considered as one single character.  For character deletion, though, they should be treated as two characters, but I guess that doesn't matter here.

Is this how your patch handles this right now?
Comment 40 Justin Lebar (not reading bugmail) 2010-03-11 17:13:54 PST
(In reply to comment #39)
> Is this how your patch handles this right now?

The patch treats تَ as a single character and لا as two.  I'm adding tests for these anyway; I'll post the new patch in a few minutes.

One thing which is a little strange is that given the string "a تَ b" (for an arbitrary RTL character تَ), if the cursor is immediately in front of the تَ, the command "move forward character" will move the cursor to immediately in front of b, eating the space after the تَ.  "move right character" places the cursor immediately to the right of the تَ.

It seems like we're interpreting the space after the RTL character as part of the RTL span.  That seems strange to me, but a lot of the RTL behavior seems pretty strange to me.  :)

(FWIW, Webkit doesn't have this behavior; in this case, "move right character" and "move forward character" do what Firefox does with "move right character".)
Comment 41 :Ehsan Akhgari 2010-03-11 17:45:07 PST
(In reply to comment #40)
> (In reply to comment #39)
> > Is this how your patch handles this right now?
> 
> The patch treats تَ as a single character and لا as two.  I'm adding tests for
> these anyway; I'll post the new patch in a few minutes.

Good.

> One thing which is a little strange is that given the string "a تَ b" (for an
> arbitrary RTL character تَ), if the cursor is immediately in front of the تَ,
> the command "move forward character" will move the cursor to immediately in
> front of b, eating the space after the تَ.  "move right character" places the
> cursor immediately to the right of the تَ.

Then, this is a bug which should be fixed.  I think you can use the caret movements in editable fields as a sort of reference as to what should happen.

BTW, as someone with a bidi mind, it was very confusing to figure out that you meant "to the left of" by "in front of"! ;-)

> It seems like we're interpreting the space after the RTL character as part of
> the RTL span.  That seems strange to me, but a lot of the RTL behavior seems
> pretty strange to me.  :)
> 
> (FWIW, Webkit doesn't have this behavior; in this case, "move right character"
> and "move forward character" do what Firefox does with "move right character".)

As a general rule, in LTR, you should be able to s/right/forward/ and s/left/backward/ and get the equivalent behavior, and in RTL, you should be able to s/right/backward/ and s/left/forward/ and get the equivalent behavior.
Comment 42 Justin Lebar (not reading bugmail) 2010-03-11 19:36:58 PST
(In reply to comment #41)
> I think you can use the caret movements in editable fields as a sort of
> reference as to what should happen.

> As a general rule, in LTR, you should be able to s/right/forward/ and
> s/left/backward/ and get the equivalent behavior, and in RTL, you should be
> able to s/right/backward/ and s/left/forward/ and get the equivalent behavior.

The question is, is the space to the right of the تَ an LTR or RTL character?  In any case, I don't think this is a bug with the patch here; it happens when using the arrow keys in a contenteditable.  I've filed bug 551877.
Comment 43 Justin Lebar (not reading bugmail) 2010-03-11 19:40:13 PST
Created attachment 432058 [details] [diff] [review]
Patch v4

Adding case-insensitivity, a test for the 'تَ' and 'لا' characters.  Also updating the documentation on nsISelection::Modify.
Comment 44 Mats Palmgren (:mats) 2010-03-23 10:55:15 PDT
Comment on attachment 432058 [details] [diff] [review]
Patch v4

Nits:
In nsISelection.idl:
Please add to the comment that the values are case-insensitive.

In layout/generic/nsSelection.cpp:
> +  nsIFrame *frame;
> +  PRInt32 offset;
> +  rv = GetPrimaryFrameForFocusNode(&frame, &offset, visual);
> +  if (frame) {

'frame' has an undefined value if GetPrimaryFrameForFocusNode fails.
Please change it to "if (NS_SUCCEEDED(rv) && frame)".

It would be nice to have a test for a range that's been
detached (selection.getRangeAt(0).detach())

r=mats with the above nits addressed


Since this is an API change we need superreview as well.
There's a couple of issues that I'll defer to the sr:

In nsISelection.idl:
>   * @status FROZEN

I don't think we can add the new method here.

In layout/generic/nsSelection.cpp:
> +nsTypedSelection::Modify(...
> +  // Silently exit if there's no selection.
> +  if (!mFrameSelection || !GetAnchorFocusRange()) {
> +    return NS_OK;
> +  }

This is a minor issue, but I would prefer to return
NS_ERROR_DOM_INVALID_STATE_ERR when there is no selection
or the range has been detached. As we argue in:
https://bugs.webkit.org/show_bug.cgi?id=33510

The patch in https://bugs.webkit.org/show_bug.cgi?id=33509
changes the webkit IDL to "modify(...) raises(DOMException)"
so they seem to be accepting that modify() can throw...
If that patch is accepted, I think the argument in 
https://bugs.webkit.org/show_bug.cgi?id=33510#c2 is weakened.

Justin, thanks for the great work and for arguing our case
to webkit.
Comment 45 Justin Lebar (not reading bugmail) 2010-03-23 12:56:04 PDT
Thanks for the review, Mats!

It looks like detach() is a no-op in Chrome, so Chrome is perfectly happy to run modify() right after a detach().  This patch has us throwing an exception if modify() is called after detach, but I'll change it so we silently fail, like we do when there's no selection.
Comment 46 Justin Lebar (not reading bugmail) 2010-03-23 13:22:30 PDT
Created attachment 434324 [details] [diff] [review]
Patch v5

Addressing Mats's review in comment 44.
Comment 47 Johnny Stenback (:jst, jst@mozilla.com) 2010-03-23 14:39:51 PDT
Comment on attachment 434324 [details] [diff] [review]
Patch v5

Looks good!
Comment 48 Justin Lebar (not reading bugmail) 2010-03-23 15:29:45 PDT
Mats, do you think we should go ahead and check this in as-is, or should we try and push WebKit a little more on getting an exception when there's no selection?
Comment 49 Mats Palmgren (:mats) 2010-03-23 18:44:18 PDT
I think it's fine to land as-is, thanks.
Comment 50 Justin Lebar (not reading bugmail) 2010-03-24 11:01:43 PDT
Some minor fixes to the test on Windows are coming.  Once tryserver comes back green, I'll post the changes and we should be all set to go.
Comment 51 Justin Lebar (not reading bugmail) 2010-03-24 13:57:06 PDT
Created attachment 434656 [details] [diff] [review]
Patch for checkin

This is looking good on try.
Comment 52 Justin Lebar (not reading bugmail) 2010-03-25 14:01:01 PDT
We may want to put checking this in on hold now that WebKit appears to be moving away from throwing an exception in selection.modify() on invalid parameters [1].

Mats, how do you think we should proceed?

[1] https://bugs.webkit.org/show_bug.cgi?id=33509#c23
Comment 53 Johnny Stenback (:jst, jst@mozilla.com) 2010-03-25 15:11:42 PDT
I'd say let's get this in and track what WebKit does and adjust our implementation as needed once it's time to do so.
Comment 54 Justin Lebar (not reading bugmail) 2010-03-25 15:20:19 PDT
Do we want to throw on a valid but unsupported granularity ('sentence', etc.) but fail silently on a granularity that WebKit doesn't support?  That's unpleasant.

But not throwing at all gives pages no way of noticing that we don't support those granularities.
Comment 55 Mats Palmgren (:mats) 2010-03-25 15:31:24 PDT
I still think we should land as-is.  Throwing an exception for invalid or
unimplemented values makes perfect sense from an API point of view and is
consistent with existing APIs such as HTML5 Selection and DOM-Level-2-Traversal-Range.
Comment 56 Justin Lebar (not reading bugmail) 2010-04-05 16:13:30 PDT
> I still think we should land as-is.

I spoke with jst over IRC, and he agrees.  We can address the details of WebKit compatibility in a separate bug.
Comment 57 Mats Palmgren (:mats) 2010-04-28 14:15:18 PDT
http://hg.mozilla.org/mozilla-central/rev/1ebf050980b3
Comment 58 Henri Sivonen (:hsivonen) 2010-04-29 03:52:18 PDT
The test case for this bug makes the whole mochitest-plain time out when the HTML5 parser is enabled. File as bug 562631.
Comment 59 Eric Shepherd [:sheppy] 2010-06-15 11:49:05 PDT
Documented:

https://developer.mozilla.org/en/DOM/Selection/modify
Comment 60 Mats Palmgren (:mats) 2010-06-15 13:08:36 PDT
Eric, thanks for documenting this.  In the description of "forward"/"backward"
you say "direction based on the locale".  I think it's actually based on the
on the text direction (LTR/RTL) of the focusNode+offset of the selection.
https://developer.mozilla.org/en/DOM/Selection/focusNode

Justin, please correct me if I'm wrong.
Comment 61 Eric Shepherd [:sheppy] 2010-06-15 13:09:33 PDT
I take it that's not the same thing? I was trying to find a short way to say that, because describing the text direction stuff can be so wordy.
Comment 62 Mats Palmgren (:mats) 2010-06-15 13:26:49 PDT
Maybe it's just my programmer background that makes me misinterpret
the word "locale" then (which is a static property of the application).

Could you say "based on the text direction" instead?
Comment 63 Eric Shepherd [:sheppy] 2010-06-15 13:27:35 PDT
Yeah, I updated it a couple minutes ago, should be less weird now. Thanks for pointing it out.
Comment 64 Mats Palmgren (:mats) 2010-06-15 13:30:22 PDT
Great. Thanks.
Comment 65 :Ehsan Akhgari 2011-03-20 12:52:16 PDT
Note that over in bug 642823, I'm changing the word selection algorithm to not depend on the platform (i.e., never eat spaces next to words) to move us in par with the Webkit implementation, and so that the behavior of this API does not depend on the platform.
Comment 66 :Ehsan Akhgari 2011-03-28 16:10:51 PDT
(In reply to comment #65)
> Note that over in bug 642823, I'm changing the word selection algorithm to not
> depend on the platform (i.e., never eat spaces next to words) to move us in par
> with the Webkit implementation, and so that the behavior of this API does not
> depend on the platform.

I've started a WHATWG thread about whether the behavior of selection.modify should be platform-specific: <http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-March/031000.html>

If you have opinions on this, please comment in that thread.  Thanks!

Note You need to log in before you can comment on or make changes to this bug.