Closed Bug 1323681 Opened 9 years ago Closed 8 years ago

It might be better to stop supporting multiple selection in editor

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox53 --- affected

People

(Reporter: masayuki, Unassigned)

Details

Neither Edge nor Google Chrome supports multiple selection, check these testcases: 1. editor: https://jsfiddle.net/d_toybox/9Lrs62po/3/ 2. normal: https://jsfiddle.net/d_toybox/9Lrs62po/2/ For usability, I don't think that we should remove multiple selection feature of non-editable case. However, it might be better to remove it when an editor has focus because supporting multiple selection causes a lot of similar crash bugs like bug 1317704 and makes the editor implementation complicated. So, if almost nobody loves this feature in editor, dropping multiple selection in editor makes our code simpler.
Other browser vendors have said that they agree that supporting multiple ranges would be a good thing, since it is needed to achieve some functionality native apps have.
Oh, really? Do you have some URL of public discussion of that?
Look at the note just above https://w3c.github.io/selection-api/#extensions-to-other-interfaces for references to multiple selection ranges and browser support.
(In reply to Olli Pettay [:smaug] from comment #1) > Other browser vendors have said that they agree that supporting multiple > ranges would be a good thing, since it is needed to achieve some > functionality native apps have. Really? See for example: <https://bugs.chromium.org/p/chromium/issues/detail?id=105651>. Ryosuke, what is your current thinking about this for WebKit?
Flags: needinfo?(rniwa)
As a vendor (Apple), we have quite a bit of interest in improving selection for bidirectional text and new CSS layout modes such as flex box and grid layout which can change the order of context. One way to achieve that would be supporting multi-range selection. As a spec author, there has been some discussion about bringing back multi-range selection, there is no wide support nor consensus about it.
Flags: needinfo?(rniwa)
IIRC, Tab was also positive about multi-range selections to solve visual selection in Grid layout. (I can't find the exact post on www-style though). Fwiw, I will try to implement visual selection for Grid in Gecko (using multiple ranges as needed) when I can find some time. (subgrid is higher prio though)
Yeah, Selection is just *silly* in common Grid cases under the old "single start and end DOM locations" model. Need multi-range selection to make it work.
As a vendor (Apple), I’m not certain the current multi-range selection API Gecko implements is the way to go given all the feedback Ehsan and others at Gecko have given. e.g. Maybe we don’t let JavaScript modify each range separately since that seems to be the primary source of many complexities.
First, thank you for a lot of information, and sorry for the delay to reply due to my machine trouble :-( (In reply to Mats Palmgren (:mats) from comment #6) > IIRC, Tab was also positive about multi-range selections to solve visual > selection in Grid layout. (I can't find the exact post on www-style though). > > Fwiw, I will try to implement visual selection for Grid in Gecko (using > multiple ranges as needed) when I can find some time. (subgrid is higher > prio though) Interesting. Traditionally, Gecko selects any ranges with a DOM range with a set of mouse drag. That causes odd selection range when it's across position:fixed, position:abosolute or float even in CSS2 level layout. I'd be happy if all of such odd case would be gone. According to comments here and replies in dev.platform, we should keep supporting multi-range selection for now. However, I wonder, if we could make the editing behavior simpler, it'd make me happy. For example, only first or last range is modifiable.
(In reply to Ryosuke Niwa from comment #9) > As a vendor (Apple), I’m not certain the current multi-range selection API > Gecko implements is the way to go given all the feedback Ehsan and others at > Gecko have given. e.g. Maybe we don’t let JavaScript modify each range > separately since that seems to be the primary source of many complexities. That part is actually not too bad, the nightmare starts when you think about how to deal with multiple ranges in editing operations (think for example multiple overlapping ranges when trying to make some text bold, and how to undo that, etc.) and it becomes worse when the web page gets to edit the selection ranges in the middle of the editing operations (for example in a mutation event). Also there is quite a bit of additional work that needs to be done for editing to properly support multiple ranges. For example, what happens if you have multiple ranges and start to type?
> for example multiple overlapping ranges FUD. We don't allow overlapping Ranges in Selection. It's also sorted in document order IIRC. > it becomes worse when the web page gets to edit the selection > ranges in the middle of the editing operations (for example in > a mutation event) The editor code can easily prevent that from happening. There is a SelectionBatcher RAII class to prevent notifying Selection listeners until you're done. There is nsAutoScriptBlocker too. > what happens if you have multiple ranges and start to type? I'm not an editing expert but doesn't all typing basically start with RemoveAllRanges, and then insert any character(s) where the caret is? If we don't yet have code to make sort of thing convenient, then we should certainly add it.
(In reply to :Ehsan Akhgari from comment #11) > That part is actually not too bad, the nightmare starts when you think about > how to deal with multiple ranges in editing operations (think for example > multiple overlapping ranges when trying to make some text bold, and how to > undo that, etc.) Selection promotion, aggregation of multiple transactions into a single one. > and it becomes worse when the web page gets to edit the > selection ranges in the middle of the editing operations (for example in a > mutation event). Also there is quite a bit of additional work that needs to > be done for editing to properly support multiple ranges. For example, what > happens if you have multiple ranges and start to type? Nobody has ever said editing is simple and nobody has ever said selection is simple. All people working on editing have always known that "selection promotion" is needed for most operations. Gecko does it pretty well. > For example, what happens if you have multiple ranges and start to type? I don't call that additional work. Once it's decided/standardized, it's pretty simple to implement. And your case is pretty well defined by market practice.
(In reply to Mats Palmgren (:mats) from comment #12) > > for example multiple overlapping ranges > > FUD. We don't allow overlapping Ranges in Selection. > It's also sorted in document order IIRC. I'm not sure we sort in document order. A test I just did shows the contrary. For overlapping ranges, I don't see anything in the code preventing the creation of overlapping ranges programatically. I just wrote a test doing it and it worked fine, and the two overlapping ranges I added to the selection were not modified. We may have code preventing to do it using the mouse or the keyboard but that's a totally different story.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #10) > For example, only first or last range is modifiable. That would completely break table editing. Again, anyone extensively using tables in a content editor or even in Excel uses multiple selection all the time. In my humble opinion: 1. Gecko has always led on that front, doing better than others 2. this all boils down to specifying the response to user behaviours, not to removing features 3. the painful cases you describe like positioned elements, floats, etc can be handled working on item 2 above and declining the resolutions into interoperable code
(In reply to Daniel Glazman (:glazou) from comment #14) > ... and the two overlapping ranges I added to the selection were not > modified. Oh, maybe it's not working as well as I thought it was then. I'm pretty sure we intended to enforce non-overlapping ranges in the Selection - this was implemented in bug 348681, and the code is still there AFAICT: http://searchfox.org/mozilla-central/rev/cc2a84852bd4e6f6d8d4d5b17b8382bb5d005749/layout/generic/nsSelection.cpp#4013 I think that's a good invariant to strive for, if it makes writing editor code easier.
(In reply to Mats Palmgren (:mats) from comment #16) > I think that's a good invariant to strive for, if it makes writing > editor code easier. I tend to agree but what's supposed to happen if the user creates such overlapping ranges with mouse and keyboard?
(In reply to Daniel Glazman (:glazou) from comment #17) > I tend to agree but what's supposed to happen if the user creates such > overlapping ranges with mouse and keyboard? I did some quick testing and it seems to work fine for anything that goes through Selection::AddItem (C++), like Selection.addRange(). I think adding a new range using ctrl+drag also goes through that code. I've filed bug 1325059 to fix the remaining cases (i.e. when extending an existing range that is inside a Selection so that it overlaps some other range in that Selection).
(In reply to Mats Palmgren (:mats) from comment #12) > > for example multiple overlapping ranges > > FUD. We don't allow overlapping Ranges in Selection. > It's also sorted in document order IIRC. As the later comments talked about this, we surely allow this for programmatic modification of the ranges, but you're right about the selections made by the user for the most part, although IIRC we may have some bugs there as well, for example when the user uses the mouse to create a selection inside a cell that has been selected by Ctrl/Cmd+Click... > > it becomes worse when the web page gets to edit the selection > > ranges in the middle of the editing operations (for example in > > a mutation event) > > The editor code can easily prevent that from happening. There is > a SelectionBatcher RAII class to prevent notifying Selection listeners > until you're done. There is nsAutoScriptBlocker too. Yes, I know. :-) But I think the "easily" part can be debated. Gecko these days is much more robust against these kinds of bugs because I have fixed all of the known ones back in the day, but it by no means suggests that we don't have such bugs any more. The issue here is that it is extremely hard to protect against these issues systematically so in practice the fixes are mostly spot fixes here and there for the bugs that we know about. > > what happens if you have multiple ranges and start to type? > > I'm not an editing expert but doesn't all typing basically start with > RemoveAllRanges, and then insert any character(s) where the caret is? > If we don't yet have code to make sort of thing convenient, then we should > certainly add it. My point was to note that there are at least two possible behaviors you may have here: * Clear the selection and collapse one of the ranges and start typing there. * Support typing into multiple locations at the same time. Each will require different things to think about and specify, and so on. What Gecko currently does for example is almost definitely not ideal (we hide the caret so the from the user's point of view the range that is where typing happens is a random one, and we have bugs around the interaction, for example with the spell checker getting seriously confused when you undo typing in this situation) (In reply to Daniel Glazman (:glazou) from comment #13) > (In reply to :Ehsan Akhgari from comment #11) > > > That part is actually not too bad, the nightmare starts when you think about > > how to deal with multiple ranges in editing operations (think for example > > multiple overlapping ranges when trying to make some text bold, and how to > > undo that, etc.) > > Selection promotion, aggregation of multiple transactions into a single > one. I think you're heavily underestimating the complexity of getting this right. We do both of the above and our implementation is still full of edge case bugs. > > and it becomes worse when the web page gets to edit the > > selection ranges in the middle of the editing operations (for example in a > > mutation event). Also there is quite a bit of additional work that needs to > > be done for editing to properly support multiple ranges. For example, what > > happens if you have multiple ranges and start to type? > > Nobody has ever said editing is simple and nobody has ever said selection > is simple. All people working on editing have always known that "selection > promotion" is needed for most operations. Yes, I'm very well aware, as you probably know. Being condescending is hardly helpful. > Gecko does it pretty well. > > > For example, what happens if you have multiple ranges and start to type? > > I don't call that additional work. Once it's decided/standardized, it's > pretty > simple to implement. The hard part here isn't the code required to implement this one piece, it is specifying how the browser is to behave in each case and getting the interactions with all of the other features right. Given that to this date browsers don't yet interop very well on editing operation and we only have an unfinished spec for the current editing features that all browsers support, there's no evidence to suggest anything about multi-range selections with regards to editing is going to be simple.
(In reply to :Ehsan Akhgari from comment #19) > Being condescending is hardly helpful. I've never been "condescending" with you - I highly respect you - so why would I start today?!? My reply was not a private message and you're then not its only reader; I was giving context to people not used to editor's code. Sorry if you took it that way, absolutely not my intention. > I think you're heavily underestimating the complexity of getting this right Oh, trust me, I don't. > browsers don't yet interop very well on editing operation This is the least we can say... Most of the complex operations of editing are handled "well" only by Gecko. Others remained at a "good enough" stage and never touch some complex stuff we do. So there cannot be interoperability on things they never considered for implementations, or things they voluntarily left in a semi-correct state.
(In reply to :Ehsan Akhgari from comment #19) > The issue here is that it is > extremely hard to protect against these issues systematically I disagree about that. Preventing notifications / script from running, holding strong refs on objects you need etc. is something that (most) Gecko code have to do and I don't see why that's harder in editor code in particular. Other than that I mostly agree with your problem analysis. But the solution here is to work on the spec(s) so that they take multi-range Selection into account, not remove this feature since it's clearly needed in multiple important use cases. Perhaps the first step is to impose restrictions on the ranges so they are as simple as possible to work with. For example, make all ranges non-overlapping, and in document order. I think this is something that the Selection code could enforce. We could also enforce that the caret is always in the first or last range, but perhaps that's already implicitly true? Also, to be clear, I'm not trying to trivialize the editor code. I know it's fairly complex. But I don't think that complexity stems from multi-range Selections. As you agreed, most editing operations "Clear the selection and collapse one of the ranges and start typing there" which is equal in complexity to one range. I'm not aware of any "Support typing into multiple locations at the same time" operations that are supported in Gecko, can you give examples? (I can imagine this for a Find-and-Replace feature, but do we have that?)
(In reply to Daniel Glazman (:glazou) from comment #20) > (In reply to :Ehsan Akhgari from comment #19) > > > Being condescending is hardly helpful. > > I've never been "condescending" with you - I highly respect you - > so why would I start today?!? My reply was not a private message > and you're then not its only reader; I was giving context to people > not used to editor's code. Sorry if you took it that way, absolutely > not my intention. I'm glad to hear that, no hard feelings. :-) (In reply to Mats Palmgren (:mats) from comment #21) > (In reply to :Ehsan Akhgari from comment #19) > > The issue here is that it is > > extremely hard to protect against these issues systematically > > I disagree about that. Preventing notifications / script from > running, holding strong refs on objects you need etc. is something > that (most) Gecko code have to do and I don't see why that's harder > in editor code in particular. One reason is that in the majority of the cases, it is not clear what the code _should_ be doing, since there's no useful spec, and the behavior may be tricky to define, and browsers often demonstrate a variety of choices in terms of how the corner cases are handled. We've sadly had cases where a "fix" in Gecko broke some library because it was trying to work around an existing behavior. Another reason (although this one is a lot better these days) is that parts of the editor code base are under tested, so you can't easily change stuff and be sure that you have enough protection against Web facing regressions. Another reason is just the sheer size of the project. The editor code is notorious for not adhering to many of our standard coding practices, including basic things such as how refcounted objects are treated, what DOM node APIs are OK to be called from C++, etc. We've done a ton of work on this front but we've barely scratched the surface so far. Last but not least, when you come across some old code in the editor, and wonder why it is written the way it is, often times you go to an old bug without any details about the implementation, and there would be no code review to speak of, and definitely nobody still around who understands the choices made in the old days. In short, loads of technical debt. > Other than that I mostly agree with your problem analysis. > But the solution here is to work on the spec(s) so that they take > multi-range Selection into account, not remove this feature since > it's clearly needed in multiple important use cases. In an ideal world where we have enough human power to actually do this I would agree with you. As things stand now, we have 1 (or less than 1) person working on the code, and nobody working on the spec from Mozilla (we're not even participating in the Editing TF recently spun up yet <https://www.w3.org/wiki/Editing_Taskforce>). I am not aware of anybody else who is working on any specs for multi-range selections from other browser vendors either. Still I do want to see feature survive into the future, but I don't think that goal is really attainable unless if we're OK with the status quo of Gecko shipping a half broken implementation of this without any realistic path of any other browser shipping a compatible implementation ever. (That all being said, *I* do agree that we need a ton of more work in the area, but I haven't been able to convince anybody else to invest more here.) > Perhaps the first step is to impose restrictions on the ranges > so they are as simple as possible to work with. For example, > make all ranges non-overlapping, and in document order. I think guaranteeing all ranges to be non-overlapping is extremely important and valuable. I'm less convinced about the ordering of ranges, because I'm not 100% sure what the right thing to do is there. From a UX stand point, things such as attaching the caret to the last range in document order is pretty questionable. (Which reminds me, we do need a UX person to look at this problem space as well.) > I think > this is something that the Selection code could enforce. > We could also enforce that the caret is always in the first or > last range, but perhaps that's already implicitly true? That's already true (IIRC we attach it to the last range) but see above. > Also, to be clear, I'm not trying to trivialize the editor code. > I know it's fairly complex. But I don't think that complexity > stems from multi-range Selections. No, I didn't meant to suggest so. But multi-range selections are definitely not helping, and they have been the source of *many* bugs that I have looked at at least, and to this day I have 0 confidence in our implementation of them. :-) I can rant for hours about other problems in the editor code base; I'm merely just focusing on this one here. > As you agreed, most editing > operations "Clear the selection and collapse one of the ranges > and start typing there" which is equal in complexity to one range. That's actually not true, many editing operations are selection preserving, and those are typically the cases where we have bugs with regards to multi-range selections. The "clear and collapse" case is the easy one relatively speaking. > I'm not aware of any "Support typing into multiple locations at > the same time" operations that are supported in Gecko, can you > give examples? (I can imagine this for a Find-and-Replace > feature, but do we have that?) We have no support for that -- I raised that as an example of something that we have just ignored but it seems like a nice feature to have. My higher level point there was that just because something is currently ignored by Gecko doesn't mean that it's ignored for a good reason.
> ... In short, loads of technical debt. Right, I'm aware, which is why I think we should seriously consider "refactoring" all editor code into JS.
Spec issue to consider adding this feature to the spec: https://github.com/w3c/selection-api/issues/41
Thank you for the lots of discussion and suggesting to Selection API WG, I think that we have no choice to drop the support. On the other hand, current behavior is just "as is". I mean that editor code doesn't behave consistently. For example, if user selects each word of "abc def ghi" with any order and type something, Gecko inserts text after two whitespaces (i.e., the last (in DOM tree order) range is assumed as caret position). On the other hand, if user selects each word of "abc def ghi" with any order and paste something, Gecko inserts text before two whitespaces (i.e., the first (in DOM tree order) range is assumed as caret position). So, we should keep thinking where is the best to insert, how make editor behave consistently.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
I don't *think* the problem here is complexity per se. (At least, I haven't seen examples that seem inherently a lot harder to deal with in the multiple-selection case, assuming that they're non-overlapping.) I think the problem is that the vast majority of cases are one-range, so all code written for selection is written and tested basically for the one-range case, with multiple ranges taken into account as an untested afterthought if at all. Years ago I reviewed a bunch of selection use on websites, and IIRC it generally just used .getRangeAt(0) and ignored the possibility of additional ranges. This is an API that nobody is likely to use correctly. Including us in our own code. A good API would be one where non-contiguous selections are handled naturally by the same code that handles contiguous selections. I think this is feasible, because in real life I don't think selection-handling code deals with ranges -- it deals with the nodes (and parts of nodes) that are selected. If we design a new API that exposes a list of (partially-)selected nodes instead of a range, there would be no special work needed to accommodate non-contiguous selections. Ehsan, do you have cases that break for non-contiguous selections that would still break if they dealt with a list of possibly-non-contiguous nodes?
Flags: needinfo?(ehsan)
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #26) > Ehsan, do you have cases that break for non-contiguous selections that would > still break if they dealt with a list of possibly-non-contiguous nodes? If you also mean non-overlapping, then no, those are the nice and clean cases. :-) And I agree that is possible to solve. But I don't understand what the practical value of such a solution would be, because the user can modify these selections, so we can't really restrict them in any meaningful way, unless I'm missing something?
Flags: needinfo?(ehsan)
Why would you want to restrict them? Let a selection be an arbitrary set of nodes (and/or CharacterData characters) that are descended from the current document, and expose that set to authors as a live list of some kind. Exposing them as a live list instead of a set of non-overlapping ranges will make non-contiguous selections no longer be a special case. If something tries to select multiple overlapping ranges, just merge every pair of overlapping ranges into a single range. This way of presenting things removes the distinction between <b>[foo]</b> and <b>{foo}</b>, incidentally, but I'm not sure if that's a bad thing. (IIRC, Blink/WebKit don't make the distinction to begin with.) I guess you also can't represent <p>[foo</p>]bar, though, which is no good for some applications -- e.g., delete. So some things will still need to know about range endpoints and not just the set of selected nodes/characters. That considerably reduces the value of my suggestion. It could be salvaged by complicating it and adding info to the live list about selection of start/end of nodes.
Flags: needinfo?(ehsan)
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #28) > Why would you want to restrict them? Let a selection be an arbitrary set of > nodes (and/or CharacterData characters) that are descended from the current > document, and expose that set to authors as a live list of some kind. > Exposing them as a live list instead of a set of non-overlapping ranges will > make non-contiguous selections no longer be a special case. If something > tries to select multiple overlapping ranges, just merge every pair of > overlapping ranges into a single range. I don't really have much to add besides what I have already said here. It looks like you disagree which is fine. I doubt that if I repeat what I have said in comment 19, 22 etc is going to change your mind. :-) In general I think this topic has been talked about in high level quite a bit. What needs to happen is for some group of people who think this is a worthwhile feature to support to sit down and correctly specify the interaction of the feature with the rest of the Web platform and take that specification to the vendors for implementation feedback. It is hard for someone who takes my position to give you all of the concrete answers you are asking for without doing all of this work. This isn't something that I personally have any time to do in the near future unfortunately. :-( > This way of presenting things removes the distinction between <b>[foo]</b> > and <b>{foo}</b>, incidentally, but I'm not sure if that's a bad thing. > (IIRC, Blink/WebKit don't make the distinction to begin with.) I guess you > also can't represent <p>[foo</p>]bar, though, which is no good for some > applications -- e.g., delete. So some things will still need to know about > range endpoints and not just the set of selected nodes/characters. That > considerably reduces the value of my suggestion. It could be salvaged by > complicating it and adding info to the live list about selection of > start/end of nodes. This is the right line of thinking. Sorry if I can't be of much help right now, my plate is full of other things that require my attention.
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.