1384606 - Assertion failure: !newEditingHost->IsInNativeAnonymousSubtree(), at /home/worker/workspace/build/src/dom/base/Selection.cpp:3740

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 20170726-e8400551c2e3.

Assertion failure: !newEditingHost->IsInNativeAnonymousSubtree(), at /home/worker/workspace/build/src/dom/base/Selection.cpp:3740

ASAN:DEADLYSIGNAL
=================================================================
==29714==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f71f79f9c4b bp 0x7fffab425bd0 sp 0x7fffab425920 T0)
==29714==The signal is caused by a WRITE memory access.
==29714==Hint: address points to the zero page.
    #0 0x7f71f79f9c4a in mozilla::dom::Selection::NotifySelectionListeners() /home/worker/workspace/build/src/dom/base/Selection.cpp:3742:45
    #1 0x7f71fb226227 in nsFrameSelection::NotifySelectionListeners(mozilla::SelectionType) /home/worker/workspace/build/src/layout/generic/nsFrameSelection.cpp:2067:23
    #2 0x7f71f79f4211 in mozilla::dom::Selection::Collapse(nsINode&, unsigned int, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Selection.cpp:2581:28
    #3 0x7f71f79f4be5 in mozilla::dom::Selection::CollapseToEnd(mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Selection.cpp:2682:3
    #4 0x7f71f79f4d5d in mozilla::dom::Selection::CollapseToEndJS(mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Selection.cpp:2653:3
    #5 0x7f71f85c86d6 in mozilla::dom::SelectionBinding::collapseToEnd(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Selection*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/SelectionBinding.cpp:549:9
    #6 0x7f71f928134e in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3060:13
    #7 0x7f71fe1ae6b1 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #8 0x7f71fe1ae25d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:469:16
    #9 0x7f71fe1af0f5 in InternalCall(JSContext*, js::AnyInvokeArgs const&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:514:12
    #10 0x7f71fe1a3b55 in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3064:18
    #11 0x7f71fe18f7b1 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:409:12
    #12 0x7f71fe1b0d32 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:698:15
    #13 0x7f71fe1b18c2 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:730:12
    #14 0x7f71fea1a64f in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) /home/worker/workspace/build/src/js/src/jsapi.cpp:4637:12
    #15 0x7f71fea1aeb6 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /home/worker/workspace/build/src/js/src/jsapi.cpp:4656:12
    #16 0x7f71fea1aa9e in JS_ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:4677:12
    #17 0x7f71f7bc706b in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /home/worker/workspace/build/src/dom/base/nsJSUtils.cpp:265:8
    #18 0x7f71fa97233f in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /home/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2191:25
    #19 0x7f71fa96f1f0 in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /home/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1775:10
    #20 0x7f71fa95d5e1 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /home/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1471:10

Comment 1

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: e36088242facca249d718e6d79a6cfa7007fd42b
INFO: First bad revision: f5da859b433fe40803847ad77cc4a573fbf8dc25
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e36088242facca249d718e6d79a6cfa7007fd42b&tochange=f5da859b433fe40803847ad77cc4a573fbf8dc25

Regressed by bug 1365092.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Kirk, can you take a look?  If you want me to do it, please let me know!

Comment 3

[Robin Steuber (they/them) [:bytesized]]

I believe I have tracked down the source of the problem.

Previous behavior:
    |nsGenericHTMLElement::SetContentEditable| indirectly invokes the (virtual) function |nsGenericHTMLElement::SetAttr|.
    |nsGenericHTMLElement::SetAttr| invokes |Element::SetAttr|.
    |Element::SetAttr| creates a |mozAutoDocUpdate| object on the stack.
    |Element::SetAttr| returns, destroying the |mozAutoDocUpdate| object.
    |nsGenericHTMLElement::SetAttr| continues execution, calling |ChangeEditableState(1);|.
    This results in |nsHTMLDocument::mContentEditableCount| being changed from 0 to 1.

