Closed Bug 1287090 Opened 4 years ago Closed 4 years ago

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

Categories

(DevTools :: Inspector, defect, P1)

50 Branch
defect

Tracking

(firefox47 unaffected, firefox48 unaffected, firefox49 unaffected, firefox50 verified)

VERIFIED FIXED
Firefox 50
Iteration:
50.3 - Jul 18
Tracking Status
firefox47 --- unaffected
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- verified

People

(Reporter: adalucinet, Assigned: jdescottes)

References

Details

(Keywords: regression, Whiteboard: [reserve-html])

Attachments

(1 file)

[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.
No idea how could bug 1171736 impact this behavior; Justin, what's you opinion on this matter? Thanks in advance!
QA Whiteboard: [qe-dthtml]
Flags: needinfo?(bugspam.Callek)
Keywords: regression
Whiteboard: [devtools-html][triage]
Blocks: 1171736
I sadly can't think of any reason my change could have caused this bug. My code would have only affected localized builds
Flags: needinfo?(bugspam.Callek)
Whiteboard: [devtools-html][triage]
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.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Whiteboard: [devtools-html] [triage]
Blocks: 1266450
No longer blocks: 1171736
Iteration: --- → 50.3 - Jul 18
Flags: qe-verify+
Priority: -- → P1
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [reserve-html]
Spoke with Julian, this bug is not related to Track #1 work.
Assignee: jdescottes → nobody
No longer blocks: devtools-html-1
Status: ASSIGNED → NEW
Iteration: 50.3 - Jul 18 → ---
Priority: P1 → --
Whiteboard: [reserve-html]
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?
Flags: needinfo?(jdescottes)
(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.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Iteration: --- → 50.3 - Jul 18
Flags: needinfo?(jdescottes)
Priority: -- → P1
Whiteboard: [reserve-html]
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/
Attachment #8771462 - Attachment description: Bug 1287090 - HTMLTooltip: listen to click events on top window only if docked; → Bug 1287090 - HTMLTooltip: support host changes by retrieving top window dynamically;
Attachment #8771462 - Flags: review?(bgrinstead) → review+
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 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/
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 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/
Accidentally moved a skipif=true, with my previous patch. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6c1518ff147
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/
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/65524ee7219a
HTMLTooltip: support host changes by retrieving top window dynamically;r=bgrins
https://hg.mozilla.org/mozilla-central/rev/65524ee7219a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Verified fixed with latest Nightly 50.0a1, under Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.9.5
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.