1529205 - DateTimePickerParent shouldn't rely on gBrowser and gBrowser.popupAnchor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Magnus Melin [:mkmelin]]

This is part of bug 1527615.

showPicker(aBrowser, aData) {
let rect = aData.rect;
let type = aData.type;
let detail = aData.detail;

this._anchor = aBrowser.ownerGlobal.gBrowser.popupAnchor;

...

While it may be possible to work around this for Thunderbird somehow(?), it still looks like a bug. The pickers should just work in any document.
Code for the pickers is correctly in toolkit/ and should not rely on internal features from browser/base/content/tabbrowser.js

Comment 1

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

:hsinyi is making triage decisions on this component so I'm unsetting priority on this (see https://mozilla.github.io/bug-handling/triage-bugzilla).

If this is related to the XUL removal work, you may want to ask :bgrimsted about it.

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks Emma.
Yes, I think it'd be great to have Brian chime in on de-xul part for this feature.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Magnus Melin [:mkmelin] from comment #0)

While it may be possible to work around this for Thunderbird somehow(?)

Worst case, I could imagine TB stubbing out a gBrowser object with popupAnchor onto whichever window needs the popup. but I agree it wouldn't be ideal.

The pickers should just work in any document.
Code for the pickers is correctly in toolkit/ and should not rely on internal features from browser/base/content/tabbrowser.js

Sure, making this work without directly accessing gBrowser seems reasonable if it's not too much work, and would also allow access for non-browser-window top level windows in Firefox (although I'm not aware of any needs for that). I'm not sure if there's a typical way to handle this - perhaps just check for gBrowser existence and if it's not there then provide some default popup anchor node? Here are some other references to gBrowser in toolkit: https://searchfox.org/mozilla-central/search?q=gBrowser&path=toolkit

Anyway, this line goes back to Bug 1283384 so I'll forward this to Mike to get an opinion on if this is a change we'd want to take. If so, then perhaps you could make the patch, Magnus?

Comment 4

[Magnus Melin [:mkmelin]]

(In reply to Brian Grinstead [:bgrins] from comment #3)

Here are some other references to gBrowser
in toolkit: https://searchfox.org/mozilla-central/search?q=gBrowser&path=toolkit

There are some, but they are usually protected by a check for existence. (+ a few other cases likely broken)

Anyway, this line goes back to Bug 1283384 so I'll forward this to Mike to
get an opinion on if this is a change we'd want to take. If so, then perhaps
you could make the patch, Magnus?

Sure, I'd appreciate if someone has ideas on how to fix it best though. It looks fairly involved, but then again, perhaps there's a simple fix.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

I think I've seen a variety of different techniques for solving problems like this, including:

1. Window-global functions like getBrowser() that XUL applications can implement to expose gBrowser-like things
2. Fallbacks after attempting to access gBrowser, as described in comment 3 and comment 4.

Another thing we might try is to expose a function on nsBrowserAccess - an nsBrowserAccess instance is something that we set on the window global for both Firefox and Thunderbird... we could have nsBrowserAccess expose a popupAnchor getter.

That'd involve adding some XPIDL though. I can't say I feel too strongly either way, to be honest.

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1529205 - Remove DateTimePickerParent's dependency on gBrowser; r=mconley

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1949f8e9204
Remove DateTimePickerParent's dependency on gBrowser; r=mconley

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/f1949f8e9204

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 9072940 [details]
Bug 1529205 - Remove DateTimePickerParent's dependency on gBrowser; r=mconley

Beta/Release Uplift Approval Request

• User impact if declined: <input type="date"> will continue to be unusable in Thunderbird 68.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very little depends on this (date picker pop-ups and form validation pop-ups), and I've checked that it all still works.
• String changes made/needed:

Comment 10

[Julien Cristau [:jcristau]]

I think it's too late for this in 68, sorry.

Comment 11

[Jorg K (CEST = GMT+2)]

I'll take it on a Thunderbird branch then.

Comment 12

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-beta/rev/db4f8b58bf16e6737ac31e115a9bb4f8e4b68945 on THUNDERBIRD_68_VERBRANCH

Comment 13

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/31ea7a5211b05eb55a9341e8205ab6cfe75b7583 on THUNDERBIRD_68_VERBRANCH

Comment 14

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/924efe646715052154f12ae0e82e5018ffcc9fe7 on THUNDERBIRD_68_VERBRANCH for >= 68.1

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/a2b4204edd360face0b27e0b7d8b06827ff5f2bd - Follow-up, white-space tweak.