[Email][Search] Selecting the search bar before the Email app is finished loading will open the keyboard, but not the search page

VERIFIED FIXED

Status

Firefox OS
Gaia::E-Mail
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: DerekH, Assigned: robert.sajdok)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(tracking-b2g:backlog, b2g-v2.0 affected, b2g-v2.1 affected, b2g-v2.2 affected, b2g-v2.5 verified, b2g-master verified)

Details

(Whiteboard: [3.0-Daily-Testing], URL)

Attachments

(4 attachments)

Created attachment 8570170 [details]
Email Search Bar Logcat

Description:
If the user quickly selects the search bar before the app is finished loading, they can bring up the keyboard on the email inbox page. If the user starts typing in the search bar on the inbox page and then taps on the search bar again, they will be brought to the search page, but the characters previously typed will not appear on this page. If the user dismisses the search page to return to the inbox, the characters typed there before will still be there but the user cannot remove those characters until the Email app is force closed

Prerequisites: Have an email account signed in

Repro Steps:
1) Update a Flame to 20150226010233
2) Open the Email app
3) Quickly scroll up while the app is loading to reveal the search bar, and tap it
4) Begin typing
 

Actual:
The keyboard is up and the user is typing in the search bar, but they are not on the search page.

Expected:
The user cannot access the search bar until the app finishes loading

Environmental Variables:
Device: Flame 3.0 (319mb)(Kitkat)(Full Flash)
Build ID: 20150226010233
Gaia: 7894b929f1b0394f3c997f72a6482bc7813e758d
Gecko: dd6353d61993
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0


Repro frequency: 3/3 (More of a timing bug)
See attached: Logcat, Video - http://youtu.be/8UG8eFyCQ60
This issue DOES occur on Flame 2.2, 2.1, 2.0, and Base v18D-1

The keyboard is up and the user is typing in the search bar, but they are not on the search page.

Environmental Variables:
Device: Flame 2.2 (319mb)(Kitkat)(Full Flash)
Build ID: 20150226002503
Gaia: bf24aa57fa7760260ab05d1f53242c8d8ae59e83
Gecko: 363123044e61
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


Environmental Variables:
Device: Flame 2.1 (319mb)(Kitkat)(Full Flash)
Build ID: 20150226001720
Gaia: 5d3479fdd438412adee4452720856b6b771fe5cd
Gecko: 0390c73a827b
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0


Environmental Variables:
Device: Flame 2.0
Build ID: 20150225000239
Gaia: 366aaa19ac474dc58b79d62a91cff41756ae9dfe
Gecko: 611444d72a92
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 32.0 (2.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0


Environmental Variables:
Device: Flame v18D-1 (319mb)(Kitkat)
Build ID: 20150106124450
Gaia: 79f6218c4f30c2739575c3ab800078c2cda135cb
Gecko: d9d4000dd43a3637345a41d716dc97fdd700d715
Version: 32.0 (2.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
NI on component owner for nomination decision and assignment.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga) → needinfo?(twen)

Comment 3

3 years ago
Does not block major functions, nominate for tracking.
tracking-b2g: --- → ?
Flags: needinfo?(twen)
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

3 years ago
Assignee: nobody → robert.sajdok
Flags: needinfo?(robert.sajdok)
Created attachment 8573215 [details] [review]
[gaia] rsajdok:bug#1137486 > mozilla-b2g:master
(Assignee)

Updated

3 years ago
Attachment #8573215 - Flags: review?(jrburke)
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

3 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)
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)
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?
That sounds fine to me, feels nicer too. What do you think Robert?
(Assignee)

Comment 11

3 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"
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.
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

3 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

3 years ago
Flags: needinfo?(jrburke)
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

3 years ago
Attachment #8573215 - Flags: review?(jrburke)
(Assignee)

Comment 16

3 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 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

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
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.
Trying again, as failures unrelated to this patch.
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
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.
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

3 years ago
Flags: needinfo?(robert.sajdok)
(Assignee)

Comment 22

3 years ago
done
Flags: needinfo?(robert.sajdok) → needinfo?(jrburke)

Updated

3 years ago
Flags: needinfo?(jrburke)
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Comment 24

3 years ago
Please uplift this to V2.2 as well if there is no issue with it.

Thank you
Flags: needinfo?(jrburke)
Unfortunately we're too late in the 2.2 release cycle to approve uplifts for non-blocking bugs. This one isn't going to meet the criteria for blocking so we'll just leave it on master at this point.
Flags: needinfo?(jrburke)
This bug has been verified as "pass" on the latest build of Flame 2.5&master and Aries KK 2.5&master by the STR in comment 0.

See attachment: verified_Flame_v2.5.3gp and verified_Aries_v2.5.3gp
Reproduce rate: 0/10

Actual results: 
On Flame v2.5&master (512mb):
    Quickly scrolling up to reveal the search bar -> quickly tapping it -> very quickly typing the characters, the characters typed are left into the search input box at the top of email list (the characters typed can't be removed until the Email app is force closed), then it automatically focus/turns to search bar page successfully.

On Aries v2.5&master:
    Quickly scrolling up to reveal the search bar -> quickly tapping it -> we have no time to typing until it automatically focus/turns to search bar page successfully.


-------------------------------------------------------------------------------
Device: Flame KK master_512mb eng (Pass)
Build ID               20151201150223
Gaia Revision          59c8605876736b22acaaed25be00008e452149cb
Gaia Date              2015-12-01 02:58:37
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1003fca97839fdfbfefb89c48c9e05a940bc9fb9
Gecko Version          45.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20151201.183010
Firmware Date          Tue Dec  1 18:30:23 EST 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK master eng (Pass)
Build ID               20151202015416
Gaia Revision          7847a3c1b28e039631509978518b36fd3c5f9585
Gaia Date              2015-12-01 18:47:31
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/470f4f8c2b2d6f82e56e161a4b05262c85f55b59
Gecko Version          45.0a1
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151202.011126
Firmware Date          Wed Dec  2 01:11:35 UTC 2015
Bootloader             s1

Device: Flame KK 2.5_512mb user (Pass)
Build ID               20151201163815
Gaia Revision          07462becf08f0c26ebd64daf89646e7403a336c5
Gaia Date              2015-12-01 15:06:08
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/33a575e711faf3344aa2e31ca2ea066b4cd8aafa
Gecko Version          44.0a2
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151201.154956
Firmware Date          Tue Dec  1 15:50:05 UTC 2015
Firmware Version       v18D v4
Bootloader             L1TC000118D0

Device: Aries KK 2.5 dogfood (Pass)
Build ID               20151201171338
Gaia Revision          e05621cbfd92b3fb4e5aef86621c57cd68fb0414
Gaia Date              2015-12-01 15:07:53
Gecko Revision         http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/89b4f90d2865e62b9d898655cf082902cf2572a0
Gecko Version          44.0a2
Device Name            aries
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.worker.20151201.161919
Firmware Date          Tue Dec  1 16:19:27 UTC 2015
Bootloader             s1
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
status-b2g-v2.5: --- → verified
status-b2g-master: affected → verified
Created attachment 8694641 [details]
verified_Aries_v2.5.3gp
Created attachment 8694642 [details]
verified_Flame_v2.5.3gp
You need to log in before you can comment on or make changes to this bug.