686487 - Ctrl+K keyboard shortcut for global search does nothing if search box is hidden; should open global search tab.

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[David :Bienvenu]

For example, on windows, <cntrl K> should show the search widget even if the toolbar is hidden, because some people like to hide the toolbar to get max space for messages. Blake says the quick search widget displays itself when the shortcut is pressed, and we should be consistent with that as well.

Comment 1

[Wayne Mery (:wsmwk)]

is ctrl+K and shift+ctrl+K shown anywhere except in the respective widget, even if not hidden?  I'm not finding these in menu

Comment 2

[[e-mail]]

Same applies to Thunderbird om Mac OS X. The shortcut here is Cmd-K. When the mail toolbar is hidden, search functionality is unavailable.
From my user's point of view, a simple solution would be to have a menu item "Global Search" with shortcut Cmd-K in the Edit|Find menu. This menu item then opens a blank Search tab.
There is no other way to open a (blank) search tab.

Comment 3

[Thomas D. (:thomas8)]

(In reply to ev548 from comment #2)
> Same applies to Thunderbird om Mac OS X. The shortcut here is Cmd-K. When
> the mail toolbar is hidden, search functionality is unavailable.
> From my user's point of view, a simple solution would be to have a menu item
> "Global Search" with shortcut Cmd-K in the Edit|Find menu. This menu item
> then opens a blank Search tab.
> There is no other way to open a (blank) search tab.

+1. I've long wondered why "Global search" is missing from menus.

Expected behaviour:

1) Ctrl+K from main 3pane focuses Global search box on toolbar, if toolbar available. 
2) If toolbar not available, Ctrl+K opens a new tab with (blank) global search, and focus search box there.
3) Independent of toolbar availability, I'd suggest that using Edit > Find > Search all messages (to be added by this bug) should always open a new global search tab, which we would open anyway after starting a global search from the searchbox on toolbar (Alternatively, we could focus the global search box on toolbar if that's available).

Comment 4

[foxi]

This is still a problem in TB 24.4.0

Windows 8.1
32 bit

Control K does nothing if the mail toolbar is not in view.

Having the mail toolbar in view is a problem when navigating TB using the keyboard, it makes it more difficult.

Comment 5

[Deepjyoti Mondal]

The problem exists in ThunderBird 50.0a1 too.

Linux (ubuntu 16.04)
64 bit 

I would like to work on this bug. Can you please point out the files I need to look into in order to get started with this bug?

Comment 6

[Wei Zhang]

Attachment: search-without-toolbar.patch

Now it opens a new search tab when ctrl-K is pressed even with mail-toolbar closed.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Deepjyoti Mondal from comment #5)
> I would like to work on this bug. Can you please point out the files I need
> to look into in order to get started with this bug?

Sorry Deepjyoti, I didn't get round to assist with your helpful offer. Too much bugmail and not enough time...

Comment 8

[Thomas D. (:thomas8)]

Comment on attachment 8804815 [details] [diff] [review]
search-without-toolbar.patch

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

Thank you Wei for this nice and functional patch! :)

This looks good to me. With a few nits fixed, this should be ready to land.

1) Please focus/select the search box on the newly opened search tab, so that the keyboard workflow of Ctrl+K, then typing search words is not interrupted. This is a flaw already in the existing implementation, and I thought we already have a bug for this, but the closest I could find was my bug 525558. If it's too hard, we can do that in a followup bug.

2) The new search tab needs a title, looks odd without. Assuming that "searcher" flavor is the right type of thing to use (other types come with default title), I think it's wise to fix the original "searcher" code in glodaFacetTab.js so that there's a default title when searchString is empty:

https://dxr.mozilla.org/comm-central/source/mail/base/content/glodaFacetTab.js#58

      let searchString = aTab.searcher.searchString;
      aTab.searchInputValue = aTab.searchString = searchString;
      aTab.title = searchString ? searchString
                   : this.strings.get("glodaFacetView.tab.query.label");

3) Optionally, you could try to implement the other part of this bug: Adding a menuitem for Edit > Find > Global Search...
This is traditionally command-based, like this:
<command id="cmd_globalSearch" oncommand="...">
<menuitem command="cmd_globalSearch" key="...">
If you are lucky, such a command already exists (cmd_globalSearch was just an example, not the real name). Otherwise we have to create it.
For immediate success, we could land your corrected patch first and try the menuitem/command thing in a followup bug, cloned from this one.

Again, thank you very much for your contribution. We've got plenty of small bugs like this (often marked with "good first bug" in whiteboard*), so you are most welcome to try another one and CC me :)

*: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%3Athun%2Cmailn%20-product%3Aseam%20%5Bgood%20%5Bfirst%20%5Bbug&list_id=13295190

