Once popup-base is removed (Bug 1461793), I think <tooltip> should be a candidate for Custom Element migration: https://searchfox.org/mozilla-central/rev/2b9779c59390ecc47be7a70d99753653d8eb5afc/toolkit/content/widgets/popup.xml#270-381. We'll need to figure out if we need to / how to handle the <children> inside of the content. And also how to handle the xbl:inherits attribute (I think an attributeChangedCallback should work fine).

Are all the features of the tooltip binding even used ? It's a binding that looks simple enough that it might not need a custom element, if it turns out not everything in the binding is used.

(In reply to Tim Nguyen :ntim from comment #1) > Are all the features of the tooltip binding even used ? It's a binding that > looks simple enough that it might not need a custom element, if it turns out > not everything in the binding is used. AFAICT the features (including the integration with nsITooltipTextProvider) are used when [page=true], which is set on aHTMLTooltip.

MozReview-Commit-ID: 7dtkC0XO0JQ

Emilio - I haven't seen this before so I'm not sure if I've done something wrong with the Custom Element here. But if I apply the patch I get the following crash at startup: thread '<unnamed>' panicked at 'Resolving style on unstyled element', libcore/option.rs:960:5 stack backtrace: 0: 0x1176fb833 - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::hed04c7a1477ef1e3 1: 0x1176fe5cb - std::panicking::default_hook::{{closure}}::h0d6700a02d682978 2: 0x1176fd945 - std::panicking::default_hook::h90363c6d6ac04260 3: 0x1176fd3c3 - std::panicking::rust_panic_with_hook::h81c4f3add25e6667 4: 0x1176fd286 - std::panicking::continue_panic_fmt::hfa057b7c1de88179 5: 0x1176ffee8 - rust_begin_unwind 6: 0x11770cc51 - core::panicking::panic_fmt::h4c82aa4e615d52a2 7: 0x117701f78 - core::option::expect_failed::h05d35beb9d2793c7 8: 0x11742dce5 - Servo_ResolveStyle 9: 0x115746d8d - _ZN21nsCSSFrameConstructor20ResolveComputedStyleEP10nsIContent 10: 0x11573ed1f - _ZN21nsCSSFrameConstructor15ProcessChildrenER23nsFrameConstructorStateP10nsIContentPN7mozilla13ComputedStyleEP16nsContainerFramebR12nsFrameItemsbP14PendingBindingP8nsIFrame 11: 0x1157407ad - _ZN21nsCSSFrameConstructor24ConstructDocElementFrameEPN7mozilla3dom7ElementEP21nsILayoutHistoryState 12: 0x11574affa - _ZN21nsCSSFrameConstructor20ContentRangeInsertedEP10nsIContentS1_P21nsILayoutHistoryStateNS_13InsertionKindE 13: 0x115710178 - _ZN7mozilla9PresShell10InitializeEv 14: 0x1153d2fc9 - _ZN7mozilla3dom11XULDocument11StartLayoutEv 15: 0x1153d4576 - _ZN7mozilla3dom11XULDocument11DoneWalkingEv 16: 0x1153d013f - _ZN7mozilla3dom11XULDocument10ResumeWalkEv 17: 0x1153d5090 - _ZN7mozilla3dom11XULDocument23OnScriptCompileCompleteEP8JSScript8nsresult 18: 0x1153d4aa6 - _ZN7mozilla3dom11XULDocument16OnStreamCompleteEP15nsIStreamLoaderP11nsISupports8nsresultjPKh 19: 0x11322f923 - _ZN7mozilla3net14nsStreamLoader13OnStopRequestEP10nsIRequestP11nsISupports8nsresult 20: 0x1131d5883 - _ZN13nsBaseChannel13OnStopRequestEP10nsIRequestP11nsISupports8nsresult 21: 0x1131d594c - _ZThn128_N13nsBaseChannel13OnStopRequestEP10nsIRequestP11nsISupports8nsresult 22: 0x1131f066b - _ZN17nsInputStreamPump11OnStateStopEv 23: 0x1131effd3 - _ZN17nsInputStreamPump18OnInputStreamReadyEP19nsIAsyncInputStream 24: 0x11312a49a - _ZN23nsInputStreamReadyEvent3RunEv 25: 0x113151eeb - _ZN8nsThread16ProcessNextEventEbPb 26: 0x11315005a - _Z23NS_ProcessPendingEventsP9nsIThreadj 27: 0x11551b690 - _ZN14nsBaseAppShell19NativeEventCallbackEv 28: 0x115582557 - _ZN10nsAppShell18ProcessGeckoEventsEPv 29: 0x7fff500d2a10 - __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ 30: 0x7fff5018c42b - __CFRunLoopDoSource0 31: 0x7fff500b546f - __CFRunLoopDoSources0 32: 0x7fff500b48ec - __CFRunLoopRun 33: 0x7fff500b4152 - CFRunLoopRunSpecific 34: 0x7fff4f39ed95 - RunCurrentEventLoopInMode 35: 0x7fff4f39ea0e - ReceiveNextEventCommon 36: 0x7fff4f39e883 - _BlockUntilNextEventMatchingListInModeWithFilter 37: 0x7fff4d64fa72 - _DPSNextEvent 38: 0x7fff4dde5e33 - -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] 39: 0x115581adb - -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] 40: 0x7fff4d644884 - -[NSApplication run] 41: 0x115582c18 - _ZN10nsAppShell3RunEv 42: 0x11678dda8 - _ZN12nsAppStartup3RunEv 43: 0x1168a5d9c - _ZN7XREMain11XRE_mainRunEv 44: 0x1168a676f - _ZN7XREMain8XRE_mainEiPPcRKN7mozilla15BootstrapConfigE 45: 0x1168a6de7 - _Z8XRE_mainiPPcRKN7mozilla15BootstrapConfigE 46: 0x10bba5169 - main Is it expected that we could ever trigger that from a frontend change? I'm guessing there's something weird/special going on with <tooltip>. If I comment out `customElements.define("tooltip", MozTooltip);` the crash goes away (but of course tooltips don't work then). If I empty out the Custom Element class entirely (`customElements.define("tooltip", class MozTooltip extends XULPopupElement { })`) the crash still appears.

