Closed
Bug 1277906
Opened 8 years ago
Closed 8 years ago
HTML Tooltip keeps the focus after being hidden
Categories
(DevTools :: Shared Components, defect, P1)
DevTools
Shared Components
Tracking
(firefox49 verified)
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
Assignee | ||
Comment 1•8 years ago
|
||
Taking the bug.
Assignee: nobody → jdescottes
Blocks: devtools-html-1, 1276876
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [devtools-html] [triage]
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/57672/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57672/
Assignee | ||
Comment 4•8 years ago
|
||
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•8 years ago
|
||
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 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
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/
Assignee | ||
Comment 11•8 years ago
|
||
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/
Assignee | ||
Comment 12•8 years ago
|
||
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 :(
Assignee | ||
Comment 13•8 years ago
|
||
Try is green (https://treeherder.mozilla.org/#/jobs?repo=try&revision=5428191e513b), landing.
Comment 14•8 years ago
|
||
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
Updated•8 years ago
|
Iteration: --- → 49.3 - Jun 6
Flags: qe-verify+
QA Contact: alexandra.lucinet
Whiteboard: [devtools-html] [triage] → [devtools-html]
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a770e8c521f7
https://hg.mozilla.org/mozilla-central/rev/c2778bbb1938
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment 16•8 years ago
|
||
Reproduced on fx-team 49.0a1 (2016-06-03) Win 7.
Verified fixed Nightly 49.0a1 (2016-06-06).
Status: RESOLVED → VERIFIED
Comment 17•8 years ago
|
||
Also verified fixed on OS X 10.11, Ubuntu 14.04.
Whiteboard: [devtools-html] → [devtools-html], [qe-dthtml]
Updated•8 years ago
|
QA Whiteboard: [qe-dthtml]
Flags: qe-verify+
Whiteboard: [devtools-html], [qe-dthtml] → [devtools-html]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•