New behavior:
    |nsGenericHTMLElement::SetContentEditable| indirectly invokes the function |Element::SetAttr|
    |Element::SetAttr| creates a |mozAutoDocUpdate| object on the stack.
    |Element::SetAttr| calls |Element::SetAttrAndNotify|, which calls the (virtual) function |nsGenericHTMLElement::AfterSetAttr|.
    |nsGenericHTMLElement::AfterSetAttr| calls |ChangeEditableState(1);|.
    This results in |nsHTMLDocument::mContentEditableCount| being changed from 0 to 1.
    |Element::SetAttr| returns, destroying the |mozAutoDocUpdate| object.

|mozAutoDocUpdate::~mozAutoDocUpdate| indirectly calls |nsHTMLDocument::MaybeEditingStateChanged|. This function has no effect unless |(mContentEditableCount > 0) != IsEditingOn()| [1].

Because the new behavior changes |mContentEditableCount| before |mozAutoDocUpdate::~mozAutoDocUpdate| is called (rather than after), this function now has a completely different effect. It now results in |Selection::AddItem| being called.

When examining the previous behavior, I see that |Selection::CollapseToEnd| returns early (here [2]). But with the new behavior, it does not return there, ultimately resulting in the assertion failure described.

I am still looking into what the correct solution to this problem might be, but I wanted to post an update on my progress.

[1] http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/html/nsHTMLDocument.cpp#2490
[2] http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/base/Selection.cpp#2763

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Modified testcase that hits the same assert without the patches in bug 1365092

This testcase hits the assert on revision e36088242fac, which is before Kirk's patches.

On the original testcase in this bug, before Kirk's patch, we do ~mozAutoDocUpdate before ChangeEditableState(1) happens, so it doesn't turn on editing mode.  Then we land in nsHTMLDocument::ChangeContentEditableCount, which effectively sync-calls DeferredContentEditableCountChangeEvent::Run, which calls nsHTMLDocument::DeferredContentEditableCountChange.  That bails out early because mParser is false, so we don't turn on editing mode.  Then the selection bits don't see an editable area there, and don't end up in whatever weird state triggers the assert.

In the modified testcase, I simply trigger _another_ ~mozAutoDocUpdate after all of that stuff.  This time the ~mozAutoDocUpdate sees a nonzero mContentEditableCount, does EditingStateChanged(), etc.

All that Kirk's patch did was make the _first_ ~mozAutoDocUpdate trigger EditingStateChanged(); it's not the real culprit here.

Kirk and I looked at this for a bit, and we don't really understand the state handling in this code.  DeferredContentEditableCountChange checks mParser (and relies on EndLoad to turn on editing state), but none of the other callers of EditingStateChanged check mParser.  What are the invariants we're trying to maintain here, exactly?  Peter, I think you have blame for all this stuff, and Ehsan is out so you're our best bet here....

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Another modified testcase that triggers the assert pre-bug-1365092

This is just like trigger.html, but all the code runs onload instead of during parsing, so mParser is null in DeferredContentEditableCountChange and we go ahead and update editing state right when the contenteditable attr is set.

Fundamentally, I expect we're ending up with the selection inside the <input> but it's asserting it's in a contenteditable area or so...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Another modified testcase that triggers the assert pre-bug-1365092

Er, forgot to save after uncommenting...

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Kirk and I looked at this for a bit, and we don't really understand the
> state handling in this code.  DeferredContentEditableCountChange checks
> mParser (and relies on EndLoad to turn on editing state), but none of the
> other callers of EditingStateChanged check mParser.  What are the invariants
> we're trying to maintain here, exactly?  Peter, I think you have blame for
> all this stuff, and Ehsan is out so you're our best bet here....

Note that it's been 10 years since I last worked on this, and there's been quite a bit of changes to this code since then that I don't understand yet either.

IIRC there were two main callers to EditingStateChanged, for contentEditable and for designMode.

