Last Comment Bug 735292 - IM in TB: Fix wrong focus; improve focus ring and initial focus with no chat accounts set up
: IM in TB: Fix wrong focus; improve focus ring and initial focus with no chat ...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 15.0
Assigned To: Florian Quèze [:florian] [:flo]
:
Mentors:
Depends on:
Blocks: 714733
  Show dependency treegraph
 
Reported: 2012-03-13 09:57 PDT by Thomas D. (currently busy elsewhere; needinfo?me)
Modified: 2012-05-30 04:13 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch (3.02 KB, patch)
2012-04-27 09:23 PDT, Florian Quèze [:florian] [:flo]
bwinton: review+
bwinton: ui‑review-
Details | Diff | Review
Patch v2 (3.73 KB, patch)
2012-05-02 08:35 PDT, Florian Quèze [:florian] [:flo]
bwinton: ui‑review+
mozilla: approval‑comm‑aurora+
Details | Diff | Review

Description Thomas D. (currently busy elsewhere; needinfo?me) 2012-03-13 09:57:04 PDT
+++ This bug was initially created as a clone of Bug #735272 +++

STR

current TB 13 trunk with IM client
without any IM accounts set up yet

1 view random msg in main 3pane
2 press Chat button from Mail toolbar
3a) without any IM accounts set up yet, press Enter (hoping to press that only lonely button in the middle of the screen: "Get started")

3b) alternatively:
having discovered bad behaviour after 3a), try to it right by using <tab> to manually set focus on the button "Get started"

Actual result

3a) surprise: the selected email from main mail tab pops up in a new tab after the chat tab; i.e. although the Chat tab is in the foreground, focus is still on the previous tab (bad, guess all sorts of things could happen there).

