1482718 - Contacts Sidebar should have focus after it gets shown

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

Chances are that when the user makes contacts sidebar show up, he actually wants to use it to pick recipients (in Seamonkey, it's actually still address picker dialogue iirc). For that default scenario, it would be helpful if it was already focused (access, ux-efficiency). Otherwise getting focus there via keyboard is somewhat clumsy (non-mnemonic access key, or several Shift+Tab, or several (Shift+)F6). With mouse, we're saving unnecessary navigation and one click.

Attachment pane as a similar use case also gets focus when shown (ux-consistency).

I suggest the best spot to focus inside contacts sidebar is the search field, which is the fastest way of finding the right recipients. Also, both the list of address books and the list of contacts are easy to reach from the search field via keyboard.

Comment 1

[Thomas D. (:thomas8)]

Attachment: Patch V.1: Focus contacts side bar when it gets shown by user.

Essentially simple, but devil in the detail.

Turns out that it takes a split second for the sidebar to load when it hasn't been loaded before (and still unavailable even when sidebar.contentDocument.readyState claims "complete"), so we need to be a bit persistive as we try to set focus on the search input. This works for me now.

Comment 2

[Thomas D. (:thomas8)]

Attachment: Patch V.1.1: (cosmetic changes) Focus contacts side bar when it gets shown by user.

Aceman kindly looked into this in an IRC session. This normalises some "weird" syntax into plain vanilla (how boring! where's the excitement of having loads of brackets of all types mixed and mingled?). Otherwise it's funcionally the same.

Richard, can you test this?

Comment 3

[:aceman]

Comment on attachment 8999466 [details] [diff] [review]
Patch V.1.1: (cosmetic changes) Focus contacts side bar when it gets shown by user.

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

Yes, this includes the improvements we discussed on IRC.
I just now found some small syntax.

Also we need the UI review if this is actually wanted behaviour.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3007,3 @@
>    var sideBarBox = document.getElementById('sidebar-box');
> +  if (sideBarBox.getAttribute("sidebarVisible") == "true") {
> +    // Sidebar is supposed to to visible, so let's ensure it is loaded.

Double "to" ?

@@ +6305,5 @@
>  function focusContactsSidebarSearchInput() {
>    // Caveat: Callers must ensure that contacts side bar is visible.
> +  let peopleSearchInput = sidebarDocumentGetElementById("peopleSearchInput",
> +                                                        "abContactsPanel");
> +  if(peopleSearchInput) {

Space after 'if'.

@@ +6545,5 @@
> +      // as it may take a moment until it's available.
> +      if (aFocus) {
> +        // This little recursive function will try every 100 ms to focus
> +        // peopleSearchInput, but only for maximally 1 second; moving focus
> +        // later than that would be error-prone as this is asynchronous,

Also please add a comment on why this is done this way instead of normally putting the focusing inside an onload="" attribute of the document that is being loaded.
I think it is because we do not always want the focusing (e.g. not when restoring the sidebar on compose open) so we need control of it and do it here.

@@ +6551,5 @@
> +        // typing in body, then late shift of focus might be disruptive.
> +        let tryFocusSearchInput = function(i) {
> +          i++;
> +          let success = focusContactsSidebarSearchInput();
> +          if (i <= 10 && !success) setTimeout(tryFocusSearchInput, 100, i);

Put the setTimeout on the next line.

The "waiting" could also be done using setInterval(), but I don't think it would be any simpler.

Comment 4

[Thomas D. (:thomas8)]

Attachment: Patch V.1.2: (cosmetic changes, comment added) Focus contacts side bar when it gets shown by user.

Addressing review nits

Richard, can you test this pls

Comment 5

[:aceman]

Comment on attachment 8999479 [details] [diff] [review]
Patch V.1.2: (cosmetic changes, comment added) Focus contacts side bar when it gets shown by user.

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

Thanks.

Comment 6

[Richard Marti (:Paenglab)]

Comment on attachment 8999479 [details] [diff] [review]
Patch V.1.2: (cosmetic changes, comment added) Focus contacts side bar when it gets shown by user.

Thanks.

Not a issue by this patch because it's already without it but should need considering in a new bug: When the focus was on the "To" field and I open the contacts pane and decide to not use it and close it again, the focus jumps to the "Subject" field. We should set the focus again to the "To" field when it is empty. When it's filled, it's okay to focus the "Subject" field.

Comment 7

[Thomas D. (:thomas8)]

Thanks, so this is ready to land.

Comment 8

[Jorg K (CEST = GMT+2)]

Umm, is this really the best solution we can come up with? So the contacts sidebar loads asynchronously and we poll it at most 10 times until 1 second has lapsed, right? So what happens if the user is already doing something in a To: field, the subject or the body after 700ms?

If it loads asynchronously is should be possible to do something upon completion.

Comment 9

[Jorg K (CEST = GMT+2)]

Reading comment #3, there's an onload event. Can this distinguish between a user-triggered load and a load that happens on a new compose windows (since it was loaded before)? The key-press that triggers the load could set a variable? Or is that wishful thinking?

Comment 10

[Thomas D. (:thomas8)]

(In reply to Jorg K (GMT+2) from comment #8)
> Umm, is this really the best solution we can come up with?

I'd think so, yes.

> So the contacts sidebar loads asynchronously

Well yeah, in a way. The only time where it explicitly loads asynchronously is when you open a new composition and it reopens the sidebar for you because it was open before. Only that load happens on a 0ms timeout. When you're in composition and you press F9 to show sidebar, we just set the sidebar src attribute to abContactsPanel.xul, so that call looks synchronous afasics. However, de facto it seems to happen asynchronously (browser optimization stuff I guess), that's why we're failing to set focus immediately after changing src. Worse, even when sidebar.contentDocument.readyState reports "complete" (the equivalent of onload), we're still failing to focus, meaning it's either lying to us or I've checked the wrong thing.

(In reply to Jorg K (GMT+2) from comment #9)
> Reading comment #3, there's an onload event. Can this distinguish between a
> user-triggered load and a load that happens on a new compose windows (since
> it was loaded before)? The key-press that triggers the load could set a
> variable? Or is that wishful thinking?

I guess that could be done as I've done such hacks before, but imho it wouldn't help anything.
If we accept that contacts sidebar loads asynchronously always (as described above, and apparently beyond our control), that means that we still can't stop the user from interacting with the UI after having triggered the sidebar to appear.
So from my understanding we can't just wait for onload to complete regardless of the time that takes (maybe more than 1 sec?), and then just move focus after that unpredictable time - as we'd risk interrupting the user who was fast to do something else in the UI. Back to square one, we can only move focus if we're able to do it fast enough within a second or less, and that's exactly what we're doing now (I don't see any advantage of putting the timer into the loading document, that's actually much less transparent).

> and we poll it at most 10 times until 1 second has lapsed, right?

Yes.

> So what happens if the user is already doing something in
> a To: field, the subject or the body after 700ms?

I suggest adding a captcha to prevent robots from operating your Thunderbird... ;-)
Jokes aside, it looks both unlikely and practically impossible for the user to continue interacting with the UI within less than 1 second. The very moment you press F9 or click the menu or toolbar button, that 1 second has already elapsed, and we'll have moved the focus if we can (from my tests albeit with a small regular AB, it only takes 200ms). With mouse you'd first have to click somewhere - definitely your second already gone, we'll have moved focus to sidebar, then you'll just succeed to move it somewhere else again. Furthermore, the underlying assumption of this bug is that from a workflow perspective, it's unlikely that user just wants to show the sidebar for the sake of it and then (within less than a second???) continue with some other spot in the UI. It's much more likely to open the sidebar for the purpose of actually using it to add recipients. Let's say your focus is in To-field, so you could type there right now if you'd wish, but then you've explicitly decided to press F9 to show contacts sidebar - you'll certainly "wait" for sidebar to appear, and it's both unlikely and all but impossible to get your hand back to some other key and start typing in To-field within less then a second.

> If it loads asynchronously is should be possible to do something upon
> completion.

Dito, waiting for completion is unpredictable and might create the very problem of disruption which we're trying to avoid here.

Bottom line, I think what we have here is best under the circumstances.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8999479 [details] [diff] [review]
Patch V.1.2: (cosmetic changes, comment added) Focus contacts side bar when it gets shown by user.

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6527,5 @@
> +  // start, we may also load abContactsPanel, but in that case it shouldn't be
> +  // focused. Focusing from inside this function then requires us to ensure that
> +  // peopleSearchInput is already loaded, so we keep trying in intervals but not
> +  // longer than one second. Checking for sidebar.contentDocument.readyState
> +  // doesn't seem to work, and wouldn't eliminate the need for trying in intervals.

maybe it's just hidden?

@@ +6561,5 @@
> +          let success = focusContactsSidebarSearchInput();
> +          if (i <= 10 && !success)
> +            setTimeout(tryFocusSearchInput, 100, i);
> +        };
> +        tryFocusSearchInput(0); // Start iteration.

I agree with Jörg this isn't acceptable.

Comment 12

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #11)
> Comment on attachment 8999479 [details] [diff] [review]
> Patch V.1.2: (cosmetic changes, comment added) Focus contacts side bar when
> it gets shown by user.
> 
> Review of attachment 8999479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +6527,5 @@
> > +  // start, we may also load abContactsPanel, but in that case it shouldn't be
> > +  // focused. Focusing from inside this function then requires us to ensure that
> > +  // peopleSearchInput is already loaded, so we keep trying in intervals but not
> > +  // longer than one second. Checking for sidebar.contentDocument.readyState
> > +  // doesn't seem to work, and wouldn't eliminate the need for trying in intervals.
> 
> maybe it's just hidden?

What exactly would be hidden? readyState is actally (imo falsely) reporting "complete". From my reading, nothing hidden at this point.

> @@ +6561,5 @@
> > +          let success = focusContactsSidebarSearchInput();
> > +          if (i <= 10 && !success)
> > +            setTimeout(tryFocusSearchInput, 100, i);
> > +        };
> > +        tryFocusSearchInput(0); // Start iteration.
> 
> I agree with Jörg this isn't acceptable.

Magnus, please read my comment 10 where I explain the rationale and prove that there are no dangers here. User has just explicitly triggered the contacts sidebar to show up, most likely with an intention to use it as per comment 0, so it's both all but technically impossible and highly unlikely from a workflow pov that he'll continue at some other spot within less than a second.

We can't blindly rely on onload or increase the timespan, because then we'd actually risk disruptive failures.
We could reduce the timespan, but then we risk failing on the other end, as we also want the auto-focus to succeed reliably under most circumstances.
Please note we'll typically succeed to focus contacts sidebar within 200ms as per my tests (small, regular AB) which will immediately stop the iteration.

Comment 13

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. (currently busy elsewhere) from comment #12)
> What exactly would be hidden? readyState is actally (imo falsely) reporting
> "complete". From my reading, nothing hidden at this point.

The sidebar. readyState can be complete even if the element is hidden. (Though this was just a guess.)

> 
> > @@ +6561,5 @@
> > > +          let success = focusContactsSidebarSearchInput();
> > > +          if (i <= 10 && !success)
> > > +            setTimeout(tryFocusSearchInput, 100, i);
> > > +        };
> > > +        tryFocusSearchInput(0); // Start iteration.
> > 
> > I agree with Jörg this isn't acceptable.
> 
> Magnus, please read my comment 10 where I explain the rationale and prove
> that there are no dangers here. User has just explicitly triggered the
> contacts sidebar to show up, most likely with an intention to use it as per
> comment 0, so it's both all but technically impossible and highly unlikely
> from a workflow pov that he'll continue at some other spot within less than
> a second.

I don't know, it's quite possible he just wanted to show addresses, and then quickly moved on to write in the message body for instance. This is well doable within even half a second.

Anyway, it's very spaghetti code to go checking status on a repeated timeout. Surely there's a way to do it properly, like some event you'd listen to.

Comment 14

[Thomas D. (:thomas8)]

The only way to avoid waiting for sidebar availabilty is to make it always available:

- Always set src of sidebar browser at composition start regardless if sidebar is shown or hidden; would that pre-load the sidebar document even when it's hidden?
- the loading would occur on a timeout of 0secs similar as it does now when re-opened at composition start, so that it does not block/delay loading the composition window
- If user presses F9 or other ways of showing the sidebar, it will already be loaded; so there's no delay and we can always focus immediately.

The disadvantage of that solution is that we'll waste resources both at composition startup time and in terms of memory for users who might not even use the sidebar. Comments in the code say that the current design is deliberate to avoid such.

Comment 15

[Magnus Melin [:mkmelin]]

If there isn't a suitable event you can always trigger one yourself at the proper time. Then listen for the event and act accordingly.

Comment 16

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #13)
> (In reply to Thomas D. (currently busy elsewhere) from comment #12)

> I don't know, it's quite possible he just wanted to show addresses, and then
> quickly moved on to write in the message body for instance.

Quickly, maybe. But not racing-quickly. And even just moving your fingers from F9 back down to the alphabetical keyboard will take time. And then: Why show addresses if not to use them?

> This is well doable within even half a second.

I think even if you'd really try to force it (which is not a real scenario), you'd have a hard time going from pressing F9 to typing in message body within less than a second. And if you don't have some super-slow, super-big LDAP AB, we'll move focus within 200ms so nobody can be fast enough in real-life scenarios to type something before that.

> Anyway, it's very spaghetti code to go checking status on a repeated
> timeout.

There's no spaghetti code here. It's a clean recursion to keep trying for maximally one second (typically much less) after a specific user-triggered action of explicitly showing the sidebar.

> Surely there's a way to do it properly, like some event you'd listen to.

I have already explained why listening to events doesn't work, please read my comment 10 and respond to that.
Otherwise, could you please propose something specific and explain how that's superior and how it avoids the problems mentioned in my comments. Thanks.

Comment 17

[Thomas D. (:thomas8)]

(In reply to Magnus Melin from comment #15)
> If there isn't a suitable event you can always trigger one yourself at the
> proper time. Then listen for the event and act accordingly.

So that's just another way of coding this, but does not change the behaviour in any way, compared to my patch.

The proper time is not more than one second after the user explicitly shows the sidebar. So that would require setting a timer, which is not much different from what we have here. And creating a custom event and a custom listener sounds no less complicated and no better in terms of resources, code complexity and readability than a simple in-place mini loop which typically finishes after only 2 rounds, i.e. within a split second. It only happens when triggered, and only once if sidebar has not been shown before in this composition session, and no resource-intensive operations here, just checking for the presence of <page> element in the sidebar.

Comment 18

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. (currently busy elsewhere) from comment #10)
> we just set the
> sidebar src attribute to abContactsPanel.xul, so that call looks synchronous
> afasics. However, de facto it seems to happen asynchronously (browser
> optimization stuff I guess),
.
Setting src just triggers the load, so it is asynchronous 

> So from my understanding we can't just wait for onload to complete
> regardless of the time that takes (maybe more than 1 sec?), 

Unlikely to take more than a fraction of a second. If it takes longer then likely the whole UI is slow enough not to be useful during the loading period.

> Dito, waiting for completion is unpredictable 

No, reacting to an event would give a predictable result.

Predictable results are superior for obvious reasons. One might say predictability is all and everything any programmer ever wants. That's what it's about: you tell the computer to do something and it does it, not maybe does it.

Comment 19

[Geoff Lankow (:darktrojan)]

Attachment: 1482718-contacts-sidebar.diff

I'd like to make a suggestion in the form of a patch. If loading the sidebar document takes so long, get the document to focus the textbox itself before it does the time-consuming work.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1482718_focusContactsSideBar.patch - folded patch

Thanks, Geoff!

Since we can't land one without the other, I folded the patches and attributed the work to both authors. Hard to tell who wrote what exactly, but hey, we're all a happy team.

This works for me, so I let the reviewers and the original author decide whether they like this approach.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 9000193 [details] [diff] [review]
1482718_focusContactsSideBar.patch - folded patch

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

::: mail/components/addrbook/content/abContactsPanel.js
@@ +127,5 @@
>  
>  var mutationObs = null;
>  
> +function AbPanelLoad() {
> +  if (location.search == "?focus") {

What is .search? I thought it was .query.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6520,5 @@
> +  // Depending on aFocus argument, we're setting focus into peopleSearchInput
> +  // here; we can't use abContactsPanel's onload attribute because at composition
> +  // start, we may also load abContactsPanel, but in that case it shouldn't be
> +  // focused. Focusing from inside this function then requires us to ensure that
> +  // peopleSearchInput is already loaded, so we keep trying in intervals but not

This comment needs adjustment since we're not trying in intervals any more (here and a few lines below).

Comment 22

[Thomas D. (:thomas8)]

Comment on attachment 9000193 [details] [diff] [review]
1482718_focusContactsSideBar.patch - folded patch

Awesome. Very elegant tweak. Does exactly what we want. Thanks, Geoff!

I've updated the patch wrt comments, see my next comment.

Comment 23

[Thomas D. (:thomas8)]

Attachment: Patch V. 2.1: (Geoffs tweak, updated comments) Focus contacts side bar when it gets shown by user.

Updated comments. Nice.

(In reply to Jorg K (GMT+2) from comment #21)
> Comment on attachment 9000193 [details] [diff] [review]
> > +function AbPanelLoad() {
> > +  if (location.search == "?focus") {
> 
> What is .search? I thought it was .query.

According to https://developer.mozilla.org/en-US/docs/Web/API/Location, it's location.search.

> ".search property is a search string, also called a query string..."

One wonders why they didn't make it .query...

Comment 24

[:aceman]

I can't test it right now but how does this work? How can setting the focus in this new way be faster than the original way with wait loop? In the new way you focus the textbox as you already know it is available (from inside the document). In the old way as soon as it appeared it got focus. So yes it is more elegant. But how does it solve the problem to NOT focus the field if opening takes too long? Or is the whole compose window stuck/locked while the AB sidebar is loading (also with all the AB cards displaying) so it actually does not matter? I have to test this with a large AB.
But all the talk so far was that the user can interact with the composition while the AB sidebar is opening.

Comment 25

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #24)
> I can't test it right now but how does this work? How can setting the focus
> in this new way be faster than the original way with wait loop? In the new
> way you focus the textbox as you already know it is available (from inside
> the document). In the old way as soon as it appeared it got focus. So yes it
> is more elegant. But how does it solve the problem to NOT focus the field if
> opening takes too long?

I don't think Geoffs approach solves that problem.

> Or is the whole compose window stuck/locked while
> the AB sidebar is loading (also with all the AB cards displaying) so it
> actually does not matter? I have to test this with a large AB.

Yes, and if possible also with large LDAP, or LDAP which fails to connect.

> But all the talk so far was that the user can interact with the composition
> while the AB sidebar is opening.

Indeed.
I actually got confused with the event used by Geoff, but now I am seeing that this imo reintroduces a potential problem which my approach had already eliminated. We can't change focus if loading takes too long, because user is free to do anything whilst sidebar loads, then we'll disrupt if we change focus.

My approach was fast, safe, and predictable.

Comment 26

[Thomas D. (:thomas8)]

And honestly speaking, whilst the technical solution might be elegant, spreading a simple operation like setting focus accross several files, and sometimes focusing from inside, and sometimes focusing from outside, is not exactly improving maintainability and readability...

Comment 27

[Thomas D. (:thomas8)]

(In reply to Geoff Lankow (:darktrojan) from comment #19)
> Created attachment 9000188 [details] [diff] [review]
> 1482718-contacts-sidebar.diff
> 
> I'd like to make a suggestion in the form of a patch. If loading the sidebar
> document takes so long, get the document to focus the textbox itself before
> it does the time-consuming work.

Geoff, can you please explain your claim that your approach will focus the textbox "before the time-consuming work" of loading the contacts sidebar document? You're using <page onload="..."> event, so that's *after* loading the entire document and all resources, isn't it? But who can predict how long that takes for edge cases of huge or badly/dis-connected ABs?
Imo, we can only move focus within a second or less, otherwise there's risk of interrupting the user who might have started doing something else in the UI.

Comment 28

[Thomas D. (:thomas8)]

Also note that we offer *ALL ADDRESSBOOKS* in contacts sidebar, so that certainly can take an unpredictable amount of time to load... I'd suspect that the searchbox loads earlier, but how to find out?

Comment 29

[Thomas D. (:thomas8)]

My approach includes checking for <page> element, which is not strictly required atm (but helpful for sustainability if we ever want to show other things in the sidebar). But I think it's unlikely that the searchbox is available before the <page> of which it is a child, no?

Comment 30

[Thomas D. (:thomas8)]

Comment on attachment 9000193 [details] [diff] [review]
1482718_focusContactsSideBar.patch - folded patch

So per aceman's comment 24, and my comment 27, I'll have to revert my f+ into f-.

Comment 31

[Thomas D. (:thomas8)]

Comment on attachment 8999479 [details] [diff] [review]
Patch V.1.2: (cosmetic changes, comment added) Focus contacts side bar when it gets shown by user.

So I think this patch is back in the game. It actually has all reviews, and no risks, so still ready to land.

Comment 32

[Richard Marti (:Paenglab)]

Comment on attachment 9001237 [details] [diff] [review]
Patch V. 2.1: (Geoffs tweak, updated comments) Focus contacts side bar when it gets shown by user.

ui-r+ for focusing in the contacts sidebar. I don't have a lot of contacts and it loads fast for me.

Comment 33

[Magnus Melin [:mkmelin]]

You're confusing loading of the panel with the loading of the data in the panel. 

The panel loads (and can take focus) very quickly. Once loaded goes off to get the list of addresses populated but you don't need to wait for that.

Comment 34

[Geoff Lankow (:darktrojan)]

(In reply to Thomas D. (currently busy elsewhere) from comment #27)
> Geoff, can you please explain your claim that your approach will focus the
> textbox "before the time-consuming work" of loading the contacts sidebar
> document? You're using <page onload="..."> event, so that's *after* loading
> the entire document and all resources, isn't it? But who can predict how
> long that takes for edge cases of huge or badly/dis-connected ABs?
> Imo, we can only move focus within a second or less, otherwise there's risk
> of interrupting the user who might have started doing something else in the
> UI.

Event listeners must run in sequence. If you add a listener to wait for the "load" event, it happens so late because it must wait for any listeners added earlier to complete – including the one I've added the focus call to, which *then* does the loading from the address book into the UI. This is the bit which takes a long time.

Comment 35

[Thomas D. (:thomas8)]

Attachment: Patch 2.1 with alert for testing

Aceman, Magnus (comment 33) and Geoff (comment 34) are right that we can set focus at the start of AbPanelLoad() function because it is only *after that* in the same function that the AB gets populated, which is what may take longer. I've added an alert which exposes the UI state right after focusing: Search box is focused, but AB view is blank (see code below).

Aceman, please test this patch, and, if you're satisfied, you could just r+ Patch V. 2.1. Tia.

Maybe we would still need to check what happens if user has many AB's like 100 or 1000 (unlikely but possible), because from my understanding the list of AB's is already populated *before* we focus.

The fact remains that my first patch has the same net effect to focus the search input asap, but it's safer because if for whatever reason loading the searchbox takes too long (> 1 sec), we don't steal focus from other UI spots where due to asynchronous opening of sidebar, as user is free to continue interacting with other UI while it loads.
Whether delegating focus into some remote function vs. having a predictable mini-loop in-place is more elegant/transparent boils down to a matter of taste imo.

> function AbPanelLoad() {
>   if (location.search == "?focus") {
>     document.getElementById("peopleSearchInput").focus();
>     alert("AbPanelLoad(): Focus is already in search field, but AB contents
>            not yet loaded");
>   }
>   [snip]
>   ChangeDirectoryByURI(abPopup.value);

Comment 36

[Thomas D. (:thomas8)]

Attachment: Patch V.2.1 with alert for testing (nitfixed)

Ooops, forgot to update the patch.
Please refer to my comment 35.

Comment 37

[Thomas D. (:thomas8)]

What's the way forward here? Comment 35

Comment 38

[Thomas D. (:thomas8)]

Attachment: Patch V.2.2: (unbitrotted) Focus contacts sidebar when it gets shown by user.

Jörg, Mr. Code Sheriff, attachment 8999479 [details] [diff] [review] had all reviews before your comment 8 started this over again. A new patch using an allegedly "better" approach was produced (see my comment 35) and has been bitrotting for the last 6 months as Aceman's review hasn't happened.
I have now unbitrotted the patch - could you find a way of securing the missing review so that this can land now? Tia.

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 9046159 [details] [diff] [review]
Patch V.2.2: (unbitrotted) Focus contacts sidebar when it gets shown by user.

Yes, I told Aceman to do some reviews.

Comment 40

[:aceman]

Comment on attachment 9046159 [details] [diff] [review]
Patch V.2.2: (unbitrotted) Focus contacts sidebar when it gets shown by user.

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

Thanks, the code looks nice now, I will just test it live whether the AB cards are really loaded only after "load" event, so we can focus the inputbox before them.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6027,5 @@
>  
>  /**
>   * Focus the people search input in contacts side bar.
> + *
> + * @return {Boolean} true if peopleSearchInput was found, false otherwise.

Is the return value used anywhere? Or do you just add it as a general helper for the future?

Maybe you could use it at e.g. https://searchfox.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#6161 where if focusing contacts sidebar fails, you could move onto SetMsgBodyFrameFocus(). Otherwise we get stuck in the current element and focus is not changed to the sidebar. Thus user can press the hotkey but he gets stuck in window.content forever without moving in the ring.

Comment 41

[:aceman]

Comment on attachment 9046159 [details] [diff] [review]
Patch V.2.2: (unbitrotted) Focus contacts sidebar when it gets shown by user.

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

OK, I have run with this and all seems OK.
At least on Linux, the sidebar isn't rendered until even the cards are loaded, which may take a while (like a second) with thousands of contacts.
However it does seem focus is set before loading the cards.

The load with many cards is so slow that I manage to do this:
1. have cursor in recipient 1
2. press F9
3. click Subject and type something 
4. the click is not visible yet as TB appears frozen while loading the sidebar and all cards.
5. after the card load finishes, the cursor arrives in to the subject and the typed characters appear there.

So that is fine and focus is not stolen to AB sidebar after the cards load.

To solve the long freeze of UI, I get much better experience by loading the cards search results on a timer, e.g. setTimeout(ChangeDirectoryByURI, 0, abPopup.value); in AbPanelLoad() in abContactsPanel.js.

Comment 42

[Thomas D. (:thomas8)]

Thanks.

Let's land this now, any other polishing should go into a new bug.

We might still want to check how long it takes to load 1000 AB's into the AB picker dropdown (not the search results list), because iiuc we will only focus the search input after populating that. We don't want to steal focus after more than 1 or 2 seconds. Anyway, my fail-proof proposal was rejected on mostly aesthetic grounds, so I guess that edge case is for someone else to check.

Comment 43

[Jorg K (CEST = GMT+2)]

Any try run here? Last time we had test-focus.js in bug 1428631 comment #89.

Never mind, the friendly sheriff ran the test manually and it passed :-)

Comment 44

[:aceman]

(In reply to Thomas D. (currently busy elsewhere) from comment #42)

Let's land this now, any other polishing should go into a new bug.

Yes, we can land it, but please answer comment 40 or file the bug for it, otherwise the change to the return value seems unjustified.

And I can do the timeout enhancement.

We might still want to check how long it takes to load 1000 AB's into the AB picker dropdown (not the search results list), because iiuc we will only focus the search input after populating that. We don't want to steal focus after more than 1 or 2 seconds. Anyway, my fail-proof proposal was rejected on mostly aesthetic grounds, so I guess that edge case is for someone else to check.

I'm less concerned about that as having 1000 addressbook (as opposed to 1000 contacts) seems like very improbable situation.
Unless someone 'hoards' addressbooks, e.g. using a separate one for each project or group he participates in. For these mailinglists inside a single AB can be used.

Wayne, did you come across such a case in user reports?

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/198158c170d2
Focus people search input field when contacts sidebar gets shown by user. r=aceman, ui-r=Paenglab

Comment 46

[Geoff Lankow (:darktrojan)]

This bug has broken the mozmill test composition/test-send-button.js.

Comment 47

[:aceman]

And possibly composition/test-focus.js. I take that.

Comment 48

[Jorg K (CEST = GMT+2)]

Guys, I'm not checking any of this in any more unless you can show a green try.

Comment 49

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/719f15de3b87e2a3e481557a614bd9499b14881d
Backed out changeset 198158c170d2 (bug 1482718) for test failures in composition/test-send-button.js. a=backout

Comment 50

[Thomas D. (:thomas8)]

Thanks Geoff. Sorry I'm not into automated testing (yet). Aceman will look into it - thanks again.
I had a look at test-send-button.js, and I'm surprised why it should fail, because I don't see us changing anything here which is relevant for that test.

https://dxr.mozilla.org/comm-central/source/comm/mail/test/mozmill/composition/test-send-button.js?q=test-send-button.js&redirect_type=direct

Comment 51

[Thomas D. (:thomas8)]

Attachment: Patch V.2.2: (double-check in focus-ring) Focus contacts sidebar when it gets shown by user.

Minimally adjusted patch wrt focus ring safety checks, hence r?aceman.

(In reply to :aceman from comment #40)

Review of attachment 9046159 [details] [diff] [review]:
::: mail/components/compose/content/MsgComposeCommands.js
@@ +6027,5 @@

/**

• Focus the people search input in contacts side bar.

• • @return {Boolean} true if peopleSearchInput was found, false otherwise.

Is the return value used anywhere? Or do you just add it as a general helper
for the future?

Maybe you could use it at e.g.
https://searchfox.org/comm-central/source/mail/components/compose/content/
MsgComposeCommands.js#6161 where if focusing contacts sidebar fails, you
could move onto SetMsgBodyFrameFocus(). Otherwise we get stuck in the
current element and focus is not changed to the sidebar. Thus user can press
the hotkey but he gets stuck in window.content forever without moving in the
ring.

I guess the return value was introduced in the other patch approach. But given the delicacy of timing focus in contacts side bar as discussed in this bug, maybe it's helpful to keep it. I've added that extra safety check to the focus ring code as you suggested (although I don't see how we could possibly fail there; maybe in some extreme edge case scenarios like 1000 ABs AND weird keyboard sequences). Btw, shouldn't we dump a console.log rather than failing silently when we try to set focus bút don't find he searchbox (return=false)? Failure to focus should never happen in the current setup. If the console.error is wanted, which type should I use, dump, console.log, or something else?

Comment 52

[:aceman]

(In reply to Thomas D. from comment #51)

I guess the return value was introduced in the other patch approach. But given the delicacy of timing focus in contacts side bar as discussed in this bug, maybe it's helpful to keep it. I've added that extra safety check to the focus ring code as you suggested (although I don't see how we could possibly fail there; maybe in some extreme edge case scenarios like 1000 ABs AND weird keyboard sequences).

The sidebar could just be broken by an addon and not even load the inputbox :)

Btw, shouldn't we dump a console.log rather than failing silently when we try to set focus bút don't find he searchbox (return=false)? Failure to focus should never happen in the current setup. If the console.error is wanted, which type should I use, dump, console.log, or something else?

We could show an error but from some other place in the code (when loading the xul doc), not here where we just cycle focus.

Comment 53

[:aceman]

Comment on attachment 9047933 [details] [diff] [review]
Patch V.2.2: (double-check in focus-ring) Focus contacts sidebar when it gets shown by user.

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

Yes, this is what I meant.

Comment 54

[:aceman]

Attachment: test fix

We changed the sidebar URL so the test waiting for the specific URL needs to be adapted.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9a17388f72f49af4e2151c5843b3b87d6f58aeb8

Comment 55

[Jorg K (CEST = GMT+2)]

Comment on attachment 9047968 [details] [diff] [review]
test fix

It works, right? ;-)

Comment 56

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #44)

...
Wayne, did you come across such a case in user reports?

no

Comment 57

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #54)

Created attachment 9047968 [details] [diff] [review]
test fix

We changed the sidebar URL so the test waiting for the specific URL needs to be adapted.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9a17388f72f49af4e2151c5843b3b87d6f58aeb8

Thanks. Afasics, try looks all green.

With the benefit of hindsight, that little flaw in the test was hardly worth backing out the patch...
Back in it goes...

Comment 58

[Jorg K (CEST = GMT+2)]

(In reply to Thomas D. from comment #57)

With the benefit of hindsight, that little flaw in the test was hardly worth backing out the patch...
Back in it goes...
No, it doesn't since there is a linting issue now. Oh yes, I fixed that on the first landing.

Since we have about 10 developers working, there is a zero-tolerance to any failure, unless it can be fixed immediately. We can't have test failures on the tree that leave people wondering whether their new code caused them. Everyone has the right to get a green run.

Comment 59

[Jorg K (CEST = GMT+2)]

Grrr, the new BMO :-(

This was not meant to be part of the quote:
No, it doesn't since there is a linting issue now. Oh yes, I fixed that on the first landing.

Comment 60

[:aceman]

Yes, so it can now be checked in together with the test fix.

Comment 61

[Jorg K (CEST = GMT+2)]

Can you fix the linting error.

Comment 62

[Thomas D. (:thomas8)]

Attachment: Patch V.2.3: (nitfix linting) Focus contacts sidebar when it gets shown by user.

(In reply to Jorg K (GMT+1) from comment #61)

Can you fix the linting error.

My first time to go into that corner, but should be fixed now.
Anything else?

Comment 63

[:aceman]

Attachment: Patch V.2.3: (double-check in focus-ring) Focus contacts sidebar when it gets shown by user.

linter fix

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2e7a2dea92ad
Focus people search input field when contacts sidebar gets shown by user. r=aceman, ui-r=Paenglab
https://hg.mozilla.org/comm-central/rev/4e62f9aa0860
adapt test to changed URL of contacts sidebar. r=jorgk

Comment 65

[Thomas D. (:thomas8)]

(In reply to Pulsebot from comment #64)

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2e7a2dea92ad
Focus people search input field when contacts sidebar gets shown by user. r=aceman, ui-r=Paenglab
https://hg.mozilla.org/comm-central/rev/4e62f9aa0860
adapt test to changed URL of contacts sidebar. r=jorgk

Thank you!