For contentEditable we delayed turning on editing during page load, IIRC because of concerns for performance. That's the check for mParser that's in EndLoad and used to be in ChangeContentEditableCount (looks like that's now in DeferredContentEditableCountChange). We should probably have added it to the code in EndUpdate (now in MaybeEditingStateChanged) for bug 395340.

The other callers were for designMode, when the property was set and document.open on a document that was already in designMode. Those just didn't have the mParser checks, because it seemed unnecessary. I don't know about other callers that were added later (BeginLoad?).

I'm fine with trying to remove the mParser check if that helps, it'll just turn on editing on the first contentEditable element that's added during pageload. I think the main change will be that we'll be running more spellchecking code in pages with multiple contentEditable elments, but that might be fine.

Comment 8

[Andrew Overholt [:overholt]]

Maybe Masayuki, Makoto, or Henri know something here (I'm just guessing; feel free to just clear the needinfo and tell me I owe you a beverage some time ;)?

Comment 9

[Robin Steuber (they/them) [:bytesized]]

As mentioned above, the patch that I wrote did not cause the underlying problem here. I do not really have any knowledge specific to the area that needs to be fixed here. I am therefore unassigning myself from this bug. If I can be of any further help though, feel free to needinfo me.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK.  So my second testcase shows that all this stuff about mParser and when editing gets turned on is a red herring, apart from the code being super-confusing and complicated.  I filed bug 1410504 on that part.

The real issue here is that we end up in a state where part of the document is editable, we do a caret move, the caret ends up in the <input>, I would guess, and then selection asserts we're not in anon content, but of course we are.  So this is basically a question about what our caret behavior should be in this situation...

Comment 11

[Henri Sivonen (:hsivonen)]

(In reply to Andrew Overholt [:overholt] from comment #8)
> Maybe Masayuki, Makoto, or Henri know something here (I'm just guessing;

This is outside my area of expertise.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I still don't understand this bug. We should hit the assertion only when contenteditable element is in anonymous subtree. If the assertion's condition is wrong (not enough), let me know.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> We should hit the assertion only when contenteditable element is in anonymous subtree.

We're hitting the assertion when newEditingHost is the anonymous <div> inside an <input>.  It's definitely in an anonymous subtree, and it is very much _NOT_ contenteditable in the sense of the HTML attribute.

It does have NODE_IS_EDITABLE, because it's the editing host for the input's editor.

Now another node in the document _is_ contenteditable, which is why Selection::NotifySelectionListener is entering the relevant block at all (document is not editable as a whole, but there is an HTML editor, for the contenteditable node that's there).   And Selection::GetCommonEditingHostForAllRanges just returns the <div> there, because the selection is inside the <input>.

focusedElement is null in this case, by the way.

Comment 14

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 15

[Makoto Kato [:m_kato]]

Ah, GetCommonEditingHostForAllRanges is only called with contenteditable, but it shouldn't return anonymous node.  I make sense it.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't reproduce the assertion failure in debug build. Did we fix that Selection::Modify result may cross native anonymous subtree boundaries?

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

On the other hand, I think that we should guarantee that nsIContent::GetEditingHost() won't return <foo contenteditable> when the content is in a native anonymous subtree because currently, we don't have such feature and returning editing host with crossing native anonymous subtree boundary is not necessary (and I think it's odd).

Comment 19

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Another more recent test case.

Comment 20

[Tyson Smith [:tsmith]]

Masayuki would a Pernosco session be helpful?

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, if there is, please. Otherwise, I'll try to debug with the debugger.

Thank you for the reproducible testcase.

Comment 22

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/D-Y9tq8T_guvcicls3S3ig/index.html

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you!

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1384606 - part 1: Make `nsIFrame::GetFrameFromDirection` treat frames in different native anonymous subtree not selectable r=emilio!

It's called by PeekOffsetFor* and GetPrevNextBidiLevels, so it's used for
considering whether to put caret or move a selection range boundary.
Therefore, it should treat nodes which can be managed by Selection as
selectable. In theory, even if a native anonymous subtree does not have
an independent selection, its content nodes should not be the container of
the selection range boundaries of selection outside the subtree since
Selection API shouldn't expose nodes in native anonymous subtrees. Therefore,
it can simply treat content nodes in different anonymous subtrees are not
selectable.

Note that it's not standardized that how Selection.modify works with various
content nodes.
https://w3c.github.io/selection-api/#dom-selection-modify

And also Chrome cannot cross generated content like form controls with this API.
This could cause web-compat issues, but it does not make sense for caret
navigation, and anyway out of scope of this bug. Therefore, this patch just
adds the crash test.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1384606 - part 2: Add automated tests for caret browsing around form controls r=emilio!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3c0fd32af16
Make `nsIFrame::GetFrameFromDirection` treat frames in different native anonymous subtree not selectable r=emilio
https://hg.mozilla.org/integration/autoland/rev/bfc8cf25efac
Add automated tests for caret browsing around form controls r=emilio

Comment 27

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/38973 for changes under testing/web-platform/tests

Comment 28

[Norisz Fay [:noriszfay]]

Backed out for causing failures on test_atcaretoffset.html

Backout link

Push with failures

Failure log

Comment 29

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Really really odd... Indeed, test_atcaretoffset.html fails in my environment too. However, even if I add MOZ_CRASH in the added path, the test runs. I.e., it does not run into the new path.

On the other hand, test_wordboundary.html passes completely in my environment... So, I have no idea why they fail in CI.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Odd, it fails only in the mode of "a11y-no-cache-1proc". Sounds like that its created a11y tree and the others' ones are different.

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

When it fails, I verified that the new path runs within this call stack in a child process:
#01: NS_DebugBreak (xpcom\base\nsDebugImpl.cpp:492)
#02: nsIFrame::GetFrameFromDirection (layout\generic\nsIFrame.cpp:9611)
#03: nsIFrame::PeekOffsetForWord (layout\generic\nsIFrame.cpp:9046)
#04: nsIFrame::PeekOffset (layout\generic\nsIFrame.cpp:9336)
#05: mozilla::a11y::HyperTextAccessible::FindOffset (accessible\generic\HyperTextAccessible.cpp:546)
#06: mozilla::a11y::HyperTextAccessible::TextBeforeOffset (accessible\generic\HyperTextAccessible.cpp:988)
#07: mozilla::a11y::xpcAccessibleHyperText::GetTextBeforeOffset (accessible\xpcom\xpcAccessibleHyperText.cpp:82)

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The scan start frame is considered in HyperTextAccessible::FindOffset, however, with the patch, childFrame->GetChildFrameContainingOffset in the frame won't return text frame in <textarea> anymore.

According to the other conditions' result of a11y tests, a11y module wants to traverse frames across native anonymous subtree boundaries.

James: Is it what a11y module wants? Or just aligned for the behavior of nsIFrame::PeekOffset? For the other users of nsIFrame::PeekOffset, it won't cross the native anonymous subtree boundaries from parent to child. Therefore, I try to do it. However, a11y module keeps wanting to cross the boundaries, I'll create an option for it.

Comment 34

[James Teh [:Jamie]]

First, the reason this only fails in the no-cache test variant is that the new a11y caching architecture has a completely different text implementation that doesn't rely on PeekOffset and is more aligned with what a11y needs. The timing here is tricky. On one hand, we aim to ship this new architecture very soon. It's unfortunately too risky to just ship it immediately, but we're about to launch a release experiment and we'll hopefully ship to all users on release a few weeks after that if all goes well. On the other hand, waiting for that would delay your patch by several weeks at minimum.

Second, what a11y wants here is to navigate from a given offset in a text node to get the next/previous word, line, etc. I'm not quite clear on what childFrame is in childFrame->GetChildFrameContainingOffset in the textarea case. Is childFrame not the primary text frame? We could probably just tweak the a11y code to start with the right frame in this special case.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to James Teh [:Jamie] from comment #34)

Second, what a11y wants here is to navigate from a given offset in a text node to get the next/previous word, line, etc. I'm not quite clear on what childFrame is in childFrame->GetChildFrameContainingOffset in the textarea case. Is childFrame not the primary text frame? We could probably just tweak the a11y code to start with the right frame in this special case.

Yeah, the failure means that the childFrame is a frame outside the TextControlFrame of <textarea> or the TextControlFrame itself. I'll check which frame is the childFrame after lunch.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, odd.

 0:16.05 PASS getTextBeforeOffset (word start): wrong end offset(got '0', expected: '0'), offset: -2, id:  'textarea' ;
 0:16.07 GECKO(6496) childFrame: 0000027828813940 (000002782880FA80, #text.div.textarea['textarea'].pre['test'].body.html.#document)
 0:16.07 GECKO(6496) frameAtOffset: 0000027828813940 (000002782880FA80 #text.div.textarea['textarea'].pre['test'].body.html.#document)
 0:16.07 GECKO(6496) aFrame: 0000000000000000 (000002782880F200 #text.pre['test'].body.html.#document)
 0:16.07 GECKO(6496) this: 0000027828813940 (000002782880FA80 #text.div.textarea['textarea'].pre['test'].body.html.#document)
 0:16.07 GECKO(6496) aFrame: 0000000000000000 (000002782880F200 #text.pre['test'].body.html.#document)
 0:16.07 GECKO(6496) this: 0000027828813940 (000002782880FA80 #text.div.textarea['textarea'].pre['test'].body.html.#document)
 0:16.07 GECKO(6496) aFrame: 0000000000000000 (0000027828813EE0 p['display'].body.html.#document)
 0:16.07 GECKO(6496) this: 0000027828813940 (000002782880FA80 #text.div.textarea['textarea'].pre['test'].body.html.#document)
 0:16.07 GECKO(6496) aFrame: 0000000000000000 (000002782880FE80 #text.a.body.html.#document)
 0:16.07 GECKO(6496) this: 0000027828813940 (000002782880FA80 #text.div.textarea['textarea'].pre['test'].body.html.#document)
 0:16.07 GECKO(6496) childFrame: 0000027828813940 (000002782880FA80, #text.div.textarea['textarea'].pre['test'].body.html.#document)
 0:16.07 GECKO(6496) frameAtOffset: 0000027828813940 (000002782880FA80 #text.div.textarea['textarea'].pre['test'].body.html.#document)
 0:16.06 FAIL getTextBeforeOffset (word end): wrong text (got '
', expected: ''), offset: -2, id:  'textarea' ;

The first address is the anonymous subtree root of the content of the frame. Next address is the content of the frame. And the last path is the content of the frame's path. aFrame and this are in the IsSelectable at here. So it fails to scan a word in <textarea> starting from the previous text node in <a>.

If I back it out my change, I don't see the scan starting from #text.a.body.html.#document. Therefore, my change changes the previous result of the failing test... It seems that the simplest fix is, making nsPeekOffsetStruct take new option to keep traversing frames in native anonymous subtrees.

Comment 37

[James Teh [:Jamie]]

:( That is unfortunate given how close a11y is to not needing PeekOffset at all. I guess it's not so hard to remove that option later though.

Comment 38

[James Teh [:Jamie]]

I put a note in bug 1821951 comment 1 to remove this PeekOffset flag once a11y doesn't need it.

Comment 39

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yeah, and I don't like a lot of bools in nsPeekOffsetStruct and in the handlers. Therefore, I'll rewrite them with an EnumSet. Then, only the endpoints will need to be removed.

Comment 40

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1384606 - part 3: Make `nsPeekOffsetStruct` and its handlers treat `bool` options with an `EnumSet` r=emilio!

The constructor of nsPeekOffsetStruct and nsIFrame::GetFrameFromDirection
take too many bool arguments. Therefore, adding new bool arguments does
not make sense. Now, we have a useful mozilla:EnumSet class to treat them
with an enum class. Therefore, let's change nsPeekOffsetStruct with it.

Depends on D172347

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1384606 - part 4: Make `nsIFrame::GetFrameFromDirection` allow to return content in different native anonymous subtree if the caller wants r=emilio!

The a11y module wants to traverse frames in native anonymous subtrees.
Therefore, this patch adds new option for allowing it, makes
nsIFrame::GetFrameFromDirection check it before comparing native anonymous
subtree root nodes, and makes HyperTextAccessible::FindOffset use the
option.

Depends on D172758

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f3a38b1eac88
part 1: Make `nsIFrame::GetFrameFromDirection` treat frames in different native anonymous subtree not selectable r=emilio
https://hg.mozilla.org/integration/autoland/rev/22099c5efaf6
part 2: Add automated tests for caret browsing around form controls r=emilio
https://hg.mozilla.org/integration/autoland/rev/0782f42b2a99
part 3: Make `nsPeekOffsetStruct` and its handlers treat `bool` options with an `EnumSet` r=emilio
https://hg.mozilla.org/integration/autoland/rev/917f487fdf0f
part 4: Make `nsIFrame::GetFrameFromDirection` allow to return content in different native anonymous subtree if the caller wants r=emilio

Comment 43

[Noemi Erli[:noemi_erli]]

Backed out 4 changesets (Bug 1384606) for causing build bustage in nsFrameSelection.h CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=fL82H4oDSYu-0IovB-4HhA.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=917f487fdf0f39f6e12445f8a54d3fef5dbc00a0

Log: https://treeherder.mozilla.org/logviewer?job_id=409240439&repo=autoland&lineNumber=7018

Backout: https://hg.mozilla.org/integration/autoland/rev/7a93650998b38d6a5797322b868da6153407d268

Comment 44

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, sorry. I didn't update part.4 for the latest part.3.

Comment 45

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39b202962261
part 1: Make `nsIFrame::GetFrameFromDirection` treat frames in different native anonymous subtree not selectable r=emilio
https://hg.mozilla.org/integration/autoland/rev/eb8a5705b2d3
part 2: Add automated tests for caret browsing around form controls r=emilio
https://hg.mozilla.org/integration/autoland/rev/19c51e735059
part 3: Make `nsPeekOffsetStruct` and its handlers treat `bool` options with an `EnumSet` r=emilio
https://hg.mozilla.org/integration/autoland/rev/6f7176c0d6a3
part 4: Make `nsIFrame::GetFrameFromDirection` allow to return content in different native anonymous subtree if the caller wants r=emilio

Comment 47

[Iulian Moraru]

Backed out for causing mochitest browser a11y failures on browser_text_basics.js.

• Push with failures

• Failure log

• Backout link

[task 2023-03-17T07:43:47.528Z] 07:43:47     INFO - TEST-PASS | accessible/tests/browser/mac/browser_text_basics.js | attributed text matches non-attributed text - 
[task 2023-03-17T07:43:47.528Z] 07:43:47     INFO - Buffered messages finished
[task 2023-03-17T07:43:47.529Z] 07:43:47     INFO - TEST-UNEXPECTED-FAIL | accessible/tests/browser/mac/browser_text_basics.js | At index 282: right word matches - Got " deceived you, ", expected " "
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - Stack trace:
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochikit/content/browser-test.js:test_is:1512
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochitests/content/browser/accessible/tests/browser/mac/browser_text_basics.js:testRangeAtMarker:9
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochitests/content/browser/accessible/tests/browser/mac/browser_text_basics.js:testWords:70
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochitests/content/browser/accessible/tests/browser/mac/browser_text_basics.js:testMarkerIntegrity:168
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochitests/content/browser/accessible/tests/browser/mac/browser_text_basics.js:null:272
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js:accessibleTask/</<:557
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - resource://testing-common/BrowserTestUtils.sys.mjs:withNewTab:154
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochitests/content/browser/accessible/tests/browser/shared-head.js:accessibleTask/<:476
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochikit/content/browser-test.js:handleTask:1039
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochikit/content/browser-test.js:_runTaskBasedTest:1111
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1253
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1028
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/<:1053
[task 2023-03-17T07:43:47.530Z] 07:43:47     INFO - TEST-PASS | accessible/tests/browser/mac/browser_text_basics.js | attributed text matches non-attributed text - 

Comment 48

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm. The known intemittent failue hid it from my eyes sorry.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, HyperTextAccessibleWrap.mm is the implementation of what I changed in part.4...

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1da78b470683
part 1: Make `nsIFrame::GetFrameFromDirection` treat frames in different native anonymous subtree not selectable r=emilio
https://hg.mozilla.org/integration/autoland/rev/4e426b984cbc
part 2: Add automated tests for caret browsing around form controls r=emilio
https://hg.mozilla.org/integration/autoland/rev/38940e315386
part 3: Make `nsPeekOffsetStruct` and its handlers treat `bool` options with an `EnumSet` r=emilio
https://hg.mozilla.org/integration/autoland/rev/9bf85e2170e0
part 4: Make `nsIFrame::GetFrameFromDirection` allow to return content in different native anonymous subtree if the caller wants r=emilio

Comment 52

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/1da78b470683
https://hg.mozilla.org/mozilla-central/rev/4e426b984cbc
https://hg.mozilla.org/mozilla-central/rev/38940e315386
https://hg.mozilla.org/mozilla-central/rev/9bf85e2170e0

Comment 53

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot