Closed Bug 496275 Opened 16 years ago Closed 15 years ago

Expose JS API for modifying page's selection (window.getSelection().modify())

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: mtsui, Assigned: justin.lebar+bug)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 6 obsolete files)

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
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?
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" }
Confirming, and cc:ing Justin who will start investigating this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
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?
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.
Works on Chrome 2.0.172.39 on Windows. Thanks, Mark.
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?
"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.
(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.
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"?
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.
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.
Attached patch Work in progress 0.1.1 (obsolete) — Splinter Review
Work in progress patch. Implements "character", "word", "line", and "lineboundary" granularities. Remaining to be done is RTL support.
Assignee: nobody → justin.lebar+bug
Status: NEW → ASSIGNED
Attached patch Patch for review v1 (obsolete) — Splinter Review
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>’
Attachment #406888 - Attachment is obsolete: true
Attachment #410924 - Flags: review?
Attachment #410924 - Flags: review? → review?(matspal)
Attachment #410924 - Flags: review?(matspal) → review?
Comment on attachment 410924 [details] [diff] [review] Patch for review v1 Hm... Try is exposing some problems. Taking down the review request for now.
Attached patch Patch for review v2 (obsolete) — Splinter Review
Patch for review. This still has the compile warnings from comment 16 that I'd like to get rid of.
Attachment #410924 - Attachment is obsolete: true
Attachment #413790 - Flags: review?(matspal)
Attachment #410924 - Flags: review?
OS: Mac OS X → All
Hardware: x86 → All
> content/base/public/nsISelection.idl > * @status FROZEN Uhm, doesn't that mean we can't change it? (ever)
Summary: Expose getSelectionController to allow programmatic selection movement → Expose JS API for modifying page's selection
Attached file Testcase #2
Attached file Testcase #3
Attachment #420113 - Attachment mime type: text/html → text/html; charset=windows-1255
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.
Attachment #413790 - Flags: review?(matspal) → review-
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.
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.
(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.
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. :)
(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.
Depends on: 512295
(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.
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #413790 - Attachment is obsolete: true
Attachment #426463 - Flags: review?
Attachment #426463 - Flags: review? → review?(matspal)
Component: General → DOM: Mozilla Extensions
Keywords: dev-doc-needed
Product: Firefox → Core
QA Contact: general → general
Summary: Expose JS API for modifying page's selection → Expose JS API for modifying page's selection (window.getSelection().modify())
Version: unspecified → Trunk
Hm...Webkit's arguments are case-insensitive. Presumably we want to match them?
This looks great, Justin! Do you have try-server builds?
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: لا
CCing Johnathan because he may have more ideas along the lines of comment 33.
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.
(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.
(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.
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.
(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?
(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".)
(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.
(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.
Attached patch Patch v4 (obsolete) — Splinter Review
Adding case-insensitivity, a test for the 'تَ' and 'لا' characters. Also updating the documentation on nsISelection::Modify.
Attachment #426463 - Attachment is obsolete: true
Attachment #432058 - Flags: review?
Attachment #426463 - Flags: review?(matspal)
Attachment #432058 - Flags: review? → review?(matspal)
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.
Attachment #432058 - Flags: superreview?(jst)
Attachment #432058 - Flags: review?(matspal)
Attachment #432058 - Flags: review+
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.
Attached patch Patch v5 (obsolete) — Splinter Review
Addressing Mats's review in comment 44.
Attachment #432058 - Attachment is obsolete: true
Attachment #434324 - Flags: superreview?(jst)
Attachment #432058 - Flags: superreview?(jst)
Attachment #434324 - Flags: superreview?(jst) → superreview+
Comment on attachment 434324 [details] [diff] [review] Patch v5 Looks good!
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?
I think it's fine to land as-is, thanks.
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.
This is looking good on try.
Attachment #434324 - Attachment is obsolete: true
Keywords: checkin-needed
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
Keywords: checkin-needed
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.
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.
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.
> 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.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
The test case for this bug makes the whole mochitest-plain time out when the HTML5 parser is enabled. File as bug 562631.
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.
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.
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?
Yeah, I updated it a couple minutes ago, should be less weird now. Thanks for pointing it out.
Great. Thanks.
Depends on: 581051
Depends on: 642823
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.
(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!
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: