1461798 - Migrate <tooltip> away from XBL

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Brian Grinstead [:bgrins]]

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).

Comment 1

[Tim Nguyen :ntim]

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.

Comment 2

[Brian Grinstead [:bgrins]]

(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.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1461798 - Migrate <tooltip> to a Custom Element

MozReview-Commit-ID: 7dtkC0XO0JQ

Comment 4

[Brian Grinstead [:bgrins]]

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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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...

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

It fails because of this tooltip NAC element:

  https://searchfox.org/mozilla-central/rev/aff5d4ad5d7fb2919d267cbc23b1d87ae3cf0110/layout/xul/nsDocElementBoxFrame.cpp#108

Comment 8

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 11

[Brian Grinstead [:bgrins]]

(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++.

Comment 12

[Brendan Dahl [:bdahl]]

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

Comment 13

[Jim Mathies [:jimm]]

(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.

Comment 14

[Brendan Dahl [:bdahl]]

Attachment: Bug 1461798 - Migrate <tooltip> to a C++ implementation. r=smaug

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

Comment 15

[Brendan Dahl [:bdahl]]

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?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

yeah, I think so.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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 19

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d5a7c965683
Migrate <tooltip> to a C++ implementation. r=smaug

Comment 21

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2d5a7c965683