Closed Bug 1277906 Opened 4 years ago Closed 4 years ago

HTML Tooltip keeps the focus after being hidden

Categories

(DevTools :: Shared Components, defect, P1)

defect

Tracking

(firefox49 verified)

VERIFIED FIXED
Firefox 49
Iteration:
49.3 - Jun 6
Tracking Status
firefox49 --- verified

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files)

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
Taking the bug.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [devtools-html] [triage]
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/
Attachment #8759799 - Flags: review?(bgrinstead)
Attachment #8759800 - Flags: review?(bgrinstead)
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 on attachment 8759799 [details]
Bug 1277906 - part1: change HTMLTooltip default autofocus value to false;

https://reviewboard.mozilla.org/r/57670/#review54526
Attachment #8759799 - Flags: review?(bgrinstead) → review+
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.
Attachment #8759800 - Flags: review?(bgrinstead)
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/
Attachment #8759800 - Flags: review?(bgrinstead)
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 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
Attachment #8759800 - Flags: review?(bgrinstead) → review+
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 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/
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 :(
Pushed by jdescottes@mozilla.com:
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
Iteration: --- → 49.3 - Jun 6
Flags: qe-verify+
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
https://hg.mozilla.org/mozilla-central/rev/a770e8c521f7
https://hg.mozilla.org/mozilla-central/rev/c2778bbb1938
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Reproduced on fx-team 49.0a1 (2016-06-03) Win 7.
Verified fixed Nightly 49.0a1 (2016-06-06).
Status: RESOLVED → VERIFIED
Also verified fixed on OS X 10.11, Ubuntu 14.04.
Whiteboard: [devtools-html] → [devtools-html], [qe-dthtml]
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
Whiteboard: [devtools-html], [qe-dthtml] → [devtools-html]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.