::: mail/base/content/mailWindowOverlay.js
@@ +3539,4 @@
>    }
>  
>    var quickSearchTextBox = document.getElementById('searchInput');
> +  if (quickSearchTextBox) {

This works, but it's a bit nested and your current logic unfortunately doesn't cover that other case where the searchInput box has been removed via toolbar customization, but the toolbar is still there. We should also fix that while we are here.
I verified that somewhat surprisingly, id="searchInput" is only  found in main 3pane, search boxes on search tabs don't have any ID (maybe that's why nobody tried to focus them...). Apparently mailWindowOverlay.js only applies to 3pane main mail view, so this code doesn't need to focus any other searchInput with id="searchInput" except the one on the mail toolbar of 3pane, so imo it's safe to combine and streamline conditions, like this:

  if (quickSearchTextBox && !document.getElementById('mail-bar3').collapsed) {
    // Focus and select global search box if available.
    quickSearchTextBox.select();
  } else {
    // If global search box has been removed from toolbar,
    // or if mail toolbar is not shown, open a new search tab.
    let args = {
      searcher: new GlodaMsgSearcher(null, "")
    };
    tabmail.openTab('glodaFacet', args);
  }

I wonder how that behaves if gloda has been disabled (options/pref), but otoh, one shouldn't try to use gloda after disabling it...

Comment 9

[Wei Zhang]

Attachment: version2

Hello Thomas. I fixed the patch and uploaded a version 2 of the patch.

Specifically, I added a default title to the search tab according to your suggestion:

> 2) The new search tab needs a title, looks odd without.

Also, I fixed the nested logic in mailWindowOverlay.js.

Regarding comments 1) and 3), I think it's better to fix them in subsequent bugs.

However, with gloda disabled in preferences, ctrl-k still opens a search tab, but without a search box. This is somewhat ugly though.

Comment 10

[Thomas D. (:thomas8)]

Comment on attachment 8808071 [details] [diff] [review]
version2

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

Thanks again for swift response and new patch!
lgtm. r=me with one nit fixed.

Aceman, if you could kindly have another look at this straightforward small patch and confirm my review with f+ or r+, and request checkin. Tia.

::: mail/base/content/glodaFacetTab.js
@@ +57,5 @@
>  
>        let searchString = aTab.searcher.searchString;
> +      aTab.searchInputValue = aTab.searchString = searchString;
> +      aTab.title = searchString ? searchString
> +            : this.strings.get("glodaFacetView.tab.query.label");

Nit: Pls adjust the indenting of the ':' section. Normally we indent by 2 spaces for each level, or otherwise aligned with something in the row above. So in this case, I think my original indenting is acceptable, with the : aligned with the s of "searchString ?" (or maybe with the equal sign). Unfortunately we can't push it to be under the question mark, because then the second line would have 85 characters but there's this style rule of 80 characters per line which is of advantage for several applications.

Comment 11

[Wei Zhang]

Attachment: version2 with indentation fixed

Thank you for the review.

I have fixed the indentation in this version.

Comment 12

[Thomas D. (:thomas8)]

Comment on attachment 8808903 [details] [diff] [review]
version2 with indentation fixed

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

Hi Wei, thanks, you're very quick ;)
Your patch works well as-is for all the default cases.

(In reply to Wei Zhang from comment #9)
> Created attachment 8808071 [details] [diff] [review]
> However, with gloda disabled in preferences, ctrl-k still opens a search
> tab, but without a search box. This is somewhat ugly though.

Indeed. With gloda disabled, we should never even get here, which is a pre-existing bug. Ideally, the keyboard shortcut Ctrl+K should be totally disabled in that case by the existing pref observer in search.xml, or perhaps show an error message when applied by user. This needs a separate bug for search.xml, which involves bindings, which is a bit deep for the uninitiated... I think it's ok for now if we catch that problem where it surfaces and suppress opening the blank tab. Checking a single boolean pref should not have noticeable performance implications, but let's play safe and only do that for the non-standard setups where the searchbox has been deliberately removed from toolbar, or the entire mail toolbar has been hidden. We should also document these issues in code. So I suggest this, pending review by someone other than me:

  var quickSearchTextBox = document.getElementById('searchInput');
  if (quickSearchTextBox && !document.getElementById('mail-bar3').collapsed) {
    // Focus and select global search box if available.
    // Bug: We can end up here even when gloda is disabled,
    //      which is wrong but it doesn't hurt:
    //      mailnews.database.global.indexer.enabled = false
    //      --> quickSearchTextBox.collapsed=true (via observer in search.xml)
    quickSearchTextBox.select();
  } else {
    // If global search box has been removed from toolbar via customization,
    // or if mail toolbar is not shown, open a new search tab.
    // Do nothing if gloda is disabled (workaround for above-mentioned bug).
    if (Services.prefs.getBoolPref('mailnews.database.global.indexer.enabled')) {
      let args = {
        searcher: new GlodaMsgSearcher(null, "")
      };
      tabmail.openTab('glodaFacet', args);
    }
  }

Comment 13

[:aceman]

Comment on attachment 8808903 [details] [diff] [review]
version2 with indentation fixed

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

Nice, seems to work for me. Please add the code comments Thomas mentions in last comment.
Please somebody file a bug for adding the global search into a menu.

Comment 14

[:aceman]

Comment on attachment 8808903 [details] [diff] [review]
version2 with indentation fixed

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

::: mail/base/content/mailWindowOverlay.js
@@ +3539,4 @@
>    }
>  
>    var quickSearchTextBox = document.getElementById('searchInput');
> +  if (quickSearchTextBox && !document.getElementById('mail-bar3').collapsed) {

Actually you can't check mail-bar3 as you can move the inputbox to any other toolbar, even the tab bar. You can check quickSearchTextBox.collapsed itself (that is being set in the observer in search.xml).

Comment 15

[:aceman]

Attachment: patch v3

I'm not sure the comments are needed. I've reworked the patch a bit. I just check the pref before the if() block. Maybe the check should be at the top of the function but I am not sure about that right now. So this version should be safe. Please check if it still does what needs to be done.

Comment 16

[Thomas D. (:thomas8)]

Comment on attachment 8810243 [details] [diff] [review]
patch v3

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

Unfortunately, this doesn't work.

::: mail/base/content/mailWindowOverlay.js
@@ +3538,5 @@
>      return;
>    }
>  
> +  // Bail out if gloda is disabled.
> +  if (!Services.prefs.getBoolPref("mailnews.database.global.indexer.enabled"))

