I guess that the `Maybe` is `mOffset` of `EditorDOMPointBase` because new
`Maybe::value()`s are called with checking `Maybe::isSome()`. So, accessing
`EditorDOMPointBase::mOffset` newly should cause the assertion.
Then, I found a new caller `IsCharASCIISpace()` which calls `Char()` without
validation here:
https://hg.mozilla.org/mozilla-central/diff/289c293af80b12744b5d35c5b8427ba8d8ebf13e/editor/libeditor/WSRunObject.cpp#l1.383
That could be unset, but I cannot reproduce it, but I succeeded to reproduce
similar assertion hit with using empty text node (see the crashtest). I hope
this fixes the original crash too.
Differential Revision: https://phabricator.services.mozilla.com/D79816
This patch tries to implement Blink-compat white-space normalizer for
`HTMLEditor`.
It's difficult to list up our traditional white-space normalization rules
because `WSRunObject` touches white space sequence only when there is not
acceptable case, e.g., an ASCII white-spaces will be adjacent to another
one, and replaces only unacceptable white-space only. Therefore, whether
white-space sequence may start with either an ASCII white-space or an NBSP.
On the other hand, Blink and WebKit makes white-space sequence always
starts with an NBSP or an ASCII white-space (unfortunately, they behave
differently!). So, for web-compat, we should simulate Blink's behavior
because either behavior is reasonable but Blink have more market share.
This patch simply adds new white-space normalization path for the new one,
and it's switchable with a pref, and still disabled by default.
The other reason why we should do this is, our traditional white-space
normalizer touches the DOM a lot of times per edit action, and the timing
is both before and after touches the DOM tree. Therefore, it's difficult
to compute actual deleting range for `InputEvent.getTargetRanges()` and
touching a lot of times causes running mutation event listeners a lot and
creates a lot of transaction class instances. So, new one have a lot of
merits:
1. Improve web-compat
2. Improve the peformance
3. Improve the security
4. Improve the footprint (but this is now worse then traditional one)
5. Simplify the implementation
The new normalizer is mostly implemented with only 3 `HTMLEditor` methods.
One is `HTMLEditor::DeleteTextAndNormalizeSurroundingWhiteSpaces()`. This is
semi-public method for the edit action handlers. This takes a range with
2 `EditorDOMPoinInText` to delete the range simply. This also replaces
surrounding white-space sequence if necessary. For inserting text case,
this method also handles only white-space normalization when it's called
with collapsed range, i.e., same `EditorDOMPointInText`. This tries to use
`RepaceTextWithTransaction()` as far as possible to reduce creation cost of
transaction classes and the footprint.
Another one is `HTMLEditor::ExtendRangeToDeleteWithNormalizingWhiteSpaces()`.
This tries to extend the given range to normalize surrounding white-spaces.
This is currently not optimized for footprint because this may include
white-spaces which do not need to be replaced. This optimization should be
done before shipping, but for now, enabling `InputEvent.getTargetRanges()` in
Nightly channel is more important. So that it should be done in a follow-up
bug.
The other is `HTMLEditor::GenerateWhitepaceSequence()`. This creates
normalized white-space sequence with surrounding character information.
For keeping this method simple as far as possible, we shouldn't optimize
the range of generation even in follow-ups.
Finally, the white-space sequence is not tested in mochitests, so that we
can enable this new normalizer when we run mochitests under
`editor/libeditor/tests`. However, WPT has some tests. We should keep
them running with current normalizer for checking regression. Instead,
we should enable the pref only for the new WPT added by the previous patch.
Depends on D78655
Differential Revision: https://phabricator.services.mozilla.com/D78656
I realized that there is no word "whitespace" in formal English. This patch
replaces it with "white-space" in comments, and change method names to use
"WhiteSpace".
Depends on D78654
Differential Revision: https://phabricator.services.mozilla.com/D78655
Callers of `WSRunObject::GetASCIIWhitespacesBounds()` may want to scan only
previous or next whitespaces. Therefore, we can split it to save creation
cost of `EditorDOMPointInText`.
Additionally, this makes them scan whitespace sequence in a text node until
hitting its end for avoiding to use expensive API of `WSRunScanner`.
Differential Revision: https://phabricator.services.mozilla.com/D77986
This change was only needed when the previous version of the patch for Bug 1632425 wanted to move things into contextmenu event handler. This patch reverts the change as 1) the suggested behavior never landed and 2) opening context menu in a test can cause conflict with other tests.
Differential Revision: https://phabricator.services.mozilla.com/D78673
Fix of bug 1320229 allowed to paste longer text than `maxlength` attribute of
`<input>` and `<textarea>` because it was thought that the longer text causes
"too long" invalidate state, makes users notified and prevent to submit
form data.
However, according to Bug 1636855 comment 7 (*1), it breaks a major enterprise
web app, SAP, at least because it sends form data without checking validity of
each form data and discards invalid data on server side silently.
According to bug 1636855 comment 24 (*2), one of new behavior's fault is
on Gecko side too. The style of `<input>` element or `<textarea>` element
which has too long text after pasting is changed when it loses focus.
Therefore, users can post the data before they know pasted data is too
long if sending the form data with `Enter` key or something immediately
after pasting (i.e., without moving focus) web apps handle it by themselves.
On the other hand, the original bug report, bug 1320229, should be solved in
the future especially in password field because users may register password
which is cut by `maxlength` silently and they don't use builtin password
manager, only the pasted password is saved, and then, they won't be able to
login as the account. This is really long standing issue of the web forms.
An article (*3) warned this to web developers in 2011. Therefore, we should
keep going advance for solving this issue at least in Nightly channel to get
more feedback from testers and web developers.
1 https://bugzilla.mozilla.org/show_bug.cgi?id=1636855#c7
2 https://bugzilla.mozilla.org/show_bug.cgi?id=1636855#c24
3 https://www.christophermanning.org/writing/dont-use-maxlength-on-password-inputs
Differential Revision: https://phabricator.services.mozilla.com/D78613
According to the telemetry data, almost all web apps in the word do not use
mutation event listeners, but if they are used, editor needs to check
whether modifying node is modified by web apps. The checking cost may not
cheap in some cases. Therefore, we check whether the window has at least one
mutation event listeners or not before non-cheap checking.
However, like actual web apps, most our mochitests and WPT do not add
mutation event listeners. Therefore, such additional checking code is not
tested even though they should work as exactly same as the case when
there are mutation event listeners but they don't touch the DOM.
This patch makes it always return only in debug build for checking the
complicated path.
Differential Revision: https://phabricator.services.mozilla.com/D78173
There's no use case for stateful comparators, so they can be just plain
function pointers.
This is used in some hot places like CSS selector matching.
Differential Revision: https://phabricator.services.mozilla.com/D77084
We were dealing with it correctly when switching display from e.g. block
to inline, or such. But we were not dealing with it when the node was
undisplayed.
Handle it properly, and free one frame bit while at it. We can't really
do this for ManualNAC (the editor resizers) because they request a
reframe explicitly.
Differential Revision: https://phabricator.services.mozilla.com/D76679
In favor of the NativeAnonymous versions which they forward to.
Done automatically with:
rg -l 'IsInAnonymousSubtree' | xargs sed -i 's/IsInAnonymousSubtree/IsInNativeAnonymousSubtree/g'
And removing the function definitions afterwards.
Differential Revision: https://phabricator.services.mozilla.com/D76681
It's not used by mozilla-central, comm-central nor BlueGriffon, and it cannot
work with replacing content, mutation event listener's changes. Therefore,
we should remove this for now. If we need to support this **feature** in some
business reasons, we should provide better API than this anyway.
Differential Revision: https://phabricator.services.mozilla.com/D76080
Currently, when `HTMLEditor` replaces text in a text node, `HTMLEditor`
creates a set of `DeleteTextTransaction` and `InsertTextTransaction`.
However, this has bad impact for footprint and causes the callers messy.
This patch creates `ReplaceTextTransaction` instead and
`HTMLEditor::ReplaceTextWithTransaction()` as its wrapper. Unfortunately,
this becomes not calling `nsIEditActionListener::DidDeleteText()`, however,
this is not used by mozilla-central, comm-central nor BlueGriffon. IIRC,
it was not removed for some legacy addons of Thunderbird. Therefore, it
must be okay to remove it.
Differential Revision: https://phabricator.services.mozilla.com/D76078
For warning the potential of callers' bug, `GetPreviousSiblingOfChild()` outputs
warning into the console when its container is not a container element, e.g.,
it's odd trying to get previous sibling of child if the container is a text
node. Therefore, this patch makes the new caller,
`WSRunScanner::GetPreviousEditableCharPoint()`, check
`CanContainerHaveChildren()` before calling it.
Differential Revision: https://phabricator.services.mozilla.com/D75888
This patch makes it use early-return style and adds some comments. Its behavior
should be changed as soon as possible for web-compat. Therefore, I don't
write current behavior as comment at declaration.
Differential Revision: https://phabricator.services.mozilla.com/D75476
Although the new name is not true when neither
`maybeNBSPFollowingVisibleContent` nor `isPreviousCharASCIIWhitespace` is true.
Differential Revision: https://phabricator.services.mozilla.com/D75475
`WSRunScanner` scans around given point in `GetWSNodes()` at construction with
using `HTMLEditUtils` methods and caches editable text nodes between
`mStartReasonContent` and `mEndReasonContent`. However, it's used only by
`GetNextCharPoint()` and `GetPreviousCharPoint()`, and they shouldn't be
referred after changing the DOM tree. Therefore, we can scan it directly
only when it needs to scan.
The patch rewrites `GetNextCharPoint()` and `GetPreviousCharPoint()` without
`mNodeArray` and removes `mNodeArray` from its member. This may increase the
cost of scanning next/previous text node, but improves the scan performance
when it does not treat so wide range and they are called with a point whose
container is not a text node.
This patch unexpectedly changes the behavior of them, that causes the fix of
2 failures in `insertlinebreak.html` and `insertparagraph.html`. According to
debugger, previously GetNextCharPoint()` in
`ScanNextVisibleNodeOrBlockBoundaryFrom()` called point at `<br>` element
returned no next char, then, `ScanNextVisibleNodeOrBlockBoundaryFrom()` returned
end point which is block boundary of `<listing>` element (it is legacy HTML2
element and treated as `<pre>` element internally). Therefore, the inserted
`<br>` element was misunderstood as invisible `<br>` at end of a block and
inserted another `<br>` element for making it visible. However, the redesigned
one fixed this bug with searching correct text node. Therefore, I cannot
keep the buggy behavior in the new designed methods.
Depends on D75470
Differential Revision: https://phabricator.services.mozilla.com/D75471
`WSRunObject::DeleteRange()` removes only text nodes which are stored when
`WSRunObject` is created. Although it removes text nodes if it's removed,
this patch does not take care about it in the new method. The reason is
the following patch will remove `mNodeArray` and anyway DOM tree modifiers
can check whether they are in proper position before access if it's needed.
Differential Revision: https://phabricator.services.mozilla.com/D75470
