520221 - Investigate how to handle changes to range objects which are used in selection

Status: NEW

(Core :: DOM: Selection, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

There are at least two options:
(1) Set a flag to those range objects which are used in a selection and make those
    range objects readonly for scripts.
(2) Add a pointer to the selection to those range objects which are used in a 
    selection and update selection whenever script changes the range.

I think (1) may break some websites, so I'd prefer (2).
It could be implemented so that at least when nsRange::DoSetRange is called,
range objects in the selection get re-ordered.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> I think (1) may break some websites

The ability to modify ranges in a selection is mozilla-specific, right?  How much is it used?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Don't know how much. But in any case, (2) might be even simpler to implement.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think a fair number of sites modify selection, but do so using different UA-specific APIs in different UAs. Often time for modifying selection in editable areas to for example select the whole area of a text input when it gains focus.

Of course, what would be really nice is a real API to modify selection, then I'd imagine we can phase out whatever hacks work now.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Making ranges (which are in selection) to update selection properly would be,
IMHO, pretty nice and simple API. And web developers are already pretty
familiar with ranges.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a simple testcase

Trunk doesn't repaint the changed selection range automatically.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a patch

Something like this should work. nsTypedSelection::CopyRangeToAnchorFocus a bit ugly, but it is ugly even now :/
Needs lots of automated tests.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What's supposed to happen if a script adds two ranges to a selection, then modifies the ranges so they're the same? We'll reduce the selection to one range, I assume, so one of those ranges will just be removed from the selection as a side effect?

That's a bit weird but I guess it's OK.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)
> What's supposed to happen if a script adds two ranges to a selection, then
> modifies the ranges so they're the same? We'll reduce the selection to one
> range, I assume, so one of those ranges will just be removed from the selection
> as a side effect?
Right, the patch doesn't change the how nsTypedSelection::AddItem handles 
overlaps. Adding an overlapping range may also just modify existing ranges.

There are two cases I especially want to fix
(1) Prevent problems like bug 520070.
(2) invalidation of modified ranges

Both cases are handled by the change to nsRange::DoSetRange:
(1) because adding a detached range to selection isn't allowed, (2) because
adding a valid range to selection calls nsTypedSelection::selectFrames, which
then marks frames selected.

And one not so important case is using the same range in several selections.
Better to just prevent that by removing the range from the old selection before
adding to new one. IMO.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: + some tests

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

jonas, ping.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Will this work even when the mutations happen in a node that is fully inside the range? Such as if a child is added to <bar> in the following tree:

<foo>|<bar>text here<span/></bar>|</foo>

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a testcase for dynamic changes within range

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

This handles also the 2nd testcase, but this is not the way it should be handled.
Frame construction or something should take care of marking right frames to be
selected, I think.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 417990 [details] [diff] [review]
v2

>+         nsContentUtils::ContentIsDescendantOf(aChangedNode,
>+                                               aRange->GetStartParent()) ||
>+         nsContentUtils::ContentIsDescendantOf(aChangedNode,
>+                                               aRange->GetEndParent()))) {


This doesn't seem correct either. Consider:

<div><foo>text|here</foo><bar>more text</bar><foo>fi|nal</foo></div>

And the modification to the <bar> element. This element isn't a decedent of either the start or end parent.