Closed Bug 1668931 Opened 1 year ago Closed 11 months ago

After switching back to Options/Preferences from another tab (e.g. 3-pane folder list focus): Ctrl+F in Options tab brings up duplicated find bar - should focus options search field instead

Categories

(Thunderbird :: Preferences, defect, P3)

Tracking

(thunderbird_esr78+ fixed, thunderbird83 fixed)

RESOLVED FIXED
84 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird83 --- fixed

People

(Reporter: thomas8, Assigned: khushil324)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1656531 +++

Variation of Bug #1656531, seen on 78.3.1 (64-bit).

STR

  1. tools > options
  2. switch to 3-pane (ensure focus on folder list)
  3. switch back to Options by clicking on Options tab top
  4. press Ctrl+F, search a word on options page, then press ESC
  5. ...or instead, try cursor-up/down and monitor the 3-pane tab top...

Actual Result:

  • duplicated find bar at bottom of options (I don't drink... ;-) - see screenshot.
  • ironically, pressing ESC on the unwanted findbar will then focus the the real Find in Options, which does the search job much better
  • using cursors, you can actually navigate on the 3-pane tab in background, and delete folders which you aren't seeing with Del. See screencast attachment 9177466 [details] for the account settings equivalent (bug 1666885).

Welcome to the world of "background tab focus" in Thunderbird. It's unfortunate that our tabs are technically not as independent as they might appear to be...

Expected

  • don't keep focus on background tab (for the cursor case with focus on folder list)
  • don't show duplicated findbar (for Ctrl+F)
  • Ctrl+F to focus options search field instead, to allow proper options search
  • If focus was on some setting before, preserve and restore that focus, otherwise focus Find in options search field

Kushil, any ideas (as you fixed the same symptoms in bug 1656531)?

I understand that all the visible tabs are one thing technically, and we're just tweaking things to look and pretend to act independent(ly).
So how do we normally prevent focus from leaking into / being stuck in background tabs?

Can someone explain to me how the find bar gets duplicated anyway (apart from the fact that we don't want to show it here)? Imho, duplicated UI elements in options make us look bad to the very people whom we want to be influencers in favor of Thunderbird, e.g. enterprise admins and power users. Nothing special about pressing Ctrl+F and expecting a functional "Find in options" search.

Assignee: nobody → khushil324
Attachment #9183616 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED

Now, when we switch back to the "Preferences" tab, it focuses on the search input by dafault and when you press Ctrl + F, no find bar will be opened and the search input will be focused. Previosuly, the browser element was not getting focused when we select the "Preferences" tab, so "keydown" events were not detected.

Comment on attachment 9183616 [details] [diff] [review]
Bug-1668931_focus-search-in-preferences-tab-0.patch

Review of attachment 9183616 [details] [diff] [review]:
-----------------------------------------------------------------

Works and I guess improves a bad situation somewhat. r=mkmelin
Attachment #9183616 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 84 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/623111fffff2
Fix after switching back to Preferences from another tab Ctrl+F in Preferences tab brings up duplicated find bar. r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 9184040 [details] [diff] [review]
Bug-1668931_focus-search-in-preferences-tab-1.patch

Review of attachment 9184040 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is right. about:preferences would always have the searchInput,no? If we made sure to wait enough for the document to load. So looking up the id only after you know the url matches like before seems correct. 
Also please use getElementById when it's looking up by id - that's faster.

For try runs: ry: -b o -p linux64,macosx64 -u all doesn't really work like it did earlier: it doesn't run the mac tests so basically pointless for a patch like this. You can either use a try_task_config.json (like https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c54ccaa2a8628d2e7ff16af7a588cbee818e3002) or "-p all" to run tests on all platforms.

For patches that don't touch cpp, use --artifact which will be much faster.

hg qnew -m "try: -b o -p all -u all --artifact" try
hg qnew -m "try: -b o -p all -u all" try
Attachment #9184040 - Flags: review?(mkmelin+mozilla)

Instead of search-input, now we are focusing on the documentElement which is solving the problem.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=60d069a60146ab16d446acf0faa3bc9a45dabfda

Attachment #9184040 - Attachment is obsolete: true
Attachment #9184166 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9184166 [details] [diff] [review]
Bug-1668931_focus-search-in-preferences-tab-2.patch

Review of attachment 9184166 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/specialTabs.js
@@ +487,5 @@
>    showTab(aTab) {
>      aTab.browser.setAttribute("type", "content");
>      aTab.browser.setAttribute("primary", "true");
> +    if (aTab.browser.currentURI.spec.startsWith("about:preferences")) {
> +      aTab.browser.contentDocument.documentElement.focus();

I'm not convinced this is quite right, though it probably fixes the search bar issue for the most part in practice (not focusing the search field though, which we might not need).

The focusing is likely at the wrong spot (where things have not necessarily loaded yet). 
If the tests showed we couldn't rely on the searchInput being there - https://firefoxci.taskcluster-artifacts.net/Tkg9xbDITWq2JjAe-lnPXQ/0/public/logs/live_backing.log - are we sure it's the right contentDocument - and not still the previous one?
Attachment #9184166 - Flags: review?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #10)

The focusing is likely at the wrong spot (where things have not necessarily
loaded yet).

There can be no wrong spot as there will be only one contentDocument and corresponding documentElement in the content tab.

If the tests showed we couldn't rely on the searchInput being there -
https://firefoxci.taskcluster-artifacts.net/Tkg9xbDITWq2JjAe-lnPXQ/0/public/
logs/live_backing.log - are we sure it's the right contentDocument - and not
still the previous one?

There will be no previous one as whenever we open the tab, it will be assigned a contentDocument and corresponding documentElement. For our purpose, it doesn't matter if documentElement has child elements or not. We just need to focus on the documentElement to detect the events. Previously, the focus was not on the documentElement and the global event listeners were working whenever we switch back to the preferences tab. showTab function gets called when re-select the content tab, not when we open the content tab. So in most cases, documentElement is also loaded properly when we re-select the tab(if it's loaded or not, doesn't matter now).

Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 9184166 [details] [diff] [review]
Bug-1668931_focus-search-in-preferences-tab-2.patch

Review of attachment 9184166 [details] [diff] [review]:
-----------------------------------------------------------------

Ok lets take it.
Attachment #9184166 - Flags: review+
Flags: needinfo?(mkmelin+mozilla)

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/a0f9f5f02ea6
Fix after switching back to Preferences from another tab Ctrl+F in Preferences tab brings up duplicated find bar. r=mkmelin DONTBUILD

Status: REOPENED → RESOLVED
Closed: 11 months ago11 months ago
Resolution: --- → FIXED

Comment on attachment 9184166 [details] [diff] [review]
Bug-1668931_focus-search-in-preferences-tab-2.patch

[Approval Request Comment]
Regression caused by (bug #): 1656531
User impact if declined: Search feature in the preferences tab will be affected.
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): Low

Attachment #9184166 - Flags: approval-comm-esr78?
Attachment #9184166 - Flags: approval-comm-beta?

Comment on attachment 9184166 [details] [diff] [review]
Bug-1668931_focus-search-in-preferences-tab-2.patch

[Triage Comment]
Approved for beta

Attachment #9184166 - Flags: approval-comm-beta? → approval-comm-beta+

Comment on attachment 9184166 [details] [diff] [review]
Bug-1668931_focus-search-in-preferences-tab-2.patch

[Triage Comment]
Approved for esr78

Attachment #9184166 - Flags: approval-comm-esr78? → approval-comm-esr78+

Working properly for me in testing the 78.5.0 release candidate on Windows 10 (20H2).

You need to log in before you can comment on or make changes to this bug.