If my old comment is correct, listener will collapse quicksearchtextbox when indexer.enabled == false. So it should be easier and more performant to just check for quicksearchTextBox.collapsed here.

@@ +3545,2 @@
>    var quickSearchTextBox = document.getElementById('searchInput');
> +  if (quickSearchTextBox && !quickSearchTextBox.collapsed) {

This doesn't work. The searchbox can be on a toolbar which is collapsed, i.e. it exists but is not itself collapsed.
We need to find out on which toolbar the searchbox sits, then check if that toolbar is collapsed.

Unfortunately, I couldn't find out yet which element gets collapsed when the main toolbar is hidden, it's not the parentElement.parentElement as on mailbar-3.

@@ +3548,4 @@
>      quickSearchTextBox.select();
> +  } else {
> +    // If global search box has been removed from toolbar via customization,
> +    // or if mail toolbar is not shown, open a new search tab.

adjust comment as searchbox can be on other hidden toolbars, too.

Comment 17

[Thomas D. (:thomas8)]

Attachment: Patch V.4: Open global search in a new tab whenever, whereever, and however search input box is hiding

Per Aceman's comment 14 and my comment 16, this turned out to be much more tricky than expected. Global search box, while still present in the DOM as an element, can be hidden on all sorts of host toolbars, menubar, tabbar (via toolbar customization) when these are hidden, and it's not trivial to know when they are hidden.

With this patch, whenever, whereever, and however the search box is hidden (or removed via customization), we'll open a new global search tab.
When gloda is disabled, and/or search box is collapsed, we do nothing.
So this should handle all possible cases cleanly.

Comment 18

[Thomas D. (:thomas8)]

Aceman, ping for this little review... should be straightforward, tried and tested working!

Comment 19

[Thomas D. (:thomas8)]

Maybe we can streamline tabmail check, put it to top of function and remove all subsequent occurences:

if (!tabmail) {
  return true
}

I hesited to do that because we have code patterns which don't require tab mail at top of function. However, I believe that Gloda search results are always displayed in a new tab (displaying in same tab would require dedicated close button or navigation, which TB doesn't have), so it looks like without tabmail we can't display results and need to bail out. I also wonder how tabmail element could ever NOT be present, but anyway, we can just keep the single check at the beginning of function to err on safe side.

Comment 20

[Thomas D. (:thomas8)]

In the worst scenario, this would mean that we don't focus an existing and visible searchbox because we anticipate that we can't display results because of missing tabmail. Purely hypothetical.
If tabmail is a must for gloda searches, maybe we should also add a dump if it's not present.

Comment 21

[:aceman]

Comment on attachment 8819909 [details] [diff] [review]
Patch V.4: Open global search in a new tab whenever, whereever, and however search input box is hiding

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

