852805 - Defect - Adding "Enter" shortcut while using search (CTRL + F)

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Browser, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

When using the search function, pressing on "Enter" will not move down to the next instance of the searched word.

Steps to reproduce the issue:

1) Open Firefox Metro
2) Go to a website (Wikipedia probably the easiest)
3) Select "CTRL + F" and insert a common word (that, the etc..)
4) Press "Enter"

Actual Results:

- Nothing happens when pressing "Enter"

Expected Results:

- Searches for the next instance of the word on the page

Comment 1

[Brian R. Bondy [:bbondy]]

This is not directly mentioned in the story for bug 831940 but it's a pretty bad user experience without it. Easy to implement, maybe defect for it5?

Comment 2

[Brian R. Bondy [:bbondy]]

Please remove from it5 if you disagree.

Comment 3

[Marco Mucci [:MarcoM]]

Moving to Iteration #5 for consideration as a defect.

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

Comment 6

[Allison Naaktgeboren :ally]

Comment on attachment 733010 [details] [diff] [review]
Patch v1

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

I'm not sure what you need a front-end specific review for this, but it looks reasonable enough to me.

- Do we have a followup bug for adding tests for find in page behavior anywhere? If we don't, we should consider filing one

- Do we need to worry about removing the eventListener?

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to :Ally Naaktgeboren from comment #6)
> Comment on attachment 733010 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 733010 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure what you need a front-end specific review for this, but it
> looks reasonable enough to me.
> 
> - Do we have a followup bug for adding tests for find in page behavior
> anywhere? If we don't, we should consider filing one

I'll look around and file a bug if one doesn't already exist (and will post back here)

> - Do we need to worry about removing the eventListener?

My instinct is 'no' since we want this event listener to be around for the life of the text box, but I'll check and see whether we're removing other event listeners and follow suit.

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Tim Abraldes (:tabraldes) (:TimAbraldes) from comment #7)
> (In reply to :Ally Naaktgeboren from comment #6)
> > Comment on attachment 733010 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 733010 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm not sure what you need a front-end specific review for this, but it
> > looks reasonable enough to me.
> > 
> > - Do we have a followup bug for adding tests for find in page behavior
> > anywhere? If we don't, we should consider filing one
> 
> I'll look around and file a bug if one doesn't already exist (and will post
> back here)

Filed bug 857840.  Also this made me realize that this patch doesn't implement "shift+enter" to go to the previous occurrence.  I'll update the patch.

> > - Do we need to worry about removing the eventListener?
> 
> My instinct is 'no' since we want this event listener to be around for the
> life of the text box, but I'll check and see whether we're removing other
> event listeners and follow suit.

Now that I've looked at other event listeners, I'm pretty sure the answer is 'no'

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

Updated to include "shift+enter" to go to previous match.  Carrying forward r+

For some reason my local builds are freezing.  Once I figure that out, I'll test this and land on inbound.

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

The freezing issue magically went away this morning.  Tested patch and 'enter' and 'shift+enter' both seem to work as expected.  Landing patch:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/f6ba788a7eea

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6ba788a7eea

Comment 12

[juan becerra [:juanb]]

Tested on 2013-04-08 using a Nightly built from http://hg.mozilla.org/mozilla-central/rev/b0d842380959
- Tested with steps in comment #0 and that works now. I can cycle through the instances of the searched word, and I can also use ctrl-g to do this.
- Can search previous instances when I combine the Enter or ctrl-g with the shift key.

Comment 13

[Kamil Jozwiak [:kjozwiak]]

Went through the following "Defect" for iteration #5 testing and found an issue. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-04-14-03-10-25-mozilla-central/

Went through the following test cases:

- Went through the test cases outlined in Comment 0
- Went through the test cases outlined in Comment 12

When removing a term that you searched for in the "Search" field using the backspace, you will still be able to search for the same term while pressing "Enter" even if the "Search" text box is completely blank

Steps to reproduce the issue:

1) Open Firefox Metro
2) Go to wikipedia.com and use the "Search" feature to look for a common word (You, The, etc..)
3) Once you have found several instances of the searched word, select the "Search" text box and remove the current word using the backspace
4) Once removed (the "Search" text box should be empty), press "Enter" several times and you will notice it will keep searching for the same term.

Current Behavior:

- Removing the word you are searching for by using the backspace, and then pressing "Enter" will keep searching for the word even if the "Search" field is blank

Expected Behavior:

- Should stop finding anything when the "Search" field is blank. CTRL + G behaves correctly when you remove the your search term

Please let me know if I should have created a new ticket other then re-opening this one. Re-opened this one as the issue originated from the this defect

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Generally we try to keep to "one patch (or group of patches) landed per bug".  It's easy to file new bugs for regressions, but things get complicated if we reopen old bugs for issues that aren't exactly what the original bug describes.

I've filed bug 863060 to track the progress on the new issue.

Comment 15

[Marco Mucci [:MarcoM]]

For Kamil to test and verify.

Comment 16

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

err.. this is already verified.  The new bug that should be verified is bug 863060 (when the patch for that lands)

Comment 17

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

When I landed the patch for bug 863060, I accidentally made the commit message reference this bug:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/77eba8ff0a5f

That commit is, in fact, for bug 863060

Comment 18

[Kamil Jozwiak [:kjozwiak]]

(In reply to Tim Abraldes (:tabraldes) (:TimAbraldes) from comment #14)
> Generally we try to keep to "one patch (or group of patches) landed per
> bug".  It's easy to file new bugs for regressions, but things get
> complicated if we reopen old bugs for issues that aren't exactly what the
> original bug describes.
> 
> I've filed bug 863060 to track the progress on the new issue.

Okay, thanks Tim! Will create new bugs if the issues are not exactly related to the original bug description

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/77eba8ff0a5f

Comment 20

[Kamil Jozwiak [:kjozwiak]]

The following "Defect" failed for iteration #12 testing. Used the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-08-20-03-02-06-mozilla-central/

- Went through the original test case in comment #0 and failed as pressing "Enter" will not search for the next search term in the search filed. Tried this several times and ran into the same issue. Bug 905808 was already created

Comment 21

[Kamil Jozwiak [:kjozwiak]]

Went through the following defect for iteration #20 without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-23-03-02-03-mozilla-central/

- Went through the original test case that has been added in comment #0
- Went through Bug 905808 and used the same test cases (both issues are basically the same)
- Ensured that "Shift + Enter" worked as mentioned in comment # 9
- Ensured that pressing "Enter" or the up/down icons doesn't search for the previously used word when the text box is blank as per comment # 13
- Used the "Enter" key to search through the entire website without any issues (scrolled through using "Enter" several times)
- Ensured that pressing "Enter" only worked while the cursor was focused in the Search App Bar text box
- Ensured that both bottom/top and top/bottom transitions worked without any issues (smooth, no jank etc..)
- Ensured that you can still use both of the arrow icons (up/down) under the Search App Bar
- Ensured that the "X" dismissed the Search App Bar (also made sure it dismisses correctly while the OSK is visible)
- Ensured that pressing "CTRL + F" slides in the Search App Bar
- Ensured that pressing "ESC" dismisses the Search App Bar
- Ensured that the OSK appears/dismisses correctly when tapping in the text box under the Search App Bar
- Ensured that the background colour in the text box turns red when a word hasn't been found (made sure pressing enter didn't do anything when a word can't be found on that particular page)
- Ensured that taping on the "X" in the text box clears the current word that's being searched
- Ensured that all of the above test cases also work under filled view