Closed
Bug 1137486
Opened 9 years ago
Closed 9 years ago
[Email][Search] Selecting the search bar before the Email app is finished loading will open the keyboard, but not the search page
Categories
(Firefox OS Graveyard :: Gaia::E-Mail, defect)
Tracking
(tracking-b2g:backlog, b2g-v2.0 affected, b2g-v2.1 affected, b2g-v2.2 affected, b2g-v2.5 verified, b2g-master verified)
VERIFIED
FIXED
tracking-b2g | backlog |
People
(Reporter: dharris, Assigned: robert.sajdok)
References
()
Details
(Whiteboard: [3.0-Daily-Testing])
Attachments
(4 files)
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
Reporter | ||
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
NI on component owner for nomination decision and assignment.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga) → needinfo?(twen)
Comment 3•9 years ago
|
||
Does not block major functions, nominate for tracking.
tracking-b2g:
--- → ?
Flags: needinfo?(twen)
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
|
||
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)
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?
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.
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)
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
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 24•9 years ago
|
||
Please uplift this to V2.2 as well if there is no issue with it. Thank you
Flags: needinfo?(jrburke)
Comment 25•9 years ago
|
||
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)
Comment 26•9 years ago
|
||
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
Comment 27•9 years ago
|
||
Comment 28•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•