::: mail/base/content/mailWindowOverlay.js
@@ +3553,5 @@
> +    // Via toolbar customization, globalSearchTextBox can be in different places.
> +    // If the grandParent of globalSearchTextBox is hidden, use a new tab.
> +    let theParent = globalSearchTextBox.parentElement.parentElement;
> +    switch(theParent.id) {
> +      case "mail-bar3":

You can't just hardcode all the various toolbars here. You never catch them all, users can add their own. I even put the search box on the folder pane toolbar and that was missed here. Ctrl+K did nothing.

We need to find a generic was to see if the searchbox is visible.
Try globalSearchTextBox.ownerGlobal.getComputedStyle(globalSearchTextBox).visibility != "visible".
Taken from https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#680 .

@@ +3575,5 @@
> +      return;
> +
> +    let args = {
> +      searcher: new GlodaMsgSearcher(null, "")
> +    };

Also please do not hardcode all this? There already must be code that does this opening of search tab, so let's just call it and not duplicate. Can you do something like globalSearchTextBox.doSearch()?

Comment 22

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #21)
> Comment on attachment 8819909 [details] [diff] [review]
> Patch V.4: Open global search in a new tab whenever, whereever, and however
> search input box is hiding
> 
> Review of attachment 8819909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/mailWindowOverlay.js
> @@ +3553,5 @@
> > +    // Via toolbar customization, globalSearchTextBox can be in different places.
> > +    // If the grandParent of globalSearchTextBox is hidden, use a new tab.
> > +    let theParent = globalSearchTextBox.parentElement.parentElement;
> > +    switch(theParent.id) {
> > +      case "mail-bar3":
> 
> You can't just hardcode all the various toolbars here. You never catch them
> all, users can add their own. I even put the search box on the folder pane
> toolbar and that was missed here. Ctrl+K did nothing.
> 
> We need to find a generic was to see if the searchbox is visible.

Yes, a generic way would be much preferable...

> Try
> globalSearchTextBox.ownerGlobal.getComputedStyle(globalSearchTextBox).
> visibility != "visible".

However, this is not generic enough.
It fails already for the simple case of placing the searchbox on the menu bar (via toolbar customization), then hiding the menu bar. In that case, .visibility returns "visible" although the searchbox is NOT visible.

I tried to find a generic way of reliably determining searchbox visibility when coding this, but I couldn't find any. So unless somebody offers a better generic way of determining searchbox visibility, I can't think of another approach than the one I proposed. If we cover all inbuilt toolbars (which are limited and known), maybe addons can cover their own toolbars.

Comment 23

[Thomas D. (:thomas8)]

Also, we might be able to iterate through all toolbars including any custom toolbars.

Note that the hardcoded special-casing covers different animals:
1) the traditional toolbar case ("mail-bar3"; here we can try iteration)
2) the menubar ("mail-toolbar-menubar2"; using autohide attribute)
3) the tabbar ("tabbar-toolbar"; using autohide pref for single tab)

2) and 3) are quite unique and unlikely to be duplicated by addons, so we're most interested in 1).

Comment 24

[Thomas D. (:thomas8)]

How can I see the folder pane toolbar?

Comment 25

[Thomas D. (:thomas8)]

Aceman, can you comment on comment 22, 23, 24?

Comment 26

[Thomas D. (:thomas8)]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #25)
> Aceman, can you comment on comment 22, 23, 24?

And also comment 19...

Comment 27

[Thomas D. (:thomas8)]

^^

Comment 28

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #19)
> Maybe we can streamline tabmail check, put it to top of function and remove
> all subsequent occurences:
> 
> if (!tabmail) {
>   return true
> }
> 

I think let's try to do it.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #22)
> > We need to find a generic was to see if the searchbox is visible.
> 
> Yes, a generic way would be much preferable...
> 
> > Try
> > globalSearchTextBox.ownerGlobal.getComputedStyle(globalSearchTextBox).
> > visibility != "visible".
> 
> However, this is not generic enough.
> It fails already for the simple case of placing the searchbox on the menu
> bar (via toolbar customization), then hiding the menu bar. In that case,
> .visibility returns "visible" although the searchbox is NOT visible.

So have you tried the recursive version at
https://dxr.mozilla.org/mozilla-central/rev/8d026c60151005ad942e3d4389318fe28a0c8c54/browser/base/content/test/general/head.js#666 ?

> I tried to find a generic way of reliably determining searchbox visibility
> when coding this, but I couldn't find any. So unless somebody offers a
> better generic way of determining searchbox visibility, I can't think of
> another approach than the one I proposed. If we cover all inbuilt toolbars
> (which are limited and known), maybe addons can cover their own toolbars.

Np, addons will surely not case about this and not notice they need to add their toolbar to some whitelist for this very edge-case. They are happy they stay alive with all the hurdless Mozilla is throwing at them.

We need to find a generic way to automatically determine this.

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #24)
> How can I see the folder pane toolbar?

Enable it via view->toolbars->folder pane toolbar and it appears above the folder pane. It is not in TB45, but later. But I don't want that you start hunting for it. The solution here must work for any toolbars (or even other elements, who know where you can put the toolbar items).

Comment 29

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #28)
> So have you tried the recursive version at
> https://dxr.mozilla.org/mozilla-central/rev/
> 8d026c60151005ad942e3d4389318fe28a0c8c54/browser/base/content/test/general/
> head.js#666 ?

Out of the box, even after adding a check for the existence of element, I tested and it does NOT work, e.g. for menu bar: is_visible returns true even though menu bar with searchbox is hidden.