That's really weird, I can take a look. Tooltips and popups are special-cased in a bunch of places so it's not ultra-surprising...

Just started looking at this. On debug builds it fails earlier with: Assertion failure: anonymousItems[i].mContent->IsRootOfAnonymousSubtree() (Content should know it's an anonymous subtree), at /home/emilio/src/moz/gecko-2/layout/base/nsCSSFrameConstructor.cpp:10115 Looking into why now.

It fails because of this tooltip NAC element: https://searchfox.org/mozilla-central/rev/aff5d4ad5d7fb2919d267cbc23b1d87ae3cf0110/layout/xul/nsDocElementBoxFrame.cpp#108

We're failing to create that element, because we're hitting: https://searchfox.org/mozilla-central/rev/aff5d4ad5d7fb2919d267cbc23b1d87ae3cf0110/dom/base/nsContentUtils.cpp#10050 The fact that we execute custom element reactions from frame construction is pretty bad btw... :( Then we hit the error case in https://searchfox.org/mozilla-central/rev/aff5d4ad5d7fb2919d267cbc23b1d87ae3cf0110/layout/base/nsCSSFrameConstructor.cpp#4087, but aContents is not empty, so all our state is not set up properly and we crash. There are two bugs here: * nsCSSFrameConstructor not handling failure gracefully in any sort of correct way. * (The most important one): That <tooltip> being constructed there. We need to either not fail (maybe just use the document global in that case?), or avoid creating those NAC elements in nsDocElementBoxFrame. I can fix the first, but not sure that'd get you tooltip support without fixing the second.

So I talked with smaug and it'd be lovely not having to support CE in NAC... Could we implement <tooltip> in C++ maybe?

We should not support custom elements in NAC. Looking at the patch hints that it all should be just plain C++.

(In reply to Olli Pettay [:smaug] from comment #10) > We should not support custom elements in NAC. > Looking at the patch hints that it all should be just plain C++. Looking at the implementation, it does seem viable to implement it in C++.

Hi Jim, After changing the tooltip to a C++ implementation I get a number of test failures on XUL documents with plugins[1] which I think is due to timing changes of XBL loading. It appears all the test files assume a plugin will be loaded before the "load" event, but this is not guaranteed. Nearly all of these test file also have intermittents attached, so I'm assuming this is an issue regardless of my changes. Do you recommend that I change the tests to wait for the plugin to be loaded or change it so async plugin loading blocks the document "load" event[2]? [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d475a0cd54533fce8ec5a8ac127f46a4a189494&selectedJob=194577690 [2] https://hg.mozilla.org/try/rev/86e08d6ff60eb17ff34989eb3c2437533689949d

(In reply to Brendan Dahl [:bdahl] away until 9/4 from comment #12) > Hi Jim, > > After changing the tooltip to a C++ implementation I get a number of test > failures on XUL documents with plugins[1] which I think is due to timing > changes of XBL loading. It appears all the test files assume a plugin will > be loaded before the "load" event, but this is not guaranteed. Nearly all of > these test file also have intermittents attached, so I'm assuming this is an > issue regardless of my changes. > > Do you recommend that I change the tests to wait for the plugin to be loaded > or change it so async plugin loading blocks the document "load" event[2]? > > [1] > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=5d475a0cd54533fce8ec5a8ac127f46a4a189494&selectedJob=1 > 94577690 > [2] https://hg.mozilla.org/try/rev/86e08d6ff60eb17ff34989eb3c2437533689949d Seems good to wait for the plugin to load somehow, imho. Feel free to work up whatever works and still tests that the plugins do load.

Move the implementation of the XBL tooltip to C++ so the element can safely be created during native anonymous content creation. The 'mouseover' and 'mouseout' event handlers were not moved as they appear to be legacy code that is no longer needed. A number of tests started perma-failing after this patch. Most failures were caused by a timing change where plugins sometimes load after the document "load" event. Many of the failures had intermittents associated with them and the tests were not waiting for plugins to load before starting. The test "test_weakmap_keys_preserved2.xul" had a bug where it was possible for it to finish before all the tests were run. MozReview-Commit-ID: LGpo9TO0VRy

Olli, As noted on IRC, when the tooltip gets stuck with this patch you can still mouse over it and it will hide (like it currently does). Can we land this patch and fix the overall broken tooltip behavior in a different bug as that seems orthogonal to this issue?

yeah, I think so.

well, I'd like the patch to use PostHandleEvent and not a separate event listener

Comment on attachment 9006650 [details] Bug 1461798 - Migrate <tooltip> to a C++ implementation. r=smaug Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) has approved the revision.

Comment on attachment 9006650 [details] Bug 1461798 - Migrate <tooltip> to a C++ implementation. r=smaug Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) has requested changes to the revision.

Pushed by bdahl@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2d5a7c965683 Migrate <tooltip> to a C++ implementation. r=smaug