1277906 - HTML Tooltip keeps the focus after being hidden

Status: VERIFIED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Julian Descottes [:jdescottes]]

Regression from Bug 1276876, which I landed on fx-team earlier today.

STRs:
- open data:text/html,<div style="font-family: 'Comic Sans MS'">Inspect me</div>
- inspect the "Inspect me" element
- select the rule-view if not already selected
- focus an element (the "Filter styles" input for instance)
- mouseover the font-family value to display the preview tooltip
- mouseout the font-family value to hide the tooltip

ER: The focus should still be in the Filter styles input
AR: Focus is gone. Also keyboard shortcuts no longer work.

Right now the HTML Tooltip is using autofocus by default, but is not restoring focus after being hidden. This bug should be used to address two issues:
- HTMLTooltip should use autofocus only if opt-in
- if autofocus is on the previously focused element should be focused again when the tooltip is hidden

Comment 1

[Julian Descottes [:jdescottes]]

Taking the bug.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1277906 - part1: change HTMLTooltip default autofocus value to false;

Given the current usage of the HTMLTooltip, I think having the autofocus as
an opt-in behavior makes more sense.

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1277906 - part2: focus previous active element when hiding HTML Tooltip;

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

Comment 4

[Julian Descottes [:jdescottes]]

Brian: While working on this bug I realized I needed to update the "autofocus" test and logic for Bug 1266450.
However I want to land this fix first, because the focus-stealing can become annoying pretty quickly.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8759799 [details]
Bug 1277906 - part1: change HTMLTooltip default autofocus value to false;

https://reviewboard.mozilla.org/r/57670/#review54526

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8759800 [details]
Bug 1277906 - part2: focus previous active element when hiding HTML Tooltip;

https://reviewboard.mozilla.org/r/57672/#review54524

See comments.  Feel free to land part 1 and break this out into another bug if you don't have time to address this before the merge day this weekend.

::: devtools/client/shared/widgets/HTMLTooltip.js:194
(Diff revision 1)
>        }
>  
>        this.container.classList.add("tooltip-visible");
>  
>        this.attachEventsTimer = this.doc.defaultView.setTimeout(() => {
> +        this._focusedElement = this.doc.activeElement;

Seems like should only get set if this.autofocus is true, otherwise there's a chance it'll be changing focus when the tooltip hides, when it hasn't been stolen

::: devtools/client/shared/widgets/HTMLTooltip.js:216
(Diff revision 1)
>      if (this.isVisible()) {
>        this.topWindow.removeEventListener("click", this._onClick, true);
>        this.container.classList.remove("tooltip-visible");
>        this.emit("hidden");
> +
> +      if (this._focusedElement) {

I think we should also check to see if the tooltip is currently focused before restoring.  LIke if Element A is focused, tooltip is shown, then Element B gets focused before the tooltip is hidden, then the tooltip is hidden, then focus would get restored to Element A.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8759800 [details]
Bug 1277906 - part2: focus previous active element when hiding HTML Tooltip;

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

Comment 8

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/57672/#review54524

Thanks for the reviews! I will land part1 to be safe.

> Seems like should only get set if this.autofocus is true, otherwise there's a chance it'll be changing focus when the tooltip hides, when it hasn't been stolen

I think that with the second modification you suggested (restore focus only if active element is in tooltip), it's actually better to keep this outside of the if(autofocus).

Imagine you have a tooltip with autofocus:false ; you interact with the tooltip and focus an element inside it. When you close the tooltip, it would still be nice to have the focus back somewhere usable. What do you think?

> I think we should also check to see if the tooltip is currently focused before restoring.  LIke if Element A is focused, tooltip is shown, then Element B gets focused before the tooltip is hidden, then the tooltip is hidden, then focus would get restored to Element A.

Good point, I agree! I didn't think about this use case initially because the tooltips are supposed to close themselves on an outisde click, but it's true that a focus could still occur. Updated the test case to check this as well.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8759800 [details]
Bug 1277906 - part2: focus previous active element when hiding HTML Tooltip;

https://reviewboard.mozilla.org/r/57672/#review54570

::: devtools/client/shared/test/browser_html_tooltip-03.js:93
(Diff revision 2)
> +  is(getFocusedDocument(doc), doc, "Focus is back in the XUL document");
>  
>    yield hideTooltip(tooltip);
> +  is(getFocusedDocument(doc), doc, "Focus is still in the XUL document");
> +
> +  ok(isAncestor(doc.querySelector("#box3-input"), doc.activeElement), "test");

Can do doc.activeElement.closest("#box3-input") instead of adding the helper function

Also, the assertion string needs updating

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8759799 [details]
Bug 1277906 - part1: change HTMLTooltip default autofocus value to false;

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

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8759800 [details]
Bug 1277906 - part2: focus previous active element when hiding HTML Tooltip;

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

Comment 12

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/57672/#review54570

> Can do doc.activeElement.closest("#box3-input") instead of adding the helper function
> 
> Also, the assertion string needs updating

Indeed, I was trying to use contains() without success, but closest() works fine here, thanks a lot for the suggestion. And sorry about the temporary commit message :(

Comment 13

[Julian Descottes [:jdescottes]]

Try is green (https://treeherder.mozilla.org/#/jobs?repo=try&revision=5428191e513b), landing.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a770e8c521f7
part1: change HTMLTooltip default autofocus value to false;r=bgrins
https://hg.mozilla.org/integration/fx-team/rev/c2778bbb1938
part2: focus previous active element when hiding HTML Tooltip;r=bgrins

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a770e8c521f7
https://hg.mozilla.org/mozilla-central/rev/c2778bbb1938

Comment 16

[Paul Silaghi, QA [:pauly]]

Reproduced on fx-team 49.0a1 (2016-06-03) Win 7.
Verified fixed Nightly 49.0a1 (2016-06-06).

Comment 17

[Paul Silaghi, QA [:pauly]]

Also verified fixed on OS X 10.11, Ubuntu 14.04.