We can try checking for all those special attributes which are used when menu bar etc. are hidden (see comment 63), but then sometimes it's just a pref, so we'd have to find out which attribute is toggled by that pref. Still, it remains complicated and we are spending more and more time here for real edge cases. Searchbox on folder pane toolbar is theoretically possible, but practically very unlikely.

I would be very fine with hard-coding the most obvious cases as an incremental increment of the status quo and leave perfection for another day. It's easy to change when a better solution will be found.

Comment 30

[:aceman]

Can you try
newTab = (globalSearchTextBox.clientHeight == 0) ||
         (globalSearchTextBox.clientWidth == 0);

Comment 31

[:aceman]

(In reply to :aceman from comment #21)
> @@ +3575,5 @@
> > +      return;
> > +
> > +    let args = {
> > +      searcher: new GlodaMsgSearcher(null, "")
> > +    };
> 
> Also please do not hardcode all this. There already must be code that does
> this opening of search tab, so let's just call it and not duplicate. Can you
> do something like globalSearchTextBox.doSearch()?

Asuth, is there something we can call to not duplicate this code in the patch?

Comment 32

[Andrew Sutherland [:asuth] (he/him)]

(In reply to :aceman from comment #31)
> Asuth, is there something we can call to not duplicate this code in the
> patch?

As I understand the scenario, the motivating use-case is when there is no "#searchInput" to be found anywhere in the UI.  In that case, a "glodaFacet" tab can be opened and its ".remote-gloda-search" input used.

The problem is that the "glodaFacet" tab type defined in glodaFacetTab.js wants one of query/searcher/collection passed.

I would suggest augmenting glodaFacetTab.js to handle a fourth default "else {" case that initializes the tab to be open.  That probably just looks like:
  else {
    aTab.title = this.strings.get("glodaFacetView.tab.query.label");
    aTab.searchString = "";
  }

If the search-box doesn't auto-focus, it might be necessary to explicitly have glodaFacetTab do aTab.iframe.focus() and/or have glodaFacetView.js do something in reachOutAndTouchFrame().

This should eliminate the need for code duplication and make it easier for a menu item to open gloda search, etc.

Comment 33

[:aceman]

(In reply to :aceman from comment #28)
> Np, addons will surely not case about this and not notice they need to add
> their toolbar to some whitelist for this very edge-case. They are happy they
> stay alive with all the hurdless Mozilla is throwing at them.

Should have been: "NO, addons will surely not caRe about this"...

Comment 34

[:aceman]

Comment on attachment 8819909 [details] [diff] [review]
Patch V.4: Open global search in a new tab whenever, whereever, and however search input box is hiding

Please update the patch according to what I and asuth have said.

Comment 35

[Thomas D. (:thomas8)]

Attachment: Patch V.5: (using clientHeight/Width check) Open global search in a new tab when search input box is hidden

(In reply to :aceman from comment #30)
> Can you try
> newTab = (globalSearchTextBox.clientHeight == 0) ||
>          (globalSearchTextBox.clientWidth == 0);

Thanks, that's a nice solution which covers most cases correctly. As this still doesn't work for the menu bar, I had to special-case that.

(In reply to Andrew Sutherland [:asuth] from comment #32)
> The problem is that the "glodaFacet" tab type defined in glodaFacetTab.js
> wants one of query/searcher/collection passed.
> 
> I would suggest augmenting glodaFacetTab.js to handle a fourth default "else
> {" case that initializes the tab to be open.  That probably just looks like:
>   else {
>     aTab.title = this.strings.get("glodaFacetView.tab.query.label");
>     aTab.searchString = "";
>   }

This is probably going the right way, but doesn't work as-is.
We have a lot of gloda code like this, in various files:
> if (x in aArgs) {...}
When aArgs is omitted, such code fails with errors because "in" expects a valid object. I succeeded to eliminate that problem for glodaFacetTab.js by testing for (!aArgs) as first condition along Andrew's proposal, I even passed in an empty object {} for aArgs, which made the new Tab open at least, but with "undefined" in title and search field. Reason: there are various other files along the codepath which expect aArgs and/or assume "collection" for non-"searcher" and non-"query" tab types, which in turn fails on collection stuff because without arguments it's not a collection tab.

So this requires changes all along the gloda call chain, and might come with unexpected side effects. This is beyond me and beyond the scope of this bug.

I suggest reviewing and landing this as-is, then we can streamline the blank-gloda-tab scenario in a followup bug (which affects only 2 consecutive lines in this patch).

Comment 36

[Thomas D. (:thomas8)]

Attachment: Patch V.5.1: (using clientHeight/Width check) Open global search in a new tab when search input box is hidden

Same as V.5, re-include the check if gloda is enabled (which got lost accidentally).

Comment 37

[:aceman]

Attachment: Patch V.6

Would this work for you?

Asuth, this has one glitch. It seems to open correctly. But when I now type a string into the search and press enter another gloda tab is opened. Did I miss some set up somewhere?

Comment 38

[Andrew Sutherland [:asuth] (he/him)]

It looks like there are 2 possible paths when you trigger a search:
1) You had picked a glodacomplete "noun" item like a specific contact or tag.  This opens a new tab no matter what.  (Consult search.xml's "observe" implementation.)
2) A text string was completed or entered via search.xml's "doSearch" method.  In this case, the current tab is closed and a new one opened.  Per the comments, this is done because reusing the tab was non-trivial in the case where it was assumed the tab already had results.  That doesn't thoroughly hold here, but it's also not clear why closing the tab would fail and you'd perceive a new tab.

Comment 39

[:aceman]

Attachment: Patch V.6.1

OK, now using doSearch(). It seems passing it an empty string works fine.

Comment 40

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8854084 [details] [diff] [review]
Patch V.6.1

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

This all seems reasonable to me.

Comment 41

[Thomas D. (:thomas8)]

Comment on attachment 8854084 [details] [diff] [review]
Patch V.6.1

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

Cool. Perfect. One comment nit. Should be ready to land now.

::: mail/base/content/mailWindowOverlay.js
@@ +3578,5 @@
> +  if (globalSearchTextBox) {
> +    // Via toolbar customization, globalSearchTextBox can be in different places:
> +    // Toolbars, tab bar, menu bar, etc. When the containing elements are hidden,
> +    // the searchbox will also be hidden, so clientHeight and clientWidth of the
> +    // searchbox or its parent will typically be zero and we can test for that.

Nit:
 // searchbox or one of its parents will typically be zero and we can test for that.

@@ +3582,5 @@
> +    // searchbox or its parent will typically be zero and we can test for that.
> +    // If globalSearchTextBox is hidden, use a new tab.
> +    newTab = false;
> +    let element = globalSearchTextBox;
> +    while (element) {

Nice. I assume that you have tested this for the case where searchbox is on menu bar and menu bar is hidden?

@@ +3594,5 @@
> +    // Focus and select global search box.
> +    globalSearchTextBox.select();
> +  } else {
> +    // Open a new global search tab.
> +    globalSearchTextBox.value = "";

Fwiw, I think this will overwrite existing search words in the search box if toolbar with search box is hidden by user while words are present, but that's OK since the searchbox is already hidden so we can't continue on stale search words.

::: mail/base/content/search.xml
@@ +223,2 @@
>            try {
> +            if (this.value || aEmpty) {

Nice hack.

Comment 42

[:aceman]

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #41)
> Nit:
>  // searchbox or one of its parents will typically be zero and we can test
> for that.

OK, thanks.
 
> Nice. I assume that you have tested this for the case where searchbox is on
> menu bar and menu bar is hidden?

Yes, works.

Comment 43

[:aceman]

Attachment: patch v6.2

Carrying over r=asuth.

Comment 44

[:aceman]

Thomas, is there a bug about adding a menuitem to the main menu for the global search? Will you look into it?
It seems this request still remained in the summary of this bug but we didn't implement it.

Comment 45

[Jorg K (CEST = GMT+2)]

Let me handle the checkin here or else we run the risk of destroying the "ü" ;-)

Comment 46

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/95da4e9ee8dc6cbf96b056b3e0e70ae90d0fff46

Comment 47

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #44)
> Thomas, is there a bug about adding a menuitem to the main menu for the
> global search?

Filed Bug 1356792.

(In reply to Jorg K (GMT+2) from comment #45)
> Let me handle the checkin here or else we run the risk of destroying the "ü"
> ;-)

Fwiw, now that you've migrated me into accepting "ue" for "ü" in my name as patch author, I'd prefer if we can keep it consistent and stick with "ue", so that it can be tracked more easily and completely after the name tweak.

Comment 48

[Jorg K (CEST = GMT+2)]

I know, but the patch was submitted with a "ü".

Comment 49

[Thomas D. (:thomas8)]

Unfortunately, per testing in Daily 55.0a1 (2017-04-15) (64-bit), this doesn't work as expected after Aceman's tweaks.

1) After removing global searchbox via toolbar customization, Ctrl+K does nothing, while it should open a new search tab, which was working correctly in one of my previous patches.
Iow, we can't use globalSearchBox.doSearch when globalSearchBox element may not even be available.

2) The new search tab does not have a title, and it does not focus its searchbox, both of which was working correctly in one of my previous patches.

Which leads me to suspect we may want to include this in manual or automated tests somehow.

Scenarios for testing (covered by this bug):

1) With global search box visible on Mail Toolbar (default), press Ctrl+K.
-> Focus should move to global search box on current tab.

2) Via toolbar customization, remove global searchbox from Mail Toolbar, confirm done, press Ctrl+K.
-> New global search tab should open, with focus in search box. Tab title and document title should be "Search".

3) Via toolbar customization, move global search box to any of the following toolbars, and confirm done:
a) Mail Toolbar (default)
b) Tab bar. From Tools > Options > Advanced > General > Config editor, set mail.tabs.autoHide = true, confirm. Ensure having only 1 tab open.
c) Menu bar.
d) Folder pane toolbar.
Then hide the respective toolbar with the global search box, via main menu's View > Toolbars > ...
Press Ctrl+K.
-> New global search tab should open, with focus in search box. Tab title and document title should be "Search".

