Migrate <tooltip> away from XBL

RESOLVED FIXED in Firefox 64

Status

()

P5
normal
RESOLVED FIXED
10 months ago
4 months ago

People

(Reporter: bgrins, Assigned: bdahl)

Tracking

(Blocks: 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 months ago
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

10 months ago
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.
(Reporter)

Comment 2

10 months ago
(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.
(Reporter)

Comment 3

8 months ago
MozReview-Commit-ID: 7dtkC0XO0JQ
(Reporter)

Comment 4

8 months ago
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.
Flags: needinfo?(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...
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.
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.
Flags: needinfo?(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?
We should not support custom elements in NAC.
Looking at the patch hints that it all should be just plain C++.
(Reporter)

Comment 11

8 months ago
(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++.
(Reporter)

Updated

7 months ago
Assignee: nobody → bdahl
Status: NEW → ASSIGNED
Summary: Migrate <tooltip> to a Custom Element → Migrate <tooltip> away from XBL
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
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
(Reporter)

Updated

7 months ago
See Also: → bug 1486716
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
(Reporter)

Updated

7 months ago
Attachment #8998981 - Attachment is obsolete: true
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?
Flags: needinfo?(bugs)
yeah, I think so.
Flags: needinfo?(bugs)
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.
Attachment #9006650 - Flags: review+
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.
Attachment #9006650 - Flags: review+

Comment 20

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

Comment 21

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d5a7c965683
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1497544
(Reporter)

Updated

6 months ago
Blocks: 1497601

Updated

4 months ago
Depends on: 1509576
You need to log in before you can comment on or make changes to this bug.