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
When we don't match exactly dictionary name with `lang` attribute, we look for
language name. When looking for language name (ex. en/fr etc), we don't
consider the priority by application/system locale. So if lang=en, it may be
selected by character order. Most case is that en-GB is always selected if it
is installed.
We should respect application/system locale if language name is matched to
current content language before random selection.
Depends on D99336
Differential Revision: https://phabricator.services.mozilla.com/D99337
Actually, our locale service doesn't use UTF-16 for locale name and it will
be ASCII. Although dictionary name of spell checker is locale/language name,
it still uses UTF-16. To use locale API, I should replace it with `ACString`.
Differential Revision: https://phabricator.services.mozilla.com/D99336
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
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
Masayuki Nakano
Makoto Kato
Henri Sivonen
Kartik Gautam
Daniel Holbert
Andi-Bogdan Postelnicu
Cosmin Sabou
Emilio Cobos Álvarez
Dorel Luca
Kagami Sascha Rosylight
Simon Giesecke
Mirko Brodesser
Andrew Halberstadt
tobar2018
Ricky Stewart
Edgar Chen
Bogdan Tara
Mihai Alexandru Michis
Razvan Maries
Magnus Melin
Narcis Beleuzu
Timothy Nikkel
Nicholas Nethercote
Sylvestre Ledru
longsonr