3b) the focus ring is really bad for this scenario with no chat accounts set up yet:
1: no initial focus (because of 3a, it's still on another tab)
- here are the following <tab> stops in their current order:
2: nothing (bad)
3: nothing (bad)
4: focus now in "Search all conversations"
5: nothing again (bad)
6: focus now on "Get started" button (wow, that's 5 x <tab> to get to the only useful ui element in this scenario)
6: focus on tab caption

Expected result

3a) after showing Chat tab, ensure the focus is here, too (see 3b)

3b)
- Expected Focus ring with no accounts set up (hence initial screen with "You haven't set up..." info only):
1: initial focus in this scenario (no chat accts yet) should be on "Get Started" button, as that's the only useful/possible option in this scenario
2: Tab caption
3: "Search all convsersations"
then loop back to 1 ("Get Started")
All other tab stops are not enabled at this time and should be skipped in this scenario.
Comment 1 Thomas D. (currently busy elsewhere; needinfo?me) 2012-03-13 09:59:19 PDT
(In reply to Thomas D. from comment #0)
> 6: focus now on "Get started" button (wow, that's 5 x <tab> to get to the
> only useful ui element in this scenario)
> 6: focus on tab caption

Typo: Of course, that's "7: focus on tab caption"
Comment 2 Florian Quèze [:florian] [:flo] 2012-04-27 09:23:49 PDT
Created attachment 619073 [details] [diff] [review]
Patch

Behavior implemented by the attached patch:
When the Chat tab is selected, if there's a placeholder with a button, focus it; otherwise, give focus to the listbox of the left pane.
Comment 3 Blake Winton (:bwinton) (:☕️) 2012-04-30 09:28:29 PDT
Comment on attachment 619073 [details] [diff] [review]
Patch

This fixes part of the bug, but is missing the changes to tab-order, where we don't focus things that don't do anything.

(I also want to see some sort of highlight on the Status button if it's going to be in the tab order.)

Let me know if you need steps-to-reproduce for this, but basically hitting the "tab" key on Windows should fairly clearly show the problem.

>+++ b/mail/components/im/content/chat-messenger-overlay.js
>@@ -503,16 +505,17 @@ var chatHandler = {
>+  _placeHolderButtonId: "",
>@@ -521,19 +524,29 @@ var chatHandler = {
>+    if (connected)
>+      delete this._placeHolderButtonId;

Should this delete the _placeHolderButtonId, or just set it back to ""?

(Also, it's convention in Thunderbird to put braces around the if, if you need braces around the else.)

>+    else {
>+      this._placeHolderButtonId =
>+        hasAccount ? "openIMAccountManagerButton" : "openIMAccountWizardButton";
>+    }

Other than the things I mentioned above, this looks good, so r=me for the code, but ui-r- for the faulty tab behaviour.

Thanks,
Blake.
Comment 4 Florian Quèze [:florian] [:flo] 2012-04-30 14:47:35 PDT
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #3)
> Comment on attachment 619073 [details] [diff] [review]
> Patch
> 
> This fixes part of the bug, but is missing the changes to tab-order, where
> we don't focus things that don't do anything.

I don't see what needs to be done to change the tab-order, or skip some elements. Aren't disabled buttons automatically skipped?

> (I also want to see some sort of highlight on the Status button if it's
> going to be in the tab order.)

Isn't this bug 735658?

> Let me know if you need steps-to-reproduce for this, but basically hitting
> the "tab" key on Windows should fairly clearly show the problem.

That may be helpful as I'm not completely sure I see what you mean.

> >+++ b/mail/components/im/content/chat-messenger-overlay.js
> >@@ -503,16 +505,17 @@ var chatHandler = {
> >+  _placeHolderButtonId: "",
> >@@ -521,19 +524,29 @@ var chatHandler = {
> >+    if (connected)
> >+      delete this._placeHolderButtonId;
> 
> Should this delete the _placeHolderButtonId, or just set it back to ""?

I usually use delete to set back to the default value, to avoid duplicating the default value in the code and risking changing it only in one place in a future code changes. Deleting the properties instead of setting them to their default values should also have a good (but probably insignificant) effect on memory usage.
Comment 5 Florian Quèze [:florian] [:flo] 2012-05-01 07:00:32 PDT
So I tested the behavior of the tab key (with this patch applied), it focuses in this order:
1. the "Show chat status" button (focused by default with this patch applied)
2. The tab toolbar
3. The status selector. I think bug 735658 will theme it so that it's visible when it has focus or not. We may want to disable it when there's no configured account. I haven't done it at the same time as I disabled the other toolbar buttons as it wasn't trivial to do, I explained it in bug 735701 comment 1.
4. The 'show accounts' toolbar button.
5. The search box
6. The listbox of the left pane, which is useful if the "offline contacts" group is not empty: that allows reading logs of previous conversations without connecting accounts. We may want to add a disabled = true attribute there when the offline contacts group is empty, and select the that group by default otherwise to ensure the focus is visible.
7. Back to 1.

Without this patch applied the cycle is the same, but it starts somewhere else, probably at 2.

Blake, could you please be more explicit about which of these changes you require to ui-r the patch here? :-)

Thanks,
Florian
Comment 6 Blake Winton (:bwinton) (:☕️) 2012-05-01 11:22:03 PDT
So, that's not what I see on Windows Aero.

I see
1. "Get Started"
2. Tab toolbar.
3. Status selector.
4. Apparently nothing, but when I hit enter I can enter a custom status message sometimes.
5. search box.
6. Apparently nothing, but moving the arrow keys does something with the left pane.
7. Back to 1.

So, that seems like what you said, but with a couple of twists that make it more confusing.

So, what I want to see changed:
The left-pane needs some indication of when it has focus.
The suggestions you mention for #6.
#3 seems like it'll be handled in a different bug, which I'm okay with.
My #4 is really confusing, and I don't think we can ship with it the way it is.  (I can't get the Show Accounts button to be highlighted, which I think is okay.)

Sound reasonable?
Comment 7 Florian Quèze [:florian] [:flo] 2012-05-02 08:35:42 PDT
Created attachment 620333 [details] [diff] [review]
Patch v2

You are right that the Show account buttons doesn't get focus, I don't know why, it's the textbox part of the status selector that receives focus. I think we will also take care of that in the "polish status selector" bug.

This new patch disables the listbox of the left pane when it's completely empty, and ensures the first element is selected if there was no selection (I'm not completely happy of this behavior as the "offline contact" groups will be selected at the time it's the only thing visible, and when online contacts or conversations will appear, the item selected by default will end up in the middle of the list... but I think that's good enough for now).

You may want to give another look at the code too, as I had to change it to change the UI behavior of course.
Comment 8 Blake Winton (:bwinton) (:☕️) 2012-05-08 06:16:32 PDT
Comment on attachment 620333 [details] [diff] [review]
Patch v2

The code still seems fine, so we can carry my previous r+ over.

And I think the UI is good, at least in terms of the complaints raised in this bug, so ui-r=me, too.  :)

Thanks,
Blake.
Comment 9 Florian Quèze [:florian] [:flo] 2012-05-09 07:36:20 PDT
https://hg.mozilla.org/comm-central/rev/a15efa119675
Comment 10 Florian Quèze [:florian] [:flo] 2012-05-15 06:53:31 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/1fc886aefa10
Comment 11 Florian Quèze [:florian] [:flo] 2012-05-30 04:13:11 PDT
(In reply to Florian Quèze from comment #7)

> You are right that the Show account buttons doesn't get focus, I don't know
> why

Actually, this is bug 739677.

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