1287090 - While Dev Tools is undocked, the event details tooltip is not dismissed although out of focus

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Ada [:adalucinet]]

[Affected versions]:
- latest Nightly 50.0a1

[Affected platforms]:
- Windows 10 64-bit
- Mac OS X 10.9.5
- Ubuntu 16.04 64-bit

[Steps to reproduce]:
1. Launch Firefox.
2. Open Inspector: Ctrl + Shift + C (for Windows & Ubuntu) or Cmd + Opt + C (for Mac OS X).
3. Click on 'Show in separate window' button.
4. Click on the "ev" bubble next to a node in the markup view.
5. Click outside the event details tooltip.

[Expected result]: Tooltip vanishes.

[Actual result]: Tooltip is visible.

[Regression range]:
- Last good revision: cb493102af8a50946ac601a86cc3d076703faa57
- First bad revision: c9564b7204d452746ee472b8dec3c4d33e876716
- Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cb493102af8a50946ac601a86cc3d076703faa57&tochange=c9564b7204d452746ee472b8dec3c4d33e876716 
- Looks like the following bug has the changes which introduced the regression: https://bugzil.la/1171736

[Additional notes]:
- Screen recording: https://goo.gl/7o1r2S
- RTL builds are also affected.

Comment 1

[Ada [:adalucinet]]

No idea how could bug 1171736 impact this behavior; Justin, what's you opinion on this matter? Thanks in advance!

Comment 2

[Justin Wood (:Callek)]

I sadly can't think of any reason my change could have caused this bug. My code would have only affected localized builds

Comment 3

[Julian Descottes [:jdescottes]]

It is actually a regression from the event details tooltip migration (bug 1266450).
It should be triaged/handled by devtools-html track#1. 

@Marco: Since I won't be able to attend the standup, my opinion is that this bug should be P1 and addressed as soon as possible.

Comment 4

[Marco Mucci [:MarcoM]]

Spoke with Julian, this bug is not related to Track #1 work.

Comment 5

[Patrick Brosset <:pbro>]

Tested on 49.0a2 (2016-07-14): works fine.
Tested on 50.0a1 (2016-07-14): the popup does not close when you click outside. 

Indeed, this happens when the toolbox is undocked, but I found a way to reproduce this with the docked toolbox too, this doesn't seem to always do it but:

- Open a new tab
- Open devtools, then undock the toolbox, and dock it again right after,
- Open the tooltip and try to close it

Julian: could you elaborate in what way this is not related to Track #1?

Comment 6

[Marco Mucci [:MarcoM]]

(In reply to Patrick Brosset <:pbro> from comment #5)
> Tested on 49.0a2 (2016-07-14): works fine.
> Tested on 50.0a1 (2016-07-14): the popup does not close when you click
> outside. 
> 
> Indeed, this happens when the toolbox is undocked, but I found a way to
> reproduce this with the docked toolbox too, this doesn't seem to always do
> it but:
> 
> - Open a new tab
> - Open devtools, then undock the toolbox, and dock it again right after,
> - Open the tooltip and try to close it
> 
> Julian: could you elaborate in what way this is not related to Track #1?

Julian contacted me and bug 1287090 will be dealt with in Track #1.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1287090 - HTMLTooltip: support host changes by retrieving top window dynamically;

Review commit: https://reviewboard.mozilla.org/r/64598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64598/

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8771462 [details]
Bug 1287090 - HTMLTooltip: support host changes by retrieving top window dynamically;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64598/diff/1-2/

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8771462 [details]
Bug 1287090 - HTMLTooltip: support host changes by retrieving top window dynamically;

https://reviewboard.mozilla.org/r/64598/#review61722

::: devtools/client/inspector/markup/test/browser_markup_events-windowed-host.js:24
(Diff revision 2)
> +
> +  yield toolbox.switchHost("bottom");
> +  yield runTests(inspector);
> +
> +  yield toolbox.destroy();
> +  Services.prefs.clearUserPref("devtools.toolbox.host");

This should happen in a registerCleanupFunction call so it happens even if the test fails

::: devtools/client/shared/widgets/HTMLTooltip.js:280
(Diff revision 2)
>    /**
> +   * The top window is used to attach client event listeners in order to close the tooltip
> +   * on click. This has to be dynamically computed in case the toolbox host changes.
> +   */
> +  get topWindow() {
> +    return this.doc.defaultView.top;

There's things that would potentially not get cleaned up if the topWindow changes (the _onClick event listener in particular).  This is a pretty toolbox-specific problem though and the old window is presumably destroyed so that should all be cleaned up by swapFrameLoaders

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8771462 [details]
Bug 1287090 - HTMLTooltip: support host changes by retrieving top window dynamically;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64598/diff/2-3/

Comment 11

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/64598/#review61722

> This should happen in a registerCleanupFunction call so it happens even if the test fails

Good point, done!

> There's things that would potentially not get cleaned up if the topWindow changes (the _onClick event listener in particular).  This is a pretty toolbox-specific problem though and the old window is presumably destroyed so that should all be cleaned up by swapFrameLoaders

I'm not sure how likely this issue is to occur, but I went ahead and changed the implementation so that the topWindow reference is updated when we attach the event listeners only.

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1ae0889810b

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8771462 [details]
Bug 1287090 - HTMLTooltip: support host changes by retrieving top window dynamically;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64598/diff/3-4/

Comment 13

[Julian Descottes [:jdescottes]]

Accidentally moved a skipif=true, with my previous patch. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6c1518ff147

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8771462 [details]
Bug 1287090 - HTMLTooltip: support host changes by retrieving top window dynamically;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64598/diff/4-5/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/65524ee7219a
HTMLTooltip: support host changes by retrieving top window dynamically;r=bgrins

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/65524ee7219a

Comment 17

[Ada [:adalucinet]]

Verified fixed with latest Nightly 50.0a1, under Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.9.5