Before moving them from `EditorBase`, they should take only one option argument
whose type is an `EnumSet` class.
Note that I wanted to name each option for optional behavior, but
`FindAnyDataNode` requires to revert the its meaning (I.e., only with this
patch, `true` should be set an option). Therefore, the following patch renames
it.
Differential Revision: https://phabricator.services.mozilla.com/D113228
Like the other browsers, we shouldn't allow to user modify content in
`<select>` element. Note that this case won't occur with moving caret
with input devices. This occurs only with Selection API.
FYI: Chrome deletes `<select>` element if children of it are selected,
but it's odd behavior since user don't see where is selected visually
and may cause unexpected data loss (i.e., deleting `<select>` element).
Perhaps, we should investigate the behavior in other replaced elements
like `<textarea>` etc in follow up bugs.
Depends on D112513
Differential Revision: https://phabricator.services.mozilla.com/D112517
It's hard to understand each caller of `HTMLEditor::IsEmptyNode()` tries to
check with multiple `bool` arguments. Therefore, they should be replaced
with an `EnumSet`.
Note that only the first argument is reverted the meaning. Therefore, if
it's omitted or `false`, `EmptyCheckOption::TreatSingleBRElementAsVisible`
is specified explicitly. Otherwise, i.e., `true`, nothing should be
specified.
Differential Revision: https://phabricator.services.mozilla.com/D112513
It returns true only when it's a text node, but the text is empty.
However, `HTMLEditUtils::IsVisibleTextNode()` is used in
`HTMLEditor::IsEmptyNodeImpl()`. So, we replace it with directly using
`HTMLEditUtils::IsVisibleTextNode()`.
Depends on D112511
Differential Revision: https://phabricator.services.mozilla.com/D112512
I think that it's no matter to make `WSRunScanner` work without editing host.
Therefore, this patch make it take pointer of `Element` rather than reference.
Depends on D112502
Differential Revision: https://phabricator.services.mozilla.com/D112503
Currently, its `mEditorWasDestroyed` is set to true even while `input` event
is being dispatched, i.e., even when it finished everything for the edit action.
However, the flag should be only for blocking unexpected cause which is caused
by mutation event listeners or something other unexpected event listeners
which shouldn't be run while editor is handling an edit action.
Therefore, this patch makes it store "handled" state which is marked before
notifying editor observers and dispatching `input` event and the flag won't
be set to true after it's marked as "handled".
Depends on D111579
Differential Revision: https://phabricator.services.mozilla.com/D111580
The editing session is temporarily disabled when opening a document. Then,
when the document is closed, the editing session is restored with same
`HTMLEditor` instance. Therefore, initializing editor may occur during
an edit action is being handled.
Probably, this must not be used in normal webapps since mutation events are
used rarely. So, I think that for avoiding further complicated things, we
should make editor won't accept new edit action handling after original
editing session ended.
This patch renames `AutoEditActionDataSetter::CanHandle()` to
`IsDataAvailable()` since it just checks whether the important resource is
available or not. And makes new `CanHandle()` return `false` for
non-initializing edit actions if editing session has ended.
Differential Revision: https://phabricator.services.mozilla.com/D111241
When given point is **in** a void element, e.g., this can be when JS inserts
nodes into a void element like `<meta>`,
`HTMLEditor::SplitNodeDeepWithTransaction` may return "not handled" state.
So, it's possible case, we shouldn't assert the case.
This patch adds both WPT and crashtests because I couldn't reproduce it
with WPT's simpler API use.
Differential Revision: https://phabricator.services.mozilla.com/D111231
When the method detects the style coming from a parent block, the `MOZ_ASSERT`
checks whether the current mode is "style with CSS" or not because currently,
`HTMLEditor` does not use `<span>` with CSS when the mode is not "style with
CSS". However, fixing it requires bigger patch, and the change does not match
with the bug's summary. So, I just remove the `MOZ_ASSERT`, add WPT and filed
bug 1649639 for the root issue.
Differential Revision: https://phabricator.services.mozilla.com/D111067
The lifetime of it is guaranteed by `AutoEditActionDataSetter` which grabs
`Selection` until it's destroyed, and it's a stack only class and created
at first step of all public method calls. Therefore, we can mark it as
`MOZ_KNOWN_LIVE` and we can change it returning reference of `Selection`
instead of reference of `RefPtr<Selection>`.
Differential Revision: https://phabricator.services.mozilla.com/D110896
Currently, it dispatches `input` event with holding `mPlaceholderTransaction`
and `mPlaceholerBatch` as 1. If `input` event listener sets value in XUL
document, the setting value transaction will break the existing placeholder
transaction. Then, only the last one will be undone, and it causes inserting
new padding `br` element with transaction and clears redo history.
This patch makes it forget them before dispatching `input` event.
Note that I tried to create automated test for this, but I cannot reproduce
this with simple testcase. I guess something other things also required to
reproduce this bug.
Differential Revision: https://phabricator.services.mozilla.com/D110715
This is a bug of the assertion.
When there is 2 text nodes are adjacent, one ends with a visible character and
the other starts with a white-space, and the deleting range ends at end of the
first one, `GetReplaceRangeDataAtEndOfDeletionRange()` returns a range starting
with the start of the second one since it does not need to touch the first one.
Therefore, the white-space range start may be after the deleting range end.
This patch adds new condition to the `MOZ_ASSERT_IF`, and test this specific
case with new `MOZ_ASSERT_IF`.
Differential Revision: https://phabricator.services.mozilla.com/D110068
Those command handlers just check whether the given editor is an `HTMLEditor`
or not. Therefore, we should make them check whether the given editor is
a single line editor or not instead.
Differential Revision: https://phabricator.services.mozilla.com/D108572
Although these commands supported supported only by Gecko, we shouldn't stop
supporting them unless we know the usage in the wild. Therefore, this patch
adds the handling code for `TextEditor` too.
Differential Revision: https://phabricator.services.mozilla.com/D108571
For making `execCommand` and related methods with `<input>` and `<textarea>`
even if they are in `contenteditable` and focused, command should be handled
in active editor (focused editor if in foreground window and tab).
However, some commands should be handled by `HTMLEditor` even if an `TextEditor`
has focus. Therefore, this patch adds new enum class which have 3 state into
`InternalCommandData` and makes `AutoEditorCommandTarget` consider it with the
enum class.
Note that the new failures about `contentReadOnly` command will be fixed by
a following patch.
Differential Revision: https://phabricator.services.mozilla.com/D108569
Those command handlers just check whether the given editor is an `HTMLEditor`
or not. Therefore, we should make them check whether the given editor is
a single line editor or not instead.
Differential Revision: https://phabricator.services.mozilla.com/D108572
Although these commands supported supported only by Gecko, we shouldn't stop
supporting them unless we know the usage in the wild. Therefore, this patch
adds the handling code for `TextEditor` too.
Differential Revision: https://phabricator.services.mozilla.com/D108571
For making `execCommand` and related methods with `<input>` and `<textarea>`
even if they are in `contenteditable` and focused, command should be handled
in active editor (focused editor if in foreground window and tab).
However, some commands should be handled by `HTMLEditor` even if an `TextEditor`
has focus. Therefore, this patch adds new enum class which have 3 state into
`InternalCommandData` and makes `AutoEditorCommandTarget` consider it with the
enum class.
Note that the new failures about `contentReadOnly` command will be fixed by
a following patch.
Differential Revision: https://phabricator.services.mozilla.com/D108569
This test case doesn't generate body element and isn't general case. Actually
since we don't manage current IME focus on IMEStateManager, we shouldn't notify
IMEContext of widget.
Differential Revision: https://phabricator.services.mozilla.com/D108366
When typing at edge of a link, editor splits the link element at the edge (i.e.,
creating an empty link element). Then, unlink the new link element (i.e.,
clearing the "link style"). At this time, `class` attribute and `style`
attribute are cloned to new `<span>` element for keeping the original style.
However, in this case, other browsers discard the specified style of the link.
On the other hand, when unlinking a link with `execCommand("unlink")`, the
other browsers keep specified style coming from `style` attribute.
Therefore, this patch adds new state to `PropItem`, which indicates whether
the `class` and `style` attribute should be cloned or discarded. And then,
when preparing for inserting text, this patch makes it pass to the utility
method.
For better compatibility, we should stop cloning `class` attribute, but not
in this bug because this patch should be minimized for requesting uplift to
beta channel.
Differential Revision: https://phabricator.services.mozilla.com/D107801
E.g., comment node is an invisible data node, and it's not handled by
`HTMLEditor` nor `ContentEventHandler`. Therefore, `IMEContentObserver`
should ignore invisible data nodes, which are not derived from `dom::Text`.
Differential Revision: https://phabricator.services.mozilla.com/D107590
For making delete handlers simpler, and set better target ranges to the
corresponding `beforeinput` event, we should ignore non-editable ranges
before handling deletion.
This patch makes editor stop handling deleteion when a range crosses editing
host boundaries. In this case, Gecko has done nothing, but fired
`beforeinput` event. Note that Blink deletes editable contents in the range
**until** it meets first non-editable content, but I don't think this is
a good behavior because it makes things complicated. Therefore, I filed
a spec issue: https://github.com/w3c/editing/issues/283
On the other hand, this behavior change causes different behavior in
https://searchfox.org/mozilla-central/source/editor/libeditor/crashtests/1345015.html
It tries to insert paragraph into `<html>` element, but our editor currently
does not support it. Therefore, it hits `MOZ_ASSERT`. Therefore, this patch
added a new check into `HTMLEditor::InsertParagraphSeparatorAsSubAction()`.
Differential Revision: https://phabricator.services.mozilla.com/D107588
Blink treats each non-editable node as an atomic object. E.g., deleting or
forward-deleting from next to a non-editable element, it deletes only one
non-editable element.
Unfortunately, our layout treat adjacent non-editable nodes as a node.
Therefore, the adding WPTs do not work, but they are not new regression of
this patch.
Differential Revision: https://phabricator.services.mozilla.com/D107587
It does not make sense `WSRunScanner` handles invisible white-spaces in
non-editable elements. Therefore, this patch makes it stop handling in the
cases.
Note that this change causes new fail of some WPTs. That will be fixed by
the following patch.
Differential Revision: https://phabricator.services.mozilla.com/D107586
No behavior change. We were already returning false for HTML editor +
non-editable so we can simplify it a bit.
Depends on D107511
Differential Revision: https://phabricator.services.mozilla.com/D107512
There are 2 bugs. One is that `SplitNodeDeepWithTransaction` tries to split
comment node, but it fails. The other is, the failure result is not checked
by `HandleInsertParagraphInHeadingElement`. Therefore, the original head
element's previous node may not be a heading element.
Depends on D106591
Differential Revision: https://phabricator.services.mozilla.com/D106620
browserscope is no longer maintained, so contenteditable=false case is
incorrect now. So I would like to update this tests for non editable case.
Differential Revision: https://phabricator.services.mozilla.com/D96299
Since `WSScanResult` can return non-editable/non-removable text node,
`HandleDeleteAtomicContent` is called with non editable text node, then it cannot
remove atomic content.
This fix doesn't follow other block case such as added newer test.
Differential Revision: https://phabricator.services.mozilla.com/D96298
Currently, this feature is implemented only on Linux and macOS (see also
bug 1077515 and bug 1301497), and the code is really similar each other.
Additionally, it always tries to query selection to check whether the caret is
in vertical content or not if arrow keys are pressed. For avoiding a lot of
query, this patch makes `TextEventDispatcher` cache writing mode at every
selection change notification. However, unfortunately, it's not available when
non-editable content has focus, but it should be out of scope of this bug since
it requires a lot of changes.
Anyway, with this patch, we can write a mochitest only on Linux and macOS.
The following patch adds a test for this as a fix of bug 1103374.
Differential Revision: https://phabricator.services.mozilla.com/D102881
Applying default edit action for some shortcut keys on Linux and macOS
causes some oranges. The new failure reasons are:
* The keyboard events are consumed in the bubbling phase of the system
group if editor has focus (`EditorEventListener` or `TextInputListener`
does it).
* No key combinations are mapped to per-word move or select on macOS.
* Home/End keys on macOS do not change selection.
Therefore, this adjusts each failure case for the new behavior.
Differential Revision: https://phabricator.services.mozilla.com/D102878
The issue is that the test was depending on the native resizers (or on a
previous test having loaded the non-native ones).
The first snapshot will not have resizers (because we just started the
load of the SVG) and thus following snapshots would fail.
Wait for the load event and pre-show the ltr textarea, so that the
resizer is known loaded.
This doesn't happen without the non-native theme because that uses
native resizers which don't depend on the image load.
Differential Revision: https://phabricator.services.mozilla.com/D103591
According to the debug log and result on tryserver, the frequency of this
test failure becomes higher is caused by bug 1676966. When global reflow
is kicked during dragging the grabber, the implicit capture is released
forcibly. Fortunately, editor behavior shouldn't depend on font list
loading system, so, disabling the feature until fixing bug 1688430 is fine.
Differential Revision: https://phabricator.services.mozilla.com/D102889
This is same behavior as the other browsers. When selection is collapsed to
a edge of a link after deletion, we should stop applying link style to new
inserted content.
Differential Revision: https://phabricator.services.mozilla.com/D101006
When selection is not collapsed and is across link element boundary, we can
assume that user does not try to modify the link text because the other
browsers behave so. Therefore, we should take the same behavior in this case.
Note that all the new failures of `editing/run/inserttext.html` are passed
only on Gecko. So, it should be updated in another bug.
Differential Revision: https://phabricator.services.mozilla.com/D101005
This is the most important situation. If selection is collapsed to a edge of a
link, the other browsers does not insert new content into the link. So, from
the point of view of web developers, this cause should work exactly same as
the other browsers.
Note that the new failures in `editing/run/inserttext.html` are passed only
on Gecko. So, the test should be updated after fixing this bug.
Differential Revision: https://phabricator.services.mozilla.com/D101004
When caret is not moved but selection change command is fired, it means that
the caret is already set to start or end of editing host. In this case, we
should stop keeping link style for new inserting content because otherwise,
user cannot exit from the first or last link with arrow keys.
Differential Revision: https://phabricator.services.mozilla.com/D101003
Although different from the other browsers' behavior, our traditional behavior
might be better than them because the behavior on the other browser does not
allow users to insert new content at start nor end of a link.
However, we can relax more about keeping traditional behavior for web-compat.
Perhaps, only when caret is moved from the other side of first or last character
in the link and moves caret to the edge of the link with arrow key, we should
allow users to modify the link text.
Otherwise, e.g., `Home` and `End` key press should make it stop keeping the
link style. This helps advanced users who are familiar with keyboard navigatin
in editor.
Note that this patch also changes the condition which checks
`aReason & nsISelectionListener::KEYPRESS_REASON` to check also
`nsISelectionListener::COLLAPSETO_START_REASON` and
`nsISelectionListener::COLLAPSETO_END_REASON` because all of them are
set by `nsFrameSelection::MoveCaret` exclusively.
https://searchfox.org/mozilla-central/rev/a0ccd492719b1ad2106f6456549be62a76f45acb/layout/generic/nsFrameSelection.cpp#738,741,745
Therefore, they should be treated as same as a key press.
Note that they are also set by `Selection::CollapseToStart` and
`Selection::CollapseToEnd` too. But a following patch will add a new
reason to notify selection listeners of caused by JS. So, the problem
will be fixed by the following patch.
Differential Revision: https://phabricator.services.mozilla.com/D101002
When mouse button is clicked outside a link element but caret is positioned
start or end of the link, our traditional behavior keeps inserting new content
into the link. But this is different from the other browsers, and it does
not make sense to treat such selection change is intended to keep typing in
the link element.
Therefore, this patch makes `TypeInState::OnSelectionChange()` handle
selection change reason is `mousedown` and `mouseup` cases. However,
it cannot know whether the event was fired in the parent link element or
not. Therefore, this patch makes `HTMLEditorEventListener` notifies
`TypeInState` of mouse events via `HTMLEditor`.
Differential Revision: https://phabricator.services.mozilla.com/D101001
They were designed for object resizer and grabber to move absolutely positioned
element. Therefore, they should have better name to explain what they do.
Then, we can create event listener methods for generic cases.
Differential Revision: https://phabricator.services.mozilla.com/D100999
Currently, the test checks the result only in `contenteditable`, but I'd be
better to do same tests in `designMode` editor too.
Additionally, for detecting regressions of the following patches, it should
check the result of 2nd typing too because inserting text into end of a link
element may cause unlink it.
Differential Revision: https://phabricator.services.mozilla.com/D100997
DONTBUILD because this is a comment-only change and hence doesn't affect
behavior at all.
I ran across this typo in nsPageFrame.cpp, and figured I might as well grep for
it and fix it more comprehensively. And while I'm at it, I also fixed some
neighboring instances of a duplicated word around a linebreak, in
nsXMLContentSerializer.h.
Differential Revision: https://phabricator.services.mozilla.com/D100852
Before deleting `IMEState::Enabled::PLUGIN`, let's make it an enum class
for making the change safer. Almost all of this change is done by
"replace" of VSCode.
Differential Revision: https://phabricator.services.mozilla.com/D100100
This lifts a bunch of string conversions higher up the stack, but allows
us to make the servo code use utf-8 unconditionally, and seemed faster
in my benchmarking (see comment 0).
It should also make a bunch of attribute setters faster too (like
setting .cssText), now that we use UTF8String for them (we couldn't
because we couldn't specify different string types for the getter and
setters).
Differential Revision: https://phabricator.services.mozilla.com/D99590
Turns out this causes a bit of overhead when appending to an AutoString,
because Truncate() calls SetToEmptyBuffer(), which makes it go from
inline-string to buffer string, and next time we append we need to turn
the string from a buffer-based string to an inline string again, which
is some minor, but measurable, work, while profiling some
micro-benchmarks.
Probably we should make Truncate() a no-op for a zero-length string, but
I'll send that separately.
Differential Revision: https://phabricator.services.mozilla.com/D99493
DragEvent::GetDataTransfer() is fallible when memory allocation fails as it lazily constructs DataTransfer. This tries failing gracely when that happens.
Differential Revision: https://phabricator.services.mozilla.com/D97663
There are not enough tests comparing delete operation result and result of
`getTargetRanges()` when selection is in/around/across list item elements.
This patch creates a utility method to make the test body not need to use
Selection API for making simpler tests.
The expected behavior is based on Blink and WebKit unless their behavior is
buggy because their behavior is more reasonable than Gecko's in most cases.
Note that the removing tests are covered by the new tests.
Differential Revision: https://phabricator.services.mozilla.com/D96800
I guess that `DataTransfer::HasType()` is inlined in the opt builds and
actually crashed in `EditorEventListener::DragEventHasSupportingData()`
at accessing `aDragEvent->GetDataTransfer()` result without null-check
because `DataTransfer::mItems` is set to `nullptr` only by the cycle
collector, but it does not make sense to think that it occurs the STR
in bug 1627673 comment 3.
Therefore, this patch adds null-checks in
`EditorEventListener::DragEventHasSupportingData()`.
I have no idea how to test this with automated tests.
Differential Revision: https://phabricator.services.mozilla.com/D96310
`InsertBRElementIfHardLineIsEmptyAndEndsWithBlockBoundary()` checks whether
a line containing `aPointToInsert` requires invisible `<br>` element.
Therefore, `aPointToInsert` must be set and valid. However, all selection
ranges may be removed by somebody when modifying the DOM tree before that.
Therefore, this patch makes all callers of it check whether they succeeded
to retrieve caret position or not.
Differential Revision: https://phabricator.services.mozilla.com/D96280
They join first line of a descendant block element into the line at the other
block contains the descendant block. First of all of their jobs, they call
`DeleteInvisibleASCIIWhiteSpace()` to clean up unnecessary whitespaces at
the other block. At this time, the point which tells whether the descendant
element is contained in the other block may become invalid if script runs
and it causes changing the DOM tree. Therefore, they should check the
given point is still valid after calling `DeleteInvisibleASCIIWhiteSpace()`
because `AutoTrackDOMPoint` requires valid point.
Differential Revision: https://phabricator.services.mozilla.com/D95987
This supports one manifestparser expression per line in the 'skip-if',
'fail-if' and 'run-if' keys. As a side effect the:
skip-if = foo ||
bar
syntax is no longer supported. Instead it can be:
skip-if =
foo # bug 123
bar # bug 456
Differential Revision: https://phabricator.services.mozilla.com/D95927
`HTMLEditor` ignores input events when a text control element in an editing
host has focus.
https://searchfox.org/mozilla-central/rev/d866b96d74ec2a63f09ee418f048d23f4fd379a2/editor/libeditor/HTMLEditor.cpp#6322
However, focus event does not check it and modifies ancestor limiter of
`Selection` for the document. And even in this case, public methods for
handling some edit actions are open for `execCommand` requests. Then,
edit action handlers may set selection outside the elements, but new focus
event to update the selection ancestor limiter will be handled later.
Therefore, until getting next focus event, `HTMLEditor` does not work as
expected since `Selection::*InLimiter()` APIs refuse to change selection.
Ideally, we should make `nsFocusManager` notifies editor of focus change
synchronously, but I cannot fix it quickly because it needs to touch security
sensitive area. Therefore, this patch makes `HTMLEditor` do not set
ancestor limiter when a text control in any editing host gets focus.
Additionally, this patch makes
`HTMLEditor::AutoDeleteRangesHandler::AutoBlockElementsJoiner::HandleDeleteAtOtherBlockBoundary`
stop handling leaf node deletion if it cannot change caret position.
Differential Revision: https://phabricator.services.mozilla.com/D95268
The test runs `execCommand("insertHorizontalRule")` when the document node
has no children. In this case, `TextFragmentData` fails to initialize itself.
In this case, our editor cannot do anything. Therefore, returning error
in this case must be better.
This patch makes all callers of the scan methods handle the error case unless
the caller cannot return error.
Comparing with Blink, perhaps, we should insert `<html>` and `<body>` element
in this case first and keep doing the requested operation in the future.
Currently, doing it may cause another complicated issue with mutation event
listeners.
Differential Revision: https://phabricator.services.mozilla.com/D95126
The reason of hitting the assertion is, a clipboard event listener nests
another edit action, initializing the editor, runs, and tries to create an
anonymous `<br>` element with transaction, but it checks whether `beforeinput`
event has already been dispatched or not with the parent edit action data.
For fixing it, this patch adds a flag to edit action data to indicate whether
a clipboard event has already been dispatched or not. If it's already been
dispatched, it's **okay** to modify the DOM tree. Ideally, we should put off
modifying the DOM tree after dispatching `beforeinput` event, but this case
does not notify web apps anything so that this patch should be fine.
Although this patch adds a crash test which is attached by the reporter,
it's not reproducible with current editor code because we stopped modifying
native anonymous nodes with transaction, but it depends on the behavior.
Still this assertion hits in `test_clipboard_noeditor.html` intermittently,
but I don't have idea how to reproduce this permanently. Therefore, this
patch does not contain a test to check to regression actually.
Differential Revision: https://phabricator.services.mozilla.com/D95115
Actually, `DeleteNodeWithTransaction` cannot remove non-editable node. But
we should allow it which parent node is ediatble.
Differential Revision: https://phabricator.services.mozilla.com/D94937
It's too big (+15k lines) and there are 2 groups which does not depend on the
others. One is getting current state for `Document.queryCommandState` etc.
This is copied to `HTMLEditorState.cpp`. And the other is delete handlers.
This is copied to `HTMLEditorDeleteHandler.cpp`.
Differential Revision: https://phabricator.services.mozilla.com/D94934
When `beforeinput` event is canceled, the editor content shouldn't be modified.
Therefore, `runTest()` can handle it with caching initial value before
running the test body.
Additionally, this patch makes every test ensure that target editor has focus.
So, finally, each test is now isolated completely. Developers can run only
a test with commenting out the other tests.
Depends on D94520
Differential Revision: https://phabricator.services.mozilla.com/D94521
This patch makes all test functions call `runTest()` and it adds event listeners
only while running the function. Therefore, this makes each test function do:
* initializing for running test
* running test with `runTest` and specifying expected data
Depends on D94519
Differential Revision: https://phabricator.services.mozilla.com/D94520
This allows the following patches will make all functions use batch function
to test with a framework.
Depends on D94517
Differential Revision: https://phabricator.services.mozilla.com/D94518
So, this removes functions named as `*_and_canceling_beforeinput` and
makes corresponding function take an object which has `action` and
`cancelBeforeInput` to reduce dependency on wider scope variables.
Depends on D94516
Differential Revision: https://phabricator.services.mozilla.com/D94517
Although, still there are dependencies between the functions, this allows
stack has meaningful name where a failure occurs.
Depends on D94515
Differential Revision: https://phabricator.services.mozilla.com/D94516
Indent level of the tests will be increased. Therefore, all descriptions
should be in their own lines.
Note that this and following patches rewrite almost all lines in the test
files again and again. Therefore, it's hard to rebase and existing bugs
will be fixed when I was writing each patch. So, I'd like reviewers just
put inline comment which should be fixed before landing. Then, I'll add
a patch to fix them.
Differential Revision: https://phabricator.services.mozilla.com/D94515
So, the root cause of this assertion hit was, I forgot to add this path
when I implement the methods to compute target ranges. Although the other
browsers don't support multiple selection ranges, this patch adds WPTs
for the cases.
Depends on D94241
Differential Revision: https://phabricator.services.mozilla.com/D94242
Allow-list all Python code in tree for use with the black linter, and re-format all code in-tree accordingly.
To produce this patch I did all of the following:
1. Make changes to tools/lint/black.yml to remove include: stanza and update list of source extensions.
2. Run ./mach lint --linter black --fix
3. Make some ad-hoc manual updates to python/mozbuild/mozbuild/test/configure/test_configure.py -- it has some hard-coded line numbers that the reformat breaks.
4. Make some ad-hoc manual updates to `testing/marionette/client/setup.py`, `testing/marionette/harness/setup.py`, and `testing/firefox-ui/harness/setup.py`, which have hard-coded regexes that break after the reformat.
5. Add a set of exclusions to black.yml. These will be deleted in a follow-up bug (1672023).
# ignore-this-changeset
Differential Revision: https://phabricator.services.mozilla.com/D94045
The method is tested by `test_nsITableEditor_getFirstSelectedCellInTable.html`.
And now, nobody uses `CellAndIndexes` so that this patch removes it.
Depends on D94239
Differential Revision: https://phabricator.services.mozilla.com/D94240
Unfortunately, there is no automated tests for `nsITableEditor.joinTableCells()`
and it'll take a couple of days to write its testcase because of too big
method. And it's not so important for Firefox (although it's used in
comm-central). Therefore, this patch skips to check it.
Note that `CellAndIndexes` uses `HTMLEditor::GetFirstSelectedTableCellElement()`
to retrieve first selected cell element and then, it retrieves its indexes.
https://searchfox.org/mozilla-central/rev/61728de8273c04fe2417c475fc0637e8b79210d7/editor/libeditor/HTMLTableEditor.cpp#4090,4098,4109
Therefore, this patch makes it stop using this simple structure too.
Depends on D94235
Differential Revision: https://phabricator.services.mozilla.com/D94236
These APIs are used only in comm-central (not in BlueGriffon) and nobody
refers the out argument which is selected range of the cell selection.
Therefore, we should drop them to make the APIs simpler.
Differential Revision: https://phabricator.services.mozilla.com/D94226
This is a simple mistake. If the given range is collapsed, scanning text node
to before/after from collapsed range may create reversed range.
Differential Revision: https://phabricator.services.mozilla.com/D94225
Allow-list all Python code in tree for use with the black linter, and re-format all code in-tree accordingly.
To produce this patch I did all of the following:
1. Make changes to tools/lint/black.yml to remove include: stanza and update list of source extensions.
2. Run ./mach lint --linter black --fix
3. Make some ad-hoc manual updates to python/mozbuild/mozbuild/test/configure/test_configure.py -- it has some hard-coded line numbers that the reformat breaks.
4. Make some ad-hoc manual updates to `testing/marionette/client/setup.py`, `testing/marionette/harness/setup.py`, and `testing/firefox-ui/harness/setup.py`, which have hard-coded regexes that break after the reformat.
5. Add a set of exclusions to black.yml. These will be deleted in a follow-up bug (1672023).
# ignore-this-changeset
Differential Revision: https://phabricator.services.mozilla.com/D94045
These APIs are used only in comm-central (not in BlueGriffon) and nobody
refers the out argument which is selected range of the cell selection.
Therefore, we should drop them to make the APIs simpler.
Differential Revision: https://phabricator.services.mozilla.com/D94226
This is a simple mistake. If the given range is collapsed, scanning text node
to before/after from collapsed range may create reversed range.
Differential Revision: https://phabricator.services.mozilla.com/D94225
Allow-list all Python code in tree for use with the black linter, and re-format all code in-tree accordingly.
To produce this patch I did all of the following:
1. Make changes to tools/lint/black.yml to remove include: stanza and update list of source extensions.
2. Run ./mach lint --linter black --fix
3. Make some ad-hoc manual updates to python/mozbuild/mozbuild/test/configure/test_configure.py -- it has some hard-coded line numbers that the reformat breaks.
4. Add a set of exclusions to black.yml. These will be deleted in a follow-up bug (1672023).
# ignore-this-changeset
Differential Revision: https://phabricator.services.mozilla.com/D94045
Blink and WebKit do not fire `beforeinput` event when user uses build-in
password manager and autocomplete. But the `inputType` value for this case,
`"insertReplacementText"` is defined as cancelable in the spec, and it's
actually cancelable when it's fired for correcting a word with built-in
spellchecker of them.
For making only our users' autocomplete and password manager not blocked by
web apps, we should make them not cancelable by default, but I think that we
should keep dispatching such non-cancelable `beforeinput` for conforming to
the standard unless we'd get a web-compat report for this.
Differential Revision: https://phabricator.services.mozilla.com/D93206
In strictly speaking, we should shrink selection ranges at very first time
of edit action handling. However, we support multiple selection ranges and
it makes the check cost really expensive, and the code would be really
complicated since ranges cannot be overlapped. I.e., changing one range
could affect some of the others.
Therefore, this patch changes `HTMLEditor::SelectAllInternal()` instead.
If computed selection root is an ancestor of `<body>` element in HTML document,
it use the `<body>` element instead.
Note that, in HTML document, there should be only one `<body>` element and
only its content should be editable at least for now. (Note that in XHTML
document, no `<body>` is allowed, multiple `<body>` elements allowed.)
Differential Revision: https://phabricator.services.mozilla.com/D93712
When the grabber to move absolutely positioned element is disabled,
`HTMLEditor::HideGrabberInternal()` is called to delete it. However,
it does not reset dragging state. Therefore, `mousemove` event listener
will try to handle drag even after the grabber is removed.
This patch makes `HideGrabberInternal()` reset the dragging state to
make the event listener stop handling the drag action.
However, I hit a buggy assertion in `EventStateManager`. It tries to
set active state to parent of the grabber (in this case, absolutely positioned
element). However, editable element in `contenteditable` cannot have
active state. Therefore, `leaf` becomes `nullptr`, but `newleaf` is the
absolutely positioned element. Therefore, this patch adds this condition
into the `MOZ_ASSERT`.
Differential Revision: https://phabricator.services.mozilla.com/D93632
Unfortunately, there is no official way to detect whether browser supports
or not `beforeinput` event in the wild because Chromium does not support
`onbeforeinput` event handler attribute. On the other hand, we're the
last browser vendor which does not support `beforeinput` event, and we
make `InputEvent.getTargetRanges()` enabled only when `beforeinput` event
because it returns meaningful value only when the event type is `beforeinput`.
So, web apps can detect `beforeinput` event support with checking type of
or existence of it instead. However, in our experience, it's guessed what
a lot of web apps check whether "Firefox" or not to consider whether it
can use `beforeinput` events. If so, we need to announce shipping
`beforeinput` event and the way to feature detection before actually shipping
it. Otherwise, we wouldn't get enough feedback from testers.
First of all for shipping `beforeinput` events, we should collect how much
web apps which use `HTMLEditor` use `beforeinput` event when it's enabled,
and how much web apps use mutation events or mutation observer instead of
`beforeinput` events even when it's enabled.
Honestly, I'd like to collect URLs which don't use `beforeinput` event, but
use mutation events or mutation observer. But it's really sensitive data
so that I believe that we shouldn't collect it.
Differential Revision: https://phabricator.services.mozilla.com/D92548
There is similar API in `Document`, but they indicate whether a node has been
observed by any mutation receiver, not only by `MutationObserver` of JS.
However, I'd like to know the percentage of web apps which use
`MutationObserver`, but not use `beforeinput` events. Therefore, this patch
adds similar API into `nsPIDOMWindowInner` as same as `beforeinput` and
ignores `MutationObserver`s which are created by chrome script and addons.
Differential Revision: https://phabricator.services.mozilla.com/D92547
When `HTMLEditor` instances are destroyed, I'd like to collect how much
instances are worked with `beforeinput` event listeners. Before adding such
telemetry probe, this patch adds methods to set/get whether a `beforeinput`
event listener has had added or not to `nsPIDOMWindowInner`.
Differential Revision: https://phabricator.services.mozilla.com/D92546
This patch tries to mark root callers of `nsINode::GetSelectionRootContent()`
which calls `nsINode::GetAnonymousRootElementOfTextEditor()` as far as possible
(and reasonable).
It's used by `ContentEventHandler` so that a lot of methods of
`EventStateManager`, `ContentEventHandler`, `IMEContentObserver` which are main
users of it are also marked as `MOZ_CAN_RUN_SCRIPT`. I think that this is
reasonable.
On the other hand, it might not be reasonable to mark `IMEStateManager` methods
as `MOZ_CAN_RUN_SCRIPT` for initializing `IMEContentObserver` because
`IMEStateManager` may be able to initialize `IMEContentObserver` asynchronously
and its root callers are in XUL layout code. Therefore, this patch uses
`MOZ_CAN_RUN_SCRIPT_BOUNDARY` for `IMEStateManager` at least for now.
Differential Revision: https://phabricator.services.mozilla.com/D92730
The previous behavior was added in bug 858918. However, it was wrong
when trying to insert a `<tr>` into a `<div contenteditable>`.
After this change, the test in bug 858918 still passes, so that specific
functionality doesn't break. However, the code is complex and one can't
exclude that other functionality breaks. The previous fix already
mentioned the same concern
https://bugzilla.mozilla.org/show_bug.cgi?id=858918#c25.
With this change,
https://searchfox.org/mozilla-central/rev/35245411b9e8a911fe3f5adb0632c3394f8b4ccb/editor/libeditor/HTMLEditorDataTransfer.cpp#938
doesn't insert the `<tr>`, but climbs up the `<tr>`'s ancestors, so that
a `<table>` is inserted. When `designMode = "on"`, this was already
previously the case.
The new code seems more correct, including pasting tables to
`contenteditable` elements working now. Therefore, it seems worth
submitting this patch, despite being unable to exclude regressions by
pure reasoning.
Differential Revision: https://phabricator.services.mozilla.com/D92590
This patch tries to mark root callers of `nsINode::GetSelectionRootContent()`
which calls `nsINode::GetAnonymousRootElementOfTextEditor()` as far as possible
(and reasonable).
It's used by `ContentEventHandler` so that a lot of methods of
`EventStateManager`, `ContentEventHandler`, `IMEContentObserver` which are main
users of it are also marked as `MOZ_CAN_RUN_SCRIPT`. I think that this is
reasonable.
On the other hand, it might not be reasonable to mark `IMEStateManager` methods
as `MOZ_CAN_RUN_SCRIPT` for initializing `IMEContentObserver` because
`IMEStateManager` may be able to initialize `IMEContentObserver` asynchronously
and its root callers are in XUL layout code. Therefore, this patch uses
`MOZ_CAN_RUN_SCRIPT_BOUNDARY` for `IMEStateManager` at least for now.
Differential Revision: https://phabricator.services.mozilla.com/D92730
The telemetry probe was added in bug 1506434, and it's not necessary anymore
because of the event in the default group was completely disabled in
bug 1288640 (Gecko 65).
Therefore, we can get rid of the pref, and we can take back a room for a
bool member in `nsPIDOMWindowInner` for new telemetry probes which need
to know whether a specific event listener has been added or not.
Depends on D92395
Differential Revision: https://phabricator.services.mozilla.com/D92397
`editor/libeditor/tests/test_bug974309.html` and
`editor/libeditor/tests/test_bug1268736.html` start to throw exception if
`beforeinput` event is enabled. They test XPCOM editor API when a non-editable
element has focus and it's outside of any editing hosts. In this case,
we shouldn't throw an exception for backward compatibility. Although the
all callers should stop handling any edit action in this case, let's make
`MaybeDispatchBeforeInputEvent()` return a special success code instead of
an error code for making all callers keep working with traditional paths.
Differential Revision: https://phabricator.services.mozilla.com/D91864
If `beforeinput` event is enabled, the assertion count hitting in
`WSRunScanner::GetEditableBlockParentOrTopmotEditableInlineContent()` becomes 0.
The reason is, `AutoEditActionDataSetter::MaybeDispatchBeforeInputEvent()`
gets `nullptr` as the result of `HTMLEditor.GetInputEventTargetElement()`.
Then, it returns error and every edit action handlers stop handling it.
However, for the main purpose of `beforeinput`, i.e., overriding browsers'
default action, we need to dispatch `beforeinput` event even in such case.
(I.e., web apps may want to do something around the non-editable node.)
Therefore, this patch makes `HTMLEditor.GetInputEventTargetElement()` scan
editing host by itself if `HTMLEditor.GetActiveEditingHost()` returns `nullptr`
due to non-editable focus node. And also make `test_bug430392.html` check
whether `beforeinput` events and `input` events are fired as expected.
I think that I should add new WPT to check nested editing host cases for
considering `beforeinput` and `input` event target, but I'd like to do it
in another bug for shipping `beforeinput` in Nightly channel as soon as
possible.
Differential Revision: https://phabricator.services.mozilla.com/D91863
If deleting/replacing range selects a text node, it may shrink the range
to start and end of the text node. So, the result may not be wider than
the original range.
This behavior is compatible with Blink.
Depends on D90639
Differential Revision: https://phabricator.services.mozilla.com/D90640
Currently, target ranges do not extend the ranges even when deleting empty
parent elements of the range. Therefore, the frag indicating whether empty
parents removed or not is not necessary for the methods.
Differential Revision: https://phabricator.services.mozilla.com/D90542
When joining 2 block elements, the left block element may ends with invisible
`<br>` element or the right block element may follows an invisible `<br>`
element if it's a descendant of the left block element. In those cases, the
invisible `<br>` element must be start of the target range of the joining
block elements since it'll be deleted.
Differential Revision: https://phabricator.services.mozilla.com/D90541
This clarifies why the flavors are added in a certain order and why
retrieving the first available one is the best one.
Moreover, this enables previously, accidentally disabled
`NS_WARNING_ASSERTIONS`.
Differential Revision: https://phabricator.services.mozilla.com/D90883
Preparing the transferable has the hidden requirement that the order of
the added flavors matters.
This is the preparation for expressing this in code.
`aTransferable` is renamed in the following commit.
Differential Revision: https://phabricator.services.mozilla.com/D90881
This clarifies why the flavors are added in a certain order and why
retrieving the first available one is the best one.
Moreover, this enables previously, accidentally disabled
`NS_WARNING_ASSERTIONS`.
Differential Revision: https://phabricator.services.mozilla.com/D90883
Preparing the transferable has the hidden requirement that the order of
the added flavors matters.
This is the preparation for expressing this in code.
`aTransferable` is renamed in the following commit.
Differential Revision: https://phabricator.services.mozilla.com/D90881
Better field packing reduces it from 48 bytes to 40 bytes on 64-bit.
This reduces the memory used by each process by 336 bytes on 64-bit.
Differential Revision: https://phabricator.services.mozilla.com/D90344
Among other things, there were some misuses of std::forward, and
GenericErrorResult was (presumably accidentally) instatiated with
references as the template argument type, e.g. const nsresult&,
which circumvented the check for not calling it with NS_OK in
ResultExtensions.h
Differential Revision: https://phabricator.services.mozilla.com/D90561
Among other things, there were some misuses of std::forward, and
GenericErrorResult was (presumably accidentally) instatiated with
references as the template argument type, e.g. const nsresult&,
which circumvented the check for not calling it with NS_OK in
ResultExtensions.h
Differential Revision: https://phabricator.services.mozilla.com/D90561
Oddly, it checks whether it deletes at least one visible thing after deleting
each content from the DOM tree. It should be done before deleting from the
DOM tree because all text nodes become visible if they are not in the DOM tree
anymore.
Unfortunately, this change does not fix any automated test result, but the
base logic --only when it does not delete any visible things, join the adjacent
block elements-- sounds reasonable. Therefore, let's take this change.
Note that without this change, cannot compute "affected ranges" of
`getTargetRanges()` in the case running this method later.
Differential Revision: https://phabricator.services.mozilla.com/D90065
Although we use CSS property for Inline table editing UI, we use edit
transaction for it unfortunately. When hiding this UI, since removing
element doesn't use edit transaction, transaction will be canceled before
showing this UI.
It is unnecessary to use edit transaction for UI, so we shouldn't use it.
Differential Revision: https://phabricator.services.mozilla.com/D90056
This is causes taking back bugs in
`WSRunScanner::GetNewInvisibleLeadingWhiteSpaceRangeIfSplittingAt()` and
`WSRunScanner::GetNewInvisibleTrailingWhiteSpaceRangeIfSplittingAt()` (I.e.,
they were fixed by bug 1647556). Therefore, this removes the odd `if` blocks
which are pointed with `XXX` comment for avoiding "regressions" (without them,
some WPTs for `InputEvents.getTargetRanges()` become failure).
Depends on D89582
Differential Revision: https://phabricator.services.mozilla.com/D89869
Masayuki Nakano
Butkovits Atila
Jonathan Kew
Makoto Kato
Brindusan Cristian
Csoregi Natalia
Emilio Cobos Álvarez
Kagami Sascha Rosylight
Tom Schuster
Henri Sivonen
Daniel Holbert
Andi-Bogdan Postelnicu
Simon Giesecke
Mirko Brodesser
Andrew Halberstadt
tobar2018
Ricky Stewart
Edgar Chen
Bogdan Tara
Mihai Alexandru Michis
Razvan Maries
Dorel Luca
Magnus Melin
Narcis Beleuzu
Timothy Nikkel
Nicholas Nethercote
Sylvestre Ledru
longsonr