Bugzilla

Comment 4

•

9 years ago

Robert, if you want to give this a pass, it may be enough for message_list's _cacheDom to add a "disabled" attribute to that input, and then in the createdCallback, make sure that disabled attribute is removed.

Flags: needinfo?(robert.sajdok)

Assignee

Updated

•

9 years ago

Assignee: nobody → robert.sajdok

Flags: needinfo?(robert.sajdok)

Comment 5

•

9 years ago

Attached file [gaia] rsajdok:bug#1137486 > mozilla-b2g:master — Details

Assignee

Updated

•

9 years ago

Attachment #8573215 - Flags: review?(jrburke)

Comment 6

•

9 years ago

Comment on attachment 8573215 [details] [review]
[gaia] rsajdok:bug#1137486 > mozilla-b2g:master

I left some comments in the pull request. Thanks for looking into this.

Attachment #8573215 - Flags: review?(jrburke)

Assignee

Comment 7

•

9 years ago

(In reply to James Burke [:jrburke] from comment #6)
> Comment on attachment 8573215 [details] [review]
> [gaia] rsajdok:bug#1137486 > mozilla-b2g:master
> 
> I left some comments in the pull request. Thanks for looking into this.

I have read your comments on the github. I have some doubts. The method createdCallback is called before the method _cacheDom. Thus, We should set this.searchButton.disabled = true; in createdCallback and this.searchButton.disabled = false; in _cacheDom. I do not know why but it works. In the opposite direction it does not work.

Flags: needinfo?(jrburke)

Andrew Sutherland [:asuth] (he/him)

Comment 8

•

9 years ago

The occurs because the HTML cache is used, so the DOM with the search input is visible/tappable, but the JS code in message_list.js has not actually been loaded yet and bound to the DOM.

So the goal is to have the disabled attribute set on the input as part of the HTML template by default, but in the createdCallback of message_list to then set it to be disabled = false, so that the user can tap on it. At that point, the message_list.js will have bound to the element and the onSearchButton function can be called when that text field is tapped.

Since the element will not be disabled when it is cloned for caching, the disabled needs to be set to true for the cloneNode. 

Enabling `this.searchButton.disabled = false` in the _cacheDom does not feel right since ideally the _cacheDom function could be removed, as it is just for caching, and the card would still function correctly.

I tried the following diff, but oddly I can still, sometimes, get the keyboard to show up even though the form element is not enabled until later, where onSearchButton for the focus should work. Maybe the issue is that focus is not available or discarded, but taps register OK? Not sure yet, feels like some subtle event stuff is going on there, so do not have a full picture of the solution.

diff --git a/apps/email/js/cards/message_list.html b/apps/email/js/cards/message_list.html
index ff412da..79a4866 100644
--- a/apps/email/js/cards/message_list.html
+++ b/apps/email/js/cards/message_list.html
@@ -100,7 +100,9 @@
         <input data-event="focus:onSearchButton"
                class="msg-search-text-tease" type="text"
                dir="auto"
-               data-l10n-id="message-search-input" />
+               data-l10n-id="message-search-input"
+               disabled
+               data-prop="searchButton" />
       </p>
     </form>
     <div data-prop="messagesContainer" class="msg-messages-container"
diff --git a/apps/email/js/cards/message_list.js b/apps/email/js/cards/message_list.js
index feee76d..6cf7552 100644
--- a/apps/email/js/cards/message_list.js
+++ b/apps/email/js/cards/message_list.js
@@ -366,6 +367,10 @@ return [
     postInsert: function() {
       this._hideSearchBoxByScrolling();

+      if (this.mode === 'nonsearch') {
+        this.searchButton.disabled = false;
+      }
+
       // Now that _hideSearchBoxByScrolling has activated the display
       // of the search box, get the height of the search box and tell
       // vScroll about it, but only do this once the DOM is displayed
@@ -1132,6 +1137,10 @@ return [
         this._cacheListLimit
       );

+      // Make sure search teaser target is disabled by default while the JS
+      // logic is loaded.
+      cacheNode.querySelector('.msg-search-text-tease').disabled = true;
+
       htmlCache.saveFromNode(module.id, cacheNode);
     },


Setting disabled false in _cacheDom may have worked better since that method executes quite a bit later, once data has been received, but I do not think that is the right place to enable that input. Trouble is, at the moment, I am not sure yet when is, at least without using a setTimeout, which would be a terrible hack. Ideally we can find an event or stage where it makes sense to do it. Not sure what that is yet, so open to ideas.

Flags: needinfo?(jrburke)

Comment 9

•

9 years ago

What if we left the search text input enabled and at HTML attach time we check if it has the focus and if so we act like the user clicked on the the field?

Comment 10

•

9 years ago

That sounds fine to me, feels nicer too. What do you think Robert?

Andrew Sutherland [:asuth] (he/him)

Assignee

Comment 11

•

9 years ago

Andrew,
Please, describe it more clearly. What does it mean "at HTML attach time" and "we act like the user clicked on the field"

Comment 12

•

9 years ago

I think that what we mean is to add code like the following in message_list's postInsert method:

  // In the case we were displayed from the HTML cache, the user may have found and
  // clicked on the search box in the interim.  In that case, trigger the search
  // functionality.  However, we can't trigger the search until we've actually loaded
  // the folder.  Using latestOnce with 'folder' should work because other code does
  // latest('folder', this._folderChanged) which will trigger showFolder.  It might
  // be better if we instead have onFolderShown emit an event and we use once() with
  // that?
  if (document.activeElement === this.searchInput) {
    model.latestOnce('folder', function() {
      this.onSearchButton();
    }.bind(this));
  }


As may be clear from my speculative comment there, I'm not 100% sure of the cleanest way to to do this.

Comment 13

•

9 years ago

For the search mode on message_list, we do not need to wait for the folder as far as that card's startup. It might be worthwhile from a performance perspective though, just to not try to animate while we are still updating the message_list card once the folder is available.

In that case, I would consider doing the work at the end of onFolderShown. It can detect if search mode and if isFirstTimeVisible and if document.activeElement matches, then do the this.onSearchButton().

Assignee

Comment 14

•

9 years ago

Andrew, James
I have tested your solutions but both of them do not work.
I have found  that the problem is in the onSearchButton function. I have commented the first condition and the bug does not exist anymore. 

onSearchButton: function() {
      // Do not bother if there is no current folder.
      /*
      if (!this.curFolder) {
        return;
      }
      */

      cards.pushCard(
        'message_list_search', 'animate',
        {
          folder: this.curFolder
        });
    },

Assignee

Updated

•

9 years ago

Flags: needinfo?(jrburke)

Comment 15

•

9 years ago

Apologies for the delay: I was sick for a few days then on a trip. I think this change fixes the issue:

diff --git a/apps/email/js/cards/message_list.html b/apps/email/js/cards/message_list.html
index 6835fc4..f3d269f 100644
--- a/apps/email/js/cards/message_list.html
+++ b/apps/email/js/cards/message_list.html
@@ -98,6 +98,7 @@
           class="msg-search-tease-bar msg-nonsearch-only">
       <p>
         <input data-event="focus:onSearchButton"
+               data-prop="searchTextTease"
                class="msg-search-text-tease" type="text"
                dir="auto"
                data-l10n-id="message-search-input" />
diff --git a/apps/email/js/cards/message_list.js b/apps/email/js/cards/message_list.js
index 8fdf2cd..00d984e 100644
--- a/apps/email/js/cards/message_list.js
+++ b/apps/email/js/cards/message_list.js
@@ -1555,6 +1555,14 @@ return [
         if (inboxFolder === this.curFolder) {
           evt.emit('inboxShown', account.id);
         }
+
+        // If user tapped in search box on message_list before the JS for the
+        // card is attached, then treat that as the signal to go to search. Only
+        // do this when first starting up though.
+        if (this.mode === 'nonsearch' &&
+            document.activeElement === this.searchTextTease) {
+          this.onSearchButton();
+        }
       }
     },
 
The approach is to only do the work in onFolderShown and in the part that waits for a folder to exist.

Just testing for this.mode and the activeElement should be enough. Since the cards infrastructure .blur()s focused elements on card transitions, and this logic only takes place on folder changes, I believe this correctly scopes the check.

Robert, how does that look to you?

Flags: needinfo?(jrburke)

Assignee

Updated

•

9 years ago

Attachment #8573215 - Flags: review?(jrburke)

Assignee

Comment 16

•

9 years ago

(In reply to James Burke [:jrburke] from comment #15)
> Robert, how does that look to you?
yes, I have tested it many times and it seems work well.

Comment 17

•

9 years ago

Comment on attachment 8573215 [details] [review]
[gaia] rsajdok:bug#1137486 > mozilla-b2g:master

Looks good. I will wait until the treeherder test run completes, then ask for checkin-needed to get autolander to land it.

Attachment #8573215 - Flags: review?(jrburke) → review+

Updated

•

9 years ago

Keywords: checkin-needed

Updated

•

9 years ago

Keywords: checkin-needed

Comment 18

•

9 years ago

http://docs.taskcluster.net/tools/task-graph-inspector/#e6ADKCE1Qvu26wCLvo6JwA

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 19

•

9 years ago

Trying again, as failures unrelated to this patch.

Keywords: checkin-needed

Updated

•

9 years ago

Keywords: checkin-needed

Comment 20

•

9 years ago

http://docs.taskcluster.net/tools/task-graph-inspector/#Xv5YmmDyToakRx-r9uX8gA

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 21

•

9 years ago

Robert, you might want to try rebasing over latest gaia master to see if that helps fix the autolander issue with some tests failing (not related to this patch). Just needinfo? me when you do and I will ask autolander to try landing it again.

Updated

•

9 years ago

Flags: needinfo?(robert.sajdok)

Assignee

Comment 22

•

9 years ago

done

Flags: needinfo?(robert.sajdok) → needinfo?(jrburke)

Updated

•

9 years ago

Flags: needinfo?(jrburke)

Keywords: checkin-needed

Updated

•

9 years ago

Keywords: checkin-needed

Comment 23

•

9 years ago

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/faf61ac3470ba311a6563046959d6fe6505fded3