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
It'll fail to join them when its utility methods actually join them.
However, it should be considered earlier timing because the preparation
DOM tree changes before joining them causes creating odd tree (see the
removed cases of the test). Therefore, we should make them stop handling
deletion before modifying the DOM tree.
Depends on D89579
Differential Revision: https://phabricator.services.mozilla.com/D89580
This makes `AutoInclusiveAncestorBlockElementsJoiner` stores whether `Run()`
will return "ignored" or not when `Prepare()` is called. And this patch adds
`NS_ASSERTION()` to compare the result of `Run()` and the guess.
Note that this shouldn't used for considering whether its user call or not
`Run()` actually for the risk of regressions and if we do it with current
implementation, some web apps may be broken if they do modify the DOM tree
at white-space adjustment before joining the left and right block elements.
The method will be used by `ComputeTargetRanges()` which will be implemented
by bug 1658702.
Depends on D89578
Differential Revision: https://phabricator.services.mozilla.com/D89579
When its `Run()` join the block elements, even if it won't move any content
nodes in first line, it may notify the callers as "handled" when there is a
preceding invisible `<br>` element since it needs to delete it in any cases.
Therefore, whether there is or not a preceding invisible `<br>` element is
important to compute target ranges since if there is one, `Run()` won't return
"ignored" and the callers won't delete leaf content instead.
Note that the new assertions are not hit in any tests, but due to searching
preceding invisible `<br>` element in `Prepare()`, expected assertion
count of 2 tests is increased.
Depends on D89577
Differential Revision: https://phabricator.services.mozilla.com/D89578
The relation is important when the class handles both joining them and computing
target ranges. Additionally, it's required to consider whether it can join the
block elements. So, it should store the relation when `Prepare()` is called.
Depends on D89576
Differential Revision: https://phabricator.services.mozilla.com/D89577
Unfortunately, `HTMLEditor::MoveOneHardLineContents()`,
`HTMLEditor::MoveChildrenWithTransaction()` and
`HTMLEditor::MoveNodeOrChildrenWithTransaction()` return strict result whether
at least one node is moved or not. Therefore, we need to scan the DOM tree
whether there is at least one content node which can be moved by them for
computing target ranges.
We cannot do same thing for ``HTMLEditor::MoveOneHardLineContents()` because
it split parent elements first and use `ContentSubtreeIterator` which lists
up topmost nodes which are completely in the range, but we need to compute
target ranges without splitting nodes at the range boundaries. Therefore,
this patch checks whether the line containing specified point has content
except invisible `<br>` element.
The others are simple. We can use same logic with them.
Finally, this adds `NS_ASSERTION()`s to check whether the computation is done
correctly at running any automated tests on debug build, and I don't see any
failure with them.
Depends on D89575
Differential Revision: https://phabricator.services.mozilla.com/D89576
When they return error, neither "handled" nor "canceled" state is referred.
So, for making the code simpler, we can make them return
`EditActionResult(NS_ERROR_*)` instead of `EditActionIgnored(NS_ERROR_*)`.
Depends on D89574
Differential Revision: https://phabricator.services.mozilla.com/D89575
The 2 of them always mark the edit action as "handled", but it's not important
when it's "canceled" or an error. Therefore, we can make the users simpler
as this patch does.
Depends on D89573
Differential Revision: https://phabricator.services.mozilla.com/D89574
Now, `AutoInclusiveAncestorBlockElementsJoiner::Run()` is wrapped by a
small block in every caller. Therefore, `AutoTrackDOMPoint` for it can
be moved into the small blocks.
Depends on D89571
Differential Revision: https://phabricator.services.mozilla.com/D89572
For making the refactoring patch simpler, `Prepare()` considers the
`EditActionResult` value of its callers instead. However, this is odd
so that it just return whether the caller should keep working with
it or not as `bool` result. Then, for the additional information, whether
the caller should consume the edit action handling or not, this patch
adds a new method, `CanJoinBlocks()`.
Depends on D89440
Differential Revision: https://phabricator.services.mozilla.com/D89571
Doing rAF rAF flushApzRepaints is not enough to make sure that any potential scroll events are sent to content. The reason is that if the apz repaint request causes us to do scrolling on the main thread then the scrolling will be finished after flushApzRepaints, and the scroll event will be pending, but it's not sent until the next refresh driver tick. So we need to do at least one rAF after flushApzRepaints.
Differential Revision: https://phabricator.services.mozilla.com/D90035
I think the scrolls that zoom to focus input causes are giving us scroll events that we don't expect. I don't think there is a better way around this.
Differential Revision: https://phabricator.services.mozilla.com/D89993
The test fixes all fell into the follow categories:
A) The test uses requestAnimationFrame to wait one frame and expects scrolling to be complete. With the desktop zooming scrollbars in order for the scrolling to show up on the main thread we need to send the scroll request to the compositor and then hear back from it via an apz repaint request (apz callback helper). Waiting on requestAnimationFrame will complete the first part, but not necessarily the second part. The fix is to wait for a scroll event.
B) Switching tests to wait for scroll events exposes another problem: the test can do things that cause a scroll in order to setup the test (and that may not be obvious that it causes a scroll) before actually proceeding to do the test and do something that causes a scroll and then checks for the scroll change of the second thing. Waiting for a requestAnimationFrame would include both those scrolls without desktop zooming scrollbars, but if we wait for a scroll event we will get the scroll event for the first thing which we are not interested in. So we need to make sure scroll events are cleared out before waiting for any scroll events. We do this by waiting two requestAnimationFrame's and waiting for apz to be flushed. We also use this when a test does something and it wants to test that scrolling is not performed.
The main thing that causes scrolling that may not be obvious: calling node.focus(). With stacks like:
from test_scroll_per_page.html
```
#01: mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, mozilla::ScrollOrigin) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d6cc0]
#02: mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, mozilla::ScrollOrigin, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d7732]
#03: mozilla::layout::ScrollAnchorContainer::ApplyAdjustments() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4742913]
#04: mozilla::PresShell::FlushPendingScrollAnchorAdjustments() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4650069]
#05: mozilla::PresShell::ProcessReflowCommands(bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x465742b]
#06: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4656af8]
#07: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1a87d3c]
#08: mozilla::PresShell::ScrollContentIntoView(nsIContent*, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4652b96]
#09: nsFocusManager::ScrollIntoView(mozilla::PresShell*, nsIContent*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1bedd1c]
#10: nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, bool, nsIContent*) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be6be0]
#11: nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be212f]
#12: nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be32ba]
#13: mozilla::dom::Element::Focus(mozilla::dom::FocusOptions const&, mozilla::dom::CallerType, mozilla::ErrorResult&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1aaf283]
#14: mozilla::dom::HTMLElement_Binding::focus(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x2d65f3b]
```
from editor/libeditor/tests/test_bug549262.html
```
#01: mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, mozilla::ScrollOrigin) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d6cc0]
#02: mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, mozilla::ScrollOrigin, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x47d7732]
#03: mozilla::PresShell::ScrollFrameRectIntoView(nsIFrame*, nsRect const&, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x46541bc]
#04: mozilla::PresShell::DoScrollContentIntoView() [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4653776]
#05: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4656b11]
#06: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1a87d3c]
#07: mozilla::PresShell::ScrollContentIntoView(nsIContent*, mozilla::ScrollAxis, mozilla::ScrollAxis, mozilla::ScrollFlags) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x4652b96]
#08: nsFocusManager::ScrollIntoView(mozilla::PresShell*, nsIContent*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1bedd1c]
#09: nsFocusManager::Focus(nsPIDOMWindowOuter*, mozilla::dom::Element*, unsigned int, bool, bool, bool, bool, bool, nsIContent*) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be6be0]
#10: nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be212f]
#11: nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1be32ba]
#12: mozilla::dom::Element::Focus(mozilla::dom::FocusOptions const&, mozilla::dom::CallerType, mozilla::ErrorResult&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x1aaf283]
#13: mozilla::dom::HTMLElement_Binding::focus(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [/Users/tim/ffopt2/src/obj-x86_64-apple-darwin19.6.0/toolkit/library/build/XUL + 0x2d65f3b]
```
C) Several tests use nsIDOMWindowUtils advanceTimeAndRefresh/restoreNormalRefresh and expect scrolling to be done after a call to advanceTimeAndRefresh. This is basically A), advanceTimeAndRefresh does a refresh driver tick but doesn't allow a repaint request to come back to the main thread.
Differential Revision: https://phabricator.services.mozilla.com/D89403
See the bug for the complications that made me write this slightly hacky
fix... Other solutions definitely welcome.
Add a test, adjusted so it would fail without the change.
Differential Revision: https://phabricator.services.mozilla.com/D89835
`RangeBoundaryBase` stores a previous sibling of child node at offset with
`mRef`. Therefore, even if the callers check whether its instance points a
child node, `mRef` may be `nullptr` when it points first child of its container.
So, `GetNextSiblingOfChildAtOffset()` needs to handle the case.
This patch adds the crash case test into
`test_dom_input_event_on_htmleditor.html` because of a basic behavior.
For now, this patch adds 2 chunks which are coded with using same style as
previous ones. However, the test should be redesigned later for making
non-dependency of each chunk guaranteed. (The new chunks are completely
independent from previously running tests.)
Differential Revision: https://phabricator.services.mozilla.com/D89440
CLOSED TREE
Backed out changeset d3d67293f115 (bug 1623413)
Backed out changeset 75ed1b8a5c67 (bug 1623413)
Backed out changeset 0eef32d6a454 (bug 1623413)
The assertion added in part 77 isn't violated, but it needs exhaustive
analysis to ensure it's indeed intended to not be violated.
Differential Revision: https://phabricator.services.mozilla.com/D88258
This change is corresponding to the part:
https://searchfox.org/mozilla-central/rev/73a14f1b367948faa571ed2fe5d7eb29460787c1/editor/libeditor/HTMLEditSubActionHandler.cpp#3143-3155
When caret is not adjacent the deleting character in bidi text, we may do
nothing except putting caret to the character. So, `ComputeRangesToDelete()`
shouldn't update the caret position since the caret position will be check
by its `Run()` later if `beforeinput` event is not canceled. For avoiding
the code duplication, this patch reimplements
`EditorBase::SetCaretBidiLevelForDeletion()` as a stack only class and
split the check and updating part from correcting the data.
Note that by the default pref, the new tests failed since it won't be
canceled, and the method still don't compute for deleting a character.
Differential Revision: https://phabricator.services.mozilla.com/D88378
This patch implements computation of target ranges for this part:
https://searchfox.org/mozilla-central/rev/73a14f1b367948faa571ed2fe5d7eb29460787c1/editor/libeditor/HTMLEditSubActionHandler.cpp#3099-3141
This patch adds some utility methods for computing the ranges. Currently,
it's not yet standardized, but the other browser engines look for leaf content
of another block when blocks are joined (or a block is deleted like this case).
Therefore, we follow the behavior basically, but different from the other
browsers, we should include invisible white-spaces into the range when they
are included. That avoids the invisible white-spaces become visible when
web apps do something instead of us. Note that utility methods have the code,
but this patch does not use it because in this case, we just delete a empty
block ancestor, not join it with previous/next block.
Differential Revision: https://phabricator.services.mozilla.com/D88377
This patch implements computation of target ranges for this part:
https://searchfox.org/mozilla-central/rev/73a14f1b367948faa571ed2fe5d7eb29460787c1/editor/libeditor/HTMLEditSubActionHandler.cpp#3099-3141
This patch adds some utility methods for computing the ranges. Currently,
it's not yet standardized, but the other browser engines look for leaf content
of another block when blocks are joined (or a block is deleted like this case).
Therefore, we follow the behavior basically, but different from the other
browsers, we should include invisible white-spaces into the range when they
are included. That avoids the invisible white-spaces become visible when
web apps do something instead of us. Note that utility methods have the code,
but this patch does not use it because in this case, we just delete a empty
block ancestor, not join it with previous/next block.
Differential Revision: https://phabricator.services.mozilla.com/D88377
In strictly speaking, we should use same computed target ranges for any edit
actions which causes removing non-collapsed selection. However, for now,
this patch makes only `DeleteSelectionAsAction()` because it's not so important
differences for shipping `beforeinput` in Nightly channel.
Differential Revision: https://phabricator.services.mozilla.com/D88376
The editor modules does QI too many times when it sets or removes some style
with `execCommand` or XPCOM API. Therefore, there should be an API to
retrieve `nsStyledElement` pointer from `nsINode*`.
Differential Revision: https://phabricator.services.mozilla.com/D87990
Although it starts to return error if it causes destroying the editor, but
it should not occur because it modifies new and orphan node and it shouldn't
kick any mutation event listeners. Therefore, this patch makes the callers
handle error as-is rather than ignoring errors except
`NS_ERROR_EDITOR_DESTROYED`.
Differential Revision: https://phabricator.services.mozilla.com/D87989
It should take `nsStyledElement&` instead of `const Element&`. Then, it won't
fail and will just return the result of `nsStyleElement::Style()`. Therefore,
we can get rid of it.
Note that this patch makes all its callers keep using strong pointer because
I'm not sure whether the layout APIs which are called with them are safe or
not.
Differential Revision: https://phabricator.services.mozilla.com/D87982
When it returns `EditActionIgnored()`,
`AutoDeleteRangesHandler::HandleDeleteAroundCollapsedRanges()` creates another
`AutoDeleteRangesHandler` instance to delete leaf content node in another
block element. However, this dependency makes developers understand the
behavior harder. Therefore, we should make the method do it instead.
Differential Revision: https://phabricator.services.mozilla.com/D87439
For avoiding infinite recursive calls, `AutoDeleteRangesHandler` returns
`EditActionIgnored()` when it needs to fall it back to
`DeleteRangesWithTransaction()` after modifying the DOM tree or the ranges
to delete. However, this makes developers understand the code harder.
And now, with making constructor of `AutoDeleteRangesHandler` manage the
recursive call state, we can make `HandleDeleteSelection()` stop handling
the fallback path.
Differential Revision: https://phabricator.services.mozilla.com/D87438
Even though their method names in stack trace become too long, but we can
guarantee that they are used only at handling deletion.
Differential Revision: https://phabricator.services.mozilla.com/D87433
This patch just moves them into the new stack only class and make each of them
take `HTMLEditor&` as their first argument.
Differential Revision: https://phabricator.services.mozilla.com/D87432
If `AutoSetTemporaryAncestorLimiter` sets ancestor limiter of the `Selection`,
existing range which is already cached by `AutoRangeArray` may be changed
into the new limiter.
Therefore, this patch makes `AutoSetTemporaryAncestorLimiter` take
`AutoRangeArray` optionally and reset it only when it sets new limiter for
the performance.
Differential Revision: https://phabricator.services.mozilla.com/D87820
The root cause of this bug is a bug of async `ScrollSelectionIntoView` that
is what it won't be canceled even if web app changes scroll position with
any API. Fixing it is really risky and this bug affects an existing website.
Therefore, this patch makes the constructors of `AutoPlaceholderBatch` take
whether do or do not `ScrollSelectionIntoView` when it's destructed. And
makes `HTMLEditor::SetInlinePropertyAsAction()` not request
`ScrollSelectionIntoView` for taking back the traditional behavior.
Note that this patch does not make it an optional argument because it's hard to
guess that it'll run `ScrollSelectionIntoView` automatically.
Differential Revision: https://phabricator.services.mozilla.com/D87819
Even though it hasn't normalize white-spaces before invisible `<br>` element,
it needs to do it for making them visible. Therefore, we should make it
use the new method in this case too.
Depends on D87030
Differential Revision: https://phabricator.services.mozilla.com/D87031
Although it does not need to join text nodes around the `<br>` element since
its previous node is `<hr>`, it can use
`WhiteSpaceVisibilityKeeper::DeleteContentNodeAndJoinTextNodesAroundIt()` too.
Depends on D87029
Differential Revision: https://phabricator.services.mozilla.com/D87030
It works with the traditional white-space normalizer. Therefore, it should
be moved into `WhiteSpaceVisibilityKeeper`.
Depends on D86910
Differential Revision: https://phabricator.services.mozilla.com/D87029
This patch changes the behavior in the following 2 points:
* When both content is in same block element, won't return error
* When `<hr>` element has block children accidentally, this solves its container
The former case must not change actual behavior because
`AutoBlockElementsJoiner` is used with content nodes which are in different
blocks.
Differential Revision: https://phabricator.services.mozilla.com/D86886
Now all users of the method is methods of `AutoBlockElementsJoiner`.
Therefore, we can move it into the nested class. And for splitting the
method in the following patch, it should be a nested class of
`AutoBlockElementJoiner`.
Depends on D86791
Differential Revision: https://phabricator.services.mozilla.com/D86881
And now, `HTMLEditor::JoinNodesDeepWithTransaction()` is used only by
`AutoBlockElementsJoiner`. Therefore, this patch moves it to the
stack only class.
Differential Revision: https://phabricator.services.mozilla.com/D86789
For making the following review easier, this patch just moves some part of
`HandleDeleteNonCollapsedRanges()` into `AutoBlockElementJoiner`.
Differential Revision: https://phabricator.services.mozilla.com/D86786
Now, only when deleting table cell contents, `HandleDeleteSelectionInternal()`
depends on `Selection`. However, this can be moved to `HandleDeleteSelection()`
because recursive callers expects `Selection` is collapsed by its previous job.
Differential Revision: https://phabricator.services.mozilla.com/D86183
Kagami Sascha Rosylight
Mirko Brodesser
Masayuki Nakano
Andrew Halberstadt
tobar2018
Makoto Kato
Ricky Stewart
Edgar Chen
Bogdan Tara
Henri Sivonen
Mihai Alexandru Michis
Razvan Maries
Dorel Luca
Magnus Melin
Narcis Beleuzu
Simon Giesecke
Timothy Nikkel
Nicholas Nethercote
Sylvestre Ledru
longsonr
Emilio Cobos Álvarez
Kagami Sascha Rosylight
Jonathan Kew
Aaron Klotz
Chris Fronk
Sonia Singla