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

VERIFIED FIXED in Firefox 23

Status

Firefox for Metro
Browser
P1
normal
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: kjozwiak, Assigned: TimAbraldes)

Tracking

Trunk
Firefox 23
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=Find_in_page_app_bar u=metro_firefox_user p=1 status=verified)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
Whiteboard: p=1
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?
Blocks: 831940
Flags: needinfo?(asa)
Please remove from it5 if you disagree.
Blocks: 850809
No longer blocks: 841214
Moving to Iteration #5 for consideration as a defect.
Flags: needinfo?(asa)
Priority: -- → P1
QA Contact: jbecerra
Summary: adding "Enter" shortcut while using search (CTRL + F) → Defect - Adding "Enter" shortcut while using search (CTRL + F)
Whiteboard: p=1 → feature=defect c=Find_in_page_app_bar u=metro_firefox_user p=1

Updated

5 years ago
Duplicate of this bug: 855660

Updated

5 years ago
Component: Shell → Browser
Version: unspecified → Trunk
Assignee: nobody → tabraldes

Updated

5 years ago
Status: NEW → ASSIGNED
Created attachment 733010 [details] [diff] [review]
Patch v1
Attachment #733010 - Flags: review?(mbrubeck)
Attachment #733010 - Attachment is patch: true
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?
Attachment #733010 - Flags: review?(mbrubeck) → review+
(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.
(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'
Created attachment 733122 [details] [diff] [review]
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.
Attachment #733010 - Attachment is obsolete: true
Attachment #733122 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/f6ba788a7eea
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23

Updated

5 years ago
Flags: needinfo?(jbecerra)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(jbecerra)
Whiteboard: feature=defect c=Find_in_page_app_bar u=metro_firefox_user p=1 → feature=defect c=Find_in_page_app_bar u=metro_firefox_user p=1 status=verified
(Reporter)

Comment 13

5 years ago
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
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Updated

5 years ago
Blocks: 855905
No longer blocks: 850809
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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
For Kamil to test and verify.
Flags: needinfo?(kamiljoz)
err.. this is already verified.  The new bug that should be verified is bug 863060 (when the patch for that lands)
Status: RESOLVED → VERIFIED
Flags: needinfo?(kamiljoz)
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
(Reporter)

Comment 18

5 years ago
(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
(Reporter)

Comment 20

5 years ago
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

Updated

5 years ago
Blocks: 909477
(Reporter)

Comment 21

5 years ago
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
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.