`Selection::Extend()` is too slow because:
- it may create some `nsRange` instances.
- it users `nsContentUtils::ComparePoints()` multiple times.
Therefore, we can improve the performance if we can stop using it in some
places. First, this patch creates `Selection::SetStartAndEnd()` and
`Selection::SetStartAndEndInLimiter()` for internal use. They remove
current ranges, reuse `nsRange` instance as far as possible and add new
range which is set by their arguments. Then, this patch makes
`Selection::SelectAllChildren()` stop using `Selection::Extend()`. At this
time, this fixes a web-compat issue. `Selection::Expand()` cannot cross the
selection limiter boundary when there is a limiter (e.g., when an editing host
has focus). But we can now fix this with using the new internal API.
Note that methods in editor shouldn't move selection to outside of active
editing host. Therefore, this patch adds `Selection::SetStartAndEndInLimiter()`
and `Selection::SetBaseAndExtentInLimiter()` for them.
Differential Revision: https://phabricator.services.mozilla.com/D23459
--HG--
extra : moz-landing-system : lando
The added crashtest still crashes on Android verify runs (TV) for
unknown reasons, so skip it.
Differential Revision: https://phabricator.services.mozilla.com/D16395
--HG--
extra : moz-landing-system : lando
This patch makes ContentIteratorBase, PostContentIterator, PreContentIterator
and ContentSubtreeIterator classes non-refcountable because most users can
create their instances in stack and such users may be in a hot path. So,
we can save a lot of cost of instantiation.
Unfortunately, only ScriptableContentIterator creates one of the concrete
classes and needs to destroy it properly. Therefore, its
EnsureContentIterator(), destructor, traverse and unlink code becomes messy.
However, ScriptableContentIterator was designed for automated tests and we
need to maintain it not so many times. Therefore, improvement of other
users must be worthwhiler than this demerit.
Differential Revision: https://phabricator.services.mozilla.com/D15928
--HG--
extra : moz-landing-system : lando
Now, all users of PostContentIterator can access it directly. This patch
makes them use the concrete class directly.
Differential Revision: https://phabricator.services.mozilla.com/D15923
--HG--
extra : moz-landing-system : lando
This patch makes ContentIteratorBase, PostContentIterator, PreContentIterator
and ContentSubtreeIterator classes non-refcountable because most users can
create their instances in stack and such users may be in a hot path. So,
we can save a lot of cost of instantiation.
Unfortunately, only ScriptableContentIterator creates one of the concrete
classes and needs to destroy it properly. Therefore, its
EnsureContentIterator(), destructor, traverse and unlink code becomes messy.
However, ScriptableContentIterator was designed for automated tests and we
need to maintain it not so many times. Therefore, improvement of other
users must be worthwhiler than this demerit.
Differential Revision: https://phabricator.services.mozilla.com/D15928
--HG--
extra : moz-landing-system : lando
Now, all users of PostContentIterator can access it directly. This patch
makes them use the concrete class directly.
Differential Revision: https://phabricator.services.mozilla.com/D15923
--HG--
extra : moz-landing-system : lando
Summary: Really sorry for the size of the patch. It's mostly automatic
s/nsIDocument/Document/ but I had to fix up in a bunch of places manually to
add the right namespacing and such.
Overall it's not a very interesting patch I think.
nsDocument.cpp turns into Document.cpp, nsIDocument.h into Document.h and
nsIDocumentInlines.h into DocumentInlines.h.
I also changed a bunch of nsCOMPtr usage to RefPtr, but not all of it.
While fixing up some of the bits I also removed some unneeded OwnerDoc() null
checks and such, but I didn't do anything riskier than that.
This is a best effort attempt at ensuring that the adverse impact of
reformatting the entire tree over the comments would be minimal. I've used a
combination of strategies including disabling of formatting, some manual
formatting and some changes to formatting to work around some clang-format
limitations.
Differential Revision: https://phabricator.services.mozilla.com/D13046
--HG--
extra : moz-landing-system : lando
There's a few subtle behavior changes here, which I'll try to break down in the
commit message.
The biggest one is the EditableDescendantCount stuff going away. This
was added in bug 1181130, to prevent clicking on the non-editable div from
selecting the editable div inside. This is problematic for multiple reasons:
* First, I don't think non-editable regions of an editable element should
be user-select: all.
* Second, it just doesn't work in Shadow DOM (the editable descendant count is
not kept up-to-date when not in the uncomposed doc), so nested
contenteditables behave differently inside vs. outside a Shadow Tree.
* Third, I think it's user hostile to just entirely disable selection if you
have a contenteditable descendant as a child of a user-select: all thing.
WebKit behaves like this patch in the following test-case (though not Blink):
https://crisal.io/tmp/user-select-all-contenteditable-descendant.html
Edge doesn't seem to support user-select: all at all (no pun intended).
But we don't allow to select anything at all which looks wrong.
* Fourth, it's not tested at all (which explains how we broke it in Shadow DOM
and not even notice...).
In any case I've verified that this doesn't regress the editor from that bug. If
this regresses anything we can fix it as outlined in the first bullet point
above, which should also make us more compatible with other UAs in that
test-case.
The other change is `all` not overriding everything else. So, something like:
<div style="-webkit-user-select: all">All <div style="-webkit-user-select: none">None</div></div>
Totally ignores the -webkit-user-select: none declaration in Firefox before this
change. This doesn't match any other UA nor the spec, and this patch aligns us
with WebKit / Blink.
This in turn makes us not need -moz-text anymore, whose only purpose was to
avoid this.
This also fixes a variety of bugs uncovered by the previous changes, like the
SetIgnoreUserModify(false) call in editor being completely useless, since
presShell->SetCaretEnabled ended in nsCaret::SetVisible, which overrode it.
This in turn uncovered even more bugs, from bugs in the caret painting code,
like not checking -moz-user-modify on the right frame if you're the last frame
of a line, to even funnier bits where before this patch you show the caret but
can't write at all...
In any case, the new setup I came up with is that when you're editing (the
selection is focused on an editable node) moving the caret forces it to end up
in an editable node, thus jumping over non-editable ones.
This has the nice effect of not completely disabling selection of
-moz-user-select: all elements that have editable descendants (which was a very
ad-hoc hack for bug 1181130, and somewhat broken per the above), and also
not needing the -moz-user-select: all for non-editable bits in contenteditable.css
at all.
This also fixes issues with br-skipping like not being able to insert content in
the following test-case:
<div contenteditable="true"><span contenteditable="false">xyz </span><br>editable</div>
If you start moving to the left from the second line, for example.
I think this yields way better behavior in all the relevant test-cases from bug
1181130 / bug 1109968 / bug 1132768, shouldn't cause any regression, and the
complexity is significantly reduced in some places.
There's still some other broken bits that this patch doesn't fix, but I'll file
follow-ups for those.
Differential Revision: https://phabricator.services.mozilla.com/D12687
--HG--
extra : moz-landing-system : lando
There's a few subtle behavior changes here, which I'll try to break down in the
commit message.
The biggest one is the EditableDescendantCount stuff going away. This
was added in bug 1181130, to prevent clicking on the non-editable div from
selecting the editable div inside. This is problematic for multiple reasons:
* First, I don't think non-editable regions of an editable element should
be user-select: all.
* Second, it just doesn't work in Shadow DOM (the editable descendant count is
not kept up-to-date when not in the uncomposed doc), so nested
contenteditables behave differently inside vs. outside a Shadow Tree.
* Third, I think it's user hostile to just entirely disable selection if you
have a contenteditable descendant as a child of a user-select: all thing.
WebKit behaves like this patch in the following test-case (though not Blink):
https://crisal.io/tmp/user-select-all-contenteditable-descendant.html
Edge doesn't seem to support user-select: all at all (no pun intended).
But we don't allow to select anything at all which looks wrong.
* Fourth, it's not tested at all (which explains how we broke it in Shadow DOM
and not even notice...).
In any case I've verified that this doesn't regress the editor from that bug. If
this regresses anything we can fix it as outlined in the first bullet point
above, which should also make us more compatible with other UAs in that
test-case.
The other change is `all` not overriding everything else. So, something like:
<div style="-webkit-user-select: all">All <div style="-webkit-user-select: none">None</div></div>
Totally ignores the -webkit-user-select: none declaration in Firefox before this
change. This doesn't match any other UA nor the spec, and this patch aligns us
with WebKit / Blink.
This in turn makes us not need -moz-text anymore, whose only purpose was to
avoid this.
This also fixes a variety of bugs uncovered by the previous changes, like the
SetIgnoreUserModify(false) call in editor being completely useless, since
presShell->SetCaretEnabled ended in nsCaret::SetVisible, which overrode it.
This in turn uncovered even more bugs, from bugs in the caret painting code,
like not checking -moz-user-modify on the right frame if you're the last frame
of a line, to even funnier bits where before this patch you show the caret but
can't write at all...
In any case, the new setup I came up with is that when you're editing (the
selection is focused on an editable node) moving the caret forces it to end up
in an editable node, thus jumping over non-editable ones.
This has the nice effect of not completely disabling selection of
-moz-user-select: all elements that have editable descendants (which was a very
ad-hoc hack for bug 1181130, and somewhat broken per the above), and also
not needing the -moz-user-select: all for non-editable bits in contenteditable.css
at all.
This also fixes issues with br-skipping like not being able to insert content in
the following test-case:
<div contenteditable="true"><span contenteditable="false">xyz </span><br>editable</div>
If you start moving to the left from the second line, for example.
I think this yields way better behavior in all the relevant test-cases from bug
1181130 / bug 1109968 / bug 1132768, shouldn't cause any regression, and the
complexity is significantly reduced in some places.
There's still some other broken bits that this patch doesn't fix, but I'll file
follow-ups for those.
Differential Revision: https://phabricator.services.mozilla.com/D12687
--HG--
extra : moz-landing-system : lando
SelectionChangeListener is too generic name but it just dispatches
selectionchange event when it's necessary. So, it should be renamed to
SelectionChangeEventDispatcher. Additionally, it's in mozilla::dom namespace
but it does not represent any DOM object. So, it should be in mozilla namespace
instead.
Differential Revision: https://phabricator.services.mozilla.com/D4913
--HG--
rename : dom/base/SelectionChangeListener.cpp => dom/base/SelectionChangeEventDispatcher.cpp
rename : dom/base/SelectionChangeListener.h => dom/base/SelectionChangeEventDispatcher.h
extra : moz-landing-system : lando
SelectionChangeListener is an nsISelectionListener class. This is added only
to Selection for "normal" and added by nsFrameSelection::Init() after
AccessibleCaretEventHub. So, we can make Selection directly treat
SelectionChangeListener.
Differential Revision: https://phabricator.services.mozilla.com/D4757
--HG--
extra : moz-landing-system : lando
AccessibleCaretEventHub is an nsISelectionListener of Selection whose type is
"normal". This is added only when nsFrameSelection::Init() is called and
accessible caret is enabled. Additionally, nsFrameSelection::Init() is
always called immediately after creating nsFrameSelection.
Therefore, when AccessibleCaretEventHub is installed to Selection, this is
always second selection listener and won't be installed multiple times. So,
Selection can store pointer of AccessibleCaretEventHub directly only when
it's enabled and the Selection needs to notify it of selection change.
This patch makes Selection stores AccessibleCaretEventHub with RefPtr, then,
makes Selection::NotifySelectionListeners() call its OnSelectionChange()
immediately after AutoCopyListener.
Unfortunately, this patch includes making of MOZ_CAN_RUN_SCRIPT_BOUNDARY and
MOZ_CAN_RUN_SCRIPT a lot since some methods of AccessibleCaretEventHub are
marked as MOZ_CAN_RUN_SCRIPT and including AccessibleCaretEventHub.h into
Selection.h causes compile the compile errors.
Differential Revision: https://phabricator.services.mozilla.com/D4733
--HG--
extra : moz-landing-system : lando
nsAutoCopyListener is a singleton class but refcountable and a selection
listener. nsFrameSelection adds it to only normal Selection when it's on
macOS or it's enabled by the pref. Additionally, it's always first selection
listener since it's added immediately after Selection instance is created.
So, we can make it a static class, and normal Selection instance should have
a bool to decide whether it should notify nsAutoCopyListener of its changes.
Then, we can save the cost of grabbing it with local RefPtr and the virtual
call.
Additionally, this patch renames nsAutoCopyListener to mozilla::AutoCopyListener
and optimizes constructor of nsFrameSelection (using bool var cache to retrieve
the pref, avoid retrieving the pref on macOS).
Differential Revision: https://phabricator.services.mozilla.com/D4504
--HG--
rename : layout/generic/nsAutoCopyListener.h => layout/generic/AutoCopyListener.h
extra : moz-landing-system : lando
Selection caches an nsRange instance for saving re-allocation cost and AddMutationObserver() and RemoveMutationObserver()'s cost when its RemoveAllRangesTemporarily() is called.
Then, the instance is detached from the Selection but still referring editing point. E.g., the only text node in TextEditor when its value is set. Therefore, it'll receive character data change notification and need to check whether the point is still valid with new text. However, the range will be always set new position later, i.e., immediately before going back to a part of Selection. Therefore, even if the point becomes invalid, nobody must not have any problems.
This patch makes Selection make the cached range not positioned, and makes nsRange ignore any mutations when it's not positioned.
Differential Revision: https://phabricator.services.mozilla.com/D2587
--HG--
extra : moz-landing-system : lando
None of the C++ callers of RemoveSelectionListener care about whether the
listener was already-added, and the only JS caller is in a test and knows the
listener was added. So the behavior change to no-op instead of throwing when
trying to remove a nonexistent listener is OK. Furthermore, the removal is
null-safe, so there's no point to explicitly failing if null is passed (which
it never is).
Since content can't directly add selection listeners, we can just use an
infallible append instead of returning errors callers don't check for anyway.
Also, no one passes null to AddSelectionListener, so we don't have to worry
about that part.
This way we don't have to deal with QI to get a Selection out of a weakref.
mfbt weakrefs don't have a SizeOfOnlyThis. In any case, the memory used by the
weakref itself is pretty minor...
Some methods of editor retrieves anchor and focus of selection. However, there
are no methods which directly access RangeBoundary of anchor and focus.
This patch adds it for making editor code simpler and avoiding unnecessary
child offset computation.
MozReview-Commit-ID: EvepQpFMi8S
--HG--
extra : rebase_source : 38f3c506db5c9d8790bbd625d552bfbc8d99cebc
This private method DoAutoScroll() modifies aPoint inside of it, and none of
other callers (StartAutoScrollTimer() and nsAutoScrollTimer::Notify()) read
aPoint afterwards, so we make aPoint pass by value rather than pass by
non-const-reference. This is necessary for later parts.
MozReview-Commit-ID: 9PtxFXIka7X
--HG--
extra : rebase_source : 3bd47f071b3cecdc439ebc3b56c6a4f7ef56eff8
Selection should have Collapse() methods which take RawRangeBoundary instead of
a set of container and offset in it. Then, if caller know only child node but
doesn't know offset in the container, neither callers, Selections nor nsRange
needs to compute offset. This makes them avoid calling expensive method,
nsINode::IndexOf().
MozReview-Commit-ID: 79IRajLe1FE
--HG--
extra : rebase_source : a8ce52ff1654974461d5ecfed98b73d9cca34133
This was automatically generated by the script modeline.py.
MozReview-Commit-ID: BgulzkGteAL
--HG--
extra : rebase_source : a4b9d16a4c06c4e85d7d85f485221b1e4ebdfede
nsISelectionPrivate is accessible to script, while nsISelection is
not, so making the former inherit from the latter means script doesn't
have a complete view of the inheritance chain so the XPIDL compiler
produces an error.
It turns out that nothing in script relies on this inheritance, which
makes sense because I'm not sure how it would even work, so just
remove it.
MozReview-Commit-ID: 3Py2T7cprlD
--HG--
extra : rebase_source : 89fac3ad0f7a30c1c71da79d554ebfa153d5fb33
There is no reason to keep StartBatchChanges and EndBatchChanges in nsISelectionPrivate since this is noscript method. And if moving it to Selection, we can remove virtual keyword.
MozReview-Commit-ID: Go6njiW3r2x
--HG--
extra : rebase_source : cb5bfce8312de7d49496e5f841e9786ff16102f6
Similar to Selection::Collapse(), if mCachedRange is available,
Selection::SetBaseAndExtent() should use it rather than creating new nsRange
instance.
Then, it can reduce the allocation cost and may reduce some other cost, e.g.,
adding it to mutation observer.
MozReview-Commit-ID: InQQusw2KMc
--HG--
extra : rebase_source : 967f0d4ad2b7bc706e417af547bbbb21e5f54306
When setting value of <input type="text">, nsTextEditorState removes all
ranges of normal selection first. Then, TextEditor sets the value. Finally,
TextEditor collapses the selection at the end of the text.
In bug 1386471, we got that there are some problems to remove the call of
Selection::RemoveAllRanges() in nsTextEditorState. Therefore, we need another
approach to improve Selection::Collapse().
The approach of this patch is, when removing all ranges from normal selection,
Selection can cache an nsRange instance if there is an instance which is not
referenced from other than the Selection (i.e., it'll be removed when
Selection::Clear() is called). Then, Selection::Collapse() can reuse it. With
this fix, Selection::Collapse() can reduce allocation cost and may reduce some
other cost like adding it to mutation observer.
However, keeping nsRange instance may cause increasing mutation observer's cost
since nsRange will be adjusted its start node/offset and end node/offset with
mutation observer to guarantee that the range is always valid. So, we can
cache such range only when the caller (or its callee) will set selection range
later. Therefore, this patch adds Selection::RemoveAllRangesTemporarily()
and make only nsTextEditorState::SetValue() and
ContentEventHandler::OnSelectionEvent() use it.
MozReview-Commit-ID: FjWrbz4S1ld
--HG--
extra : rebase_source : 83677640525e0b1a84bdd7fce63ff4704b9cc22b
nsISelectionController::SELECTION_* are declared as bit-mask. However, no
methods of nsISelectionController treat them as bit-mask and these
values need a switch statement in nsFrameSelection to convert SelectionType to
array index of nsFrameSelection::mDOMSelections because it's too big to create
an array to do it. Additionally, this conversion appears profile of
attachment 8848015.
So, now, we should declare these values as sequential integer values.
However, only nsTextFrame uses these values as bit-mask. Therefore, this patch
adds new type, SelectionTypeMask and creates new inline method,
ToSelectionTypeMask(SelectionType), to retrieve mask value for a SelectionType.
MozReview-Commit-ID: 5Za8mA6iu4
--HG--
extra : rebase_source : 86617c1f5fa23166458f4353cb834f9e7c5b131b
