Closed Bug 1444847 Opened 6 years ago Closed 4 years ago

Implement AbstractRange and StaticRange

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: annevk, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(10 files)

18.13 KB, patch
Details | Diff | Splinter Review
23.03 KB, patch
Details | Diff | Splinter Review
9.01 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
See https://github.com/whatwg/dom/pull/589 and https://github.com/w3c/web-platform-tests/pull/9967.

I suppose we might want to wait with this until we also implement https://w3c.github.io/input-events/index.html (could not find a tracking bug), unless bug 1438635 becomes high enough of a priority somehow.
Priority: -- → P3
- Edge 18 will support both (Insider build supports both)
- Safari 10.1 supports StaticRange only
- Chrome 60 supports StaticRange only with old IDL

What's the status of this bug? Perhaps, I can fix bug 998941 soon. Then, I'll work on implementing beforeinput behind a flag. (Perhaps, I need to rewrite a lot especially around keypress event dispatcher, so, still not so urgent though.)

Flags: needinfo?(m_kato)
Flags: needinfo?(m_kato)
Assignee: nobody → m_kato
Assignee: m_kato → nobody
Comment on attachment 9048090 [details] [diff] [review]
Part 2. Implement StaticRange

Actually, there no value setter in StaticRange spec. So we will need C++ method or chrome only method to use StaticRange. Since I add chrome only method for test, what method that do you want for input type?

getTargetRanges will be necessary to use undo/redo implementation by JavaScript's editor.  But it may be better way to implement undo/redo without getTargetRanges.
Attachment #9048090 - Flags: feedback?(masayuki)
Comment on attachment 9048090 [details] [diff] [review]
Part 2. Implement StaticRange

In most cases, we need to create StaticRange instance with an nsRange instance. So, nsRange::CloneAsStaticRange() or something is useful. However, I have no idea for some specific cases like "deleteWordBackward" inputType. Probably, necessary API should be added when we need actually to save the binary size.
Attachment #9048090 - Flags: feedback?(masayuki)
Component: DOM → DOM: Core & HTML
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
OS: Unspecified → All
Priority: P3 → P2
Hardware: Unspecified → All
Version: unspecified → Trunk

This patch is based on the patch created by Makoto Kato-san.

Range and StaticRange have common base interface, AbstractRange.
https://dom.spec.whatwg.org/#abstractrange

This interface has simply returns startContainer, endContainer,
startOffset, endOffset and collapsed.

Different from the original patch's approach, this patch moves related
members in nsRange to AbstractRange since this approach avoids
virtual call cost. Additionally, this patch makes them not throw as
declared by the spec. As far as I know, the destruction cost of
ErrorResult may appear in profile so that we should avoid creating
the instance if we can avoid it.

Unfortunately, the instance size of nsRange becomes larger with this
patch. The size is changed from 176 to 184. I.e., now, nsRange
requires bigger chunk.

For avoiding confusion between API of nsRange and StaticRange, I'd like to
rename nsRange::CreateRange() to nsRange::MaybeCreate() because
StaticRange::CreateStaticRange() is too long name and
StaticRange::CreateRange() sounds odd. And sometimes, it won't return new
instance. This patch renames it and changes related methods to template
methods for avoiding runtime cost of temporary RawRangeBoundary
instance creation.

Some nsRange static methods are useful in StaticRange and some of them
are used in a lot of places but not related to nsRange directly. This
patch moves them into new static method only class, mozilla::RangeUtils.

This patch is based on Makoto Kato-san's patch.

This patch implements mozilla::dom::StaticRange class and creating some
static factory methods.

Then, makes AbstractRange has a utility method of SetStartAndEnd()
method of nsRange and StaticRange for sharing same logic in one place.
However, there are some additional work is required only in nsRange, e.g.,
nsRange needs to start observing mutation of the range, but StaticRange
does not it. Therefore, it's implemented as a template method which takes
nsRange* or StaticRange* as a parameter. Then, each DoSetRange()
method of them can do different things without virtual calls.

Note that StaticRange does not have any properties, methods nor constructor.
Therefore, we need additional API to test it.

This patch makes HTMLEditRules::ExpandSelectionForDeletion() use StaticRange
instead of nsRange for comparing a point and a range (i.e., the DOM tree
won't be changed during it's alive). Unfortunately, we still have allocation
cost, but we can save the cost of registering and unregistering mutation
observer and computing common ancestor of the range.

This patch makes HTMLEditRules::PinSelectionToNewBlock() use StaticRange
instead of nsRange for comparing a point and a range (i.e., the DOM tree
won't be changed during it's alive). Unfortunately, we still have allocation
cost, but we can save the cost of registering and unregistering mutation
observer and computing common ancestor of the range.

EditorSpellCheck clones nsRange instance only for temporary use during
initialization even though the DOM tree won't be changed during it. In this
case, using StaticRange is better since it does not need to observe the
DOM tree mutation. I.e., we can save the cost of registering and
unregistering the mutation observer.

I think that we should land part.1 ~ part.5 first. Then, part.6 and part.7 should be landed separately for making it easier to look for regression-window.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/feb8bd2e7973
part 1: Create `mozilla::dom::AbstractRange` r=smaug
https://hg.mozilla.org/integration/autoland/rev/610d62589b4e
part 2: Sort out basic API of `nsRange` for consistency with coming `StaticRange` r=smaug
https://hg.mozilla.org/integration/autoland/rev/bcfd386d2585
part 3: Create `RangeUtils` to place public static methods of `nsRange` r=smaug
https://hg.mozilla.org/integration/autoland/rev/762824a037ce
part 4: Implement `mozilla::dom::StaticRange` and static factory methods r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f81be4a7864d
part 5: Make `HTMLEditRules::ExpandSelectionForDeletion()` use `StaticRange` instead of `nsRange` for temporary use r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/446927a94617
part 6: Make `HTMLEditRules::PinSelectionToNewBlock()` use `StaticRange` instead of `nsRange` for temporary use r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c8756efe2b0d
part 7: Make `EditorSpellCheck` use `StaticRange` instead of `nsRange` to initialize itself r=smaug
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1565178
No longer regressions: 1565178
You need to log in before you can comment on or make changes to this bug.