Comment 50

[Jorg K (CEST = GMT+2)]

Thanks for testing. I've backed this out since the next branch date is in three days and I prefer not to manage a follow-up crossing cycles.

Backout:
https://hg.mozilla.org/comm-central/rev/f1214f53c1c3b9ba3a99cda581e2f2396f8ca9c8

Comment 51

[Thomas D. (:thomas8)]

Attachment: Patch V. 7: New approach, streamlined code, tentatively cover Chat, fix bug 1357078

Here's a new patch which addresses the problems mentioned in comment 49.

Per bug 1357078 comment 1, also includes minimal fix for:
Bug 1357078 - Global search faceted results page has no useful keyboard focus nor indication thereof; should helpfully focus search input box to streamline workflows of modifying searches

The reshuffling/streamlining of code also effects fix for this problem:
Bug: After removing searchInput from Chat tab via toolbar customization, Ctrl+K does nothing.

I'm not 100% sure about the correct behaviour for that edge case of Chat, and I tried to test Chat fulltext searches but failed. Can someone file a screenshot of a successful global search from Chat Tab, involving chat text?
I guess the expected result for Ctrl+K on chat tab (with searchInput removed) could be anything; should probably trigger the same global search as if it was triggered from the searchInput on Chat Tab. With this patch, I think it might only return results from mail messages. Arguably still better than doing nothing... ;)
(If we want to handle newTab differently for Chat, we could either:
- revert code streamline back to modular with code duplication, but possibly keeping switch structure
- add a special condition or another switch in the newTab execution)

Feel free to take/adjust reviews as appropriate.

Comment 52

[Jorg K (CEST = GMT+2)]

Comment on attachment 8858899 [details] [diff] [review]
Patch V. 7: New approach, streamlined code, tentatively cover Chat, fix bug 1357078

I think I trust Aceman on this one.

Comment 53

[:aceman]

(In reply to Jorg K (GMT+2) from comment #52)
> I think I trust Aceman on this one.
You shouldn't, if I have supposedly botched the previous version :)

(In reply to Thomas D. (currently busy elsewhere; needinfo?me) from comment #49)
> 1) After removing global searchbox via toolbar customization, Ctrl+K does
> nothing, while it should open a new search tab, which was working correctly
> in one of my previous patches.

Thanks for checking this.

> Iow, we can't use globalSearchBox.doSearch when globalSearchBox element may
> not even be available.

Items that are in the Customize palette have a different ID so it may be somewhere but not with the ID we are checking for. But yes, hunting for out-of-DOM-tree or intentionally hidden elements is not a great pattern.

Comment 54

[:aceman]

Comment on attachment 8858899 [details] [diff] [review]
Patch V. 7: New approach, streamlined code, tentatively cover Chat, fix bug 1357078

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

Thanks, this seems to work fine for me.

::: mail/base/content/tabmail.xml
@@ +473,5 @@
>            }
>  
> +          // For "glodaFacet" tab mode, if aArgs is omitted,
> +          // default to a blank user search.
> +          if (aTabModeName=="glodaFacet" && !aArgs) {

I don't like handling this specific tab mode here in the generic function in tabmail. Can't this be done in glodaFacetTab.js? Some variation on what I have done.

Comment 55

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #54)
> Comment on attachment 8858899 [details] [diff] [review]
> Patch V. 7: New approach, streamlined code, tentatively cover Chat, fix bug
> 1357078
> 
> Review of attachment 8858899 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this seems to work fine for me.

That's great! "Working fine" looks like a good precondition for landing...

> ::: mail/base/content/tabmail.xml
> @@ +473,5 @@
> >            }
> >  
> > +          // For "glodaFacet" tab mode, if aArgs is omitted,
> > +          // default to a blank user search.
> > +          if (aTabModeName=="glodaFacet" && !aArgs) {
> 
> I don't like handling this specific tab mode here in the generic function in
> tabmail. Can't this be done in glodaFacetTab.js? Some variation on what I
> have done.

I think we're now starting to go in circles, and maybe we're going too far. Afaics, openTab is a cross-tab-mode function in generic tabmail.xml which expects aArgs, a bundle of generic and specific variables defining the behaviour of the new tab. We decided that we don't want to create aArgs in the caller because it looks odd. That's why in my latest patch, we're now creating aArgs in generic openTab function if it's missing (and to play safe, we only allow that trick for glodaFacet tab mode which we're tweaking here). Now you're requesting to move the issue of aArgs even further into the specific tab mode code module for glodaFacet tab. However, not having aArgs for openTab method in tabmail.xml would require significant and imo much more complex rewrites because aArgs is required in that generic function all over the place, as I tried to explain before (background, disregardOpener, shouldSwitchToFunc, then passed on to the specific mode's own openTab function...). There might be some way to do this in glodaFacetTab.js, if we employ more coding time or a more experienced coder. Personally, I don't see how this could easily be done in variation of your tweak... (Btw your last tweak required an extra boolean argument rather than just omitting aArgs if we don't need it, which imo is also odd and more complex for callers than necessary).

So unfortunately, there's nothing more which I can do here. Given that we expect all of TB's code to be rewritten anyway, I'm also wondering if we might get away with 95% perfection in the current code? After all, as you said, this is working as expected, and much better than before. I'd even prefer filing another bug for the remaining 5% perfection to save our precious time for more important things, and get on with fixing this and other problems for the user...

So I suggest to land this as-is after your review, if there are no other problems.

Comment 56

[Jorg K (CEST = GMT+2)]

What's the status here?

Comment 57

[Thomas D. (:thomas8)]

I've provided a patch which works correctly, fixing a great range of issues and code streamlining beyond the original scope of this bug. Per comment 54, Aceman agrees that it works but he doesn't like a small detail of the patch, namely where exactly missing aArgs array (which we want to omit from the openTab() caller) should be handled. I explained in comment 55 why I think it could/should be handled where I currently handle it, imo anything else will require significant workarounds/rewrites elsewhere which sort of leads the intention of more elegant code ad absurdum, apart from wasting more time on this rather insignificant detail. So atm there's nothing for me to do here.

So I think that my current patch should be reviewed and landed as is if there's nothing else.

Aceman?

Comment 58

[Thomas D. (:thomas8)]

Comment on attachment 8858899 [details] [diff] [review]
Patch V. 7: New approach, streamlined code, tentatively cover Chat, fix bug 1357078

Can we please have a final review and land this before it bitrots? We took long to refine this and cover lots of edge-case customization scenarios, it was a lot of work, and this final patch has been tried and tested working by Aceman. I don't see any need to stall this for shifting 2 straightforward lines of code into a more perfect location (as desired in comment 54) which imo will require a major rewrite of the whole thing (as explained in my comment 55), that's if it's possible at all. It's just not worth it. Thanks.

Comment 59

[Jorg K (CEST = GMT+2)]

Comment on attachment 8858899 [details] [diff] [review]
Patch V. 7: New approach, streamlined code, tentatively cover Chat, fix bug 1357078

I'm really not familiar with this code and it would take me a long time to get up to speed here. Aceman was on PTO (it's summer!), so I'll ask him personally. Sorry I can't give you a better answer.

Comment 60

[:aceman]

Comment on attachment 8858899 [details] [diff] [review]
Patch V. 7: New approach, streamlined code, tentatively cover Chat, fix bug 1357078

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

Thanks.

::: mail/base/content/tabmail.xml
@@ +474,5 @@
>  
> +          // For "glodaFacet" tab mode, if aArgs is omitted,
> +          // default to a blank user search.
> +          if (aTabModeName=="glodaFacet" && !aArgs) {
> +            var aArgs = {

I think this 'var' isn't needed. The variable is already declared.

::: mail/locales/en-US/chrome/messenger/glodaFacetView.properties
@@ +18,3 @@
>  # LOCALIZATION NOTE(glodaFacetView.search.label):
>  #  The heading for the search page.
> +#  A short constraint description of user's search string will be appended.

'constraint'? Did you mean something else here?

Comment 61

[Thomas D. (:thomas8)]

Attachment: Patch V.7.1: New approach, streamlined code, tentatively cover Chat, fix bug 1357078

This patch was started by Wei Zhang, with ui-r=me (comment 10), after I defined the expected UX in comment 3. Aceman helped a lot to get this right, and my implicit ui-r of comment 49 again helped a lot to get it *really* right. So ui-r=me looks right in this case ;)

We also fix Bug 1357078 with this patch (focus search input on global search faceted results), which is a small but nifty improvement of global search workflow allowing users to seamlessly correct, narrow down, or rephrase their search.

Great teamwork, nice patch!

(In reply to :aceman from comment #60)
> I think this 'var' isn't needed. The variable is already declared.

Oh yes. Fixed.

> >  # LOCALIZATION NOTE(glodaFacetView.search.label):
> >  #  The heading for the search page.
> > +#  A short constraint description of user's search string will be appended.
> 
> 'constraint'? Did you mean something else here?

I actually meant 'constraint' as in 'search constraint/condition'. But I'm not sure why I chose that term, and it's odd, maybe wrong. So I changed the comment:
> +#  A short description of user's search query will be appended.

Comment 62

[Thomas D. (:thomas8)]

Turns out that this wasn't a [good first bug], quite tricky/sophisticated to get this right.

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a6e0661213c9
Ctrl+K should open a new global search tab if searchbox is not available. r=asuth,aceman

Comment 64

[Thomas D. (:thomas8)]

Jörg is always pushing all the good things in so fast. Thank you for that! :)