If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Defect - Enter key doesn't work for find bar

VERIFIED FIXED in Firefox 27

Status

Firefox for Metro
General
P2
normal
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Samvedana, Assigned: emtwo)

Tracking

({regression})

Trunk
Firefox 27
All
Windows 8.1
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=find_in_page_app_bar u=metro_firefox_user p=3)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130815030203
Built from http://hg.mozilla.org/mozilla-central/rev/a8daa428ccbc

STR and Expected result
1. Open Mozilla wiki in Metro Firefox.
2. Open find bar.
3. Type word "Mozilla". First "Mozilla" word should highlight in Mozilla wiki.
4. Make sure that cursor pointer is still in find bar and press enter. This should highlight next word in Mozilla wiki like Desktop Firefox.

Actual result
Next word didn't highlight after pressing enter key after step-4.

Updated

4 years ago
Whiteboard: [preview-triage] feature=defect c=tbd u=tbd p=0
This is a regression from bug 890153.  The findbar stops receiving key events because we blur it after the first "enter".  Instead we should find a way to let it keep focus, like in desktop Firefox, without regressing bug 890153.
Blocks: 890153
Keywords: regression
Hardware: x86 → All
Quick note, tapping on the "Search/Enter" using the OSK has the same results as comment #0

Updated

4 years ago
Blocks: 899390
Summary: Defect - Enter key doesn't work for find bar → [MP] Defect - Enter key doesn't work for find bar
Whiteboard: [preview-triage] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=0
(Reporter)

Comment 3

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130823030204
Built from http://hg.mozilla.org/mozilla-central/rev/fb2318875cd4

WFM
Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0, I am still seeing this issue.
(Reporter)

Updated

4 years ago
Blocks: 909477

Updated

4 years ago
Duplicate of this bug: 909477

Updated

4 years ago
Assignee: nobody → msamuel
Blocks: 909637
No longer blocks: 838081
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [preview] feature=defect c=tbd u=tbd p=0 → [preview] feature=defect c=tbd u=tbd p=3

Updated

4 years ago
Whiteboard: [preview] feature=defect c=tbd u=tbd p=3 → [preview] feature=defect c=find_in_page_app_bar u=metro_firefox_user p=3
(Assignee)

Comment 5

4 years ago
Created attachment 810190 [details] [diff] [review]
v1: Reuse browser shift code from SelectionHandler to center findbar searches

I think this does what we want. I tested it with google.com searching for 'google' will shift the browser to put the search in focus. There is only one issue which I'm still looking into:

This looks jumpy on sites that are scrollable because we already scroll to focus when handling the 'FindAssist:Show' event. We need a way to know that 'FindAssist:Show' was not able to scroll to focus and only shift the browser in that case. Please let me know if you happen to know a good way of doing this :) Thanks!
Attachment #810190 - Flags: feedback?(mbrubeck)
Comment on attachment 810190 [details] [diff] [review]
v1: Reuse browser shift code from SelectionHandler to center findbar searches

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

I like the approach, and the jumping doesn't seem too bad since we're already scrolling around as you type.

I still sometimes end up with the find bar itself covering up elements near the bottom of a scrollable page (e.g. search for "Mefi Wiki" on http://metafilter.com ), but it looks like this might be a separate problem; it happens even if I'm using the hardware keyboard so the on-screen keyboard never appears.

::: browser/metro/base/content/Util.js
@@ +266,5 @@
>      delete this.displayDPI;
>      return this.displayDPI = this.getWindowUtils(window).displayDPI;
>    },
>  
> +  centerElementInView: function centerElementInView(aViewHeight, anElement) {

Please add a comment describing the arguments and what the return value means.

Naming nit: Please change anElement to "aRect" or something (since it's not a DOM Element).

@@ +294,5 @@
> +      let distanceToCenter =
> +        distanceFromChromeTop + Math.min(distanceToPageBounds, splitMargin);
> +      return distanceToCenter;
> +    }
> +    return -1;

Should we just return 0 here?  Or do we
Attachment #810190 - Flags: feedback?(mbrubeck) → feedback+

Updated

4 years ago
Blocks: 915232
No longer blocks: 899390, 909637
Summary: [MP] Defect - Enter key doesn't work for find bar → Defect - Enter key doesn't work for find bar
Whiteboard: [preview] feature=defect c=find_in_page_app_bar u=metro_firefox_user p=3 → feature=defect c=find_in_page_app_bar u=metro_firefox_user p=3
(Assignee)

Comment 7

4 years ago
Created attachment 812203 [details] [diff] [review]
v2: Reuse browser shift code from SelectionHandler to center findbar searches

Made the suggested changes + some findings on what's going on with findbar covering the text.

Basically there are a few things I tried (out of intuition/trial & error) that seemed to fix the issue with the findbar but they are not a solution. Matt, perhaps you have some idea of what may be going on here, but if not, I can try to investigate it more or file another bug for it.

You can see in the patch that I commented out the call to 'fuzzyZoom()' because it looks like it was resulting in scrolling jumpiness when focusing on text underneath the findbar. Then do the following:

1) Go to http://www.metafilter.com/ open the findbar
2) Open the settings flyout then tap outside to close it
3) Search for 'Mefi Wiki' => should be visible above the findbar

My guess would be that there is some kind of refresh that is done when opening/closing a flyout that helps in this case and that fuzzyZoom() isn't getting called at quite the right time/place. What do you think, Matt? Thanks!
Attachment #810190 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Flags: needinfo?(mbrubeck)
Comment on attachment 812203 [details] [diff] [review]
v2: Reuse browser shift code from SelectionHandler to center findbar searches

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

::: browser/metro/base/content/AnimatedZoom.js
@@ +63,5 @@
>      let scale = this.startScale * zoomRatio;
>      let scrollX = nextRect.left * zoomRatio;
>      let scrollY = nextRect.top * zoomRatio;
>  
> +    //this.browser.fuzzyZoom(scale, scrollX, scrollY);

I haven't looked closely, but this might be causing problems with this patch because AnimatedZoom.start gets the getBoundingClientRect of the <browser> at the start of the animation, and then continues using those coordinates as it calculates each frame of the animation.  With this patch, we might be moving the <browser> after the animation starts, so later frames are using out-of-date coordinates.

Also note:  AnimatedZoom is all but abandoned (I think FindHelperUI is the only live code that still uses it) and not really implemented properly on Metro (fuzzyZoom just scrolls in Metro; it's just a fancy way of saying scrollTo).  It's tempting to just get rid of AnimatedZoom and figure out a simpler way to scroll find-in-page results into view -- maybe just by scrolling directly, or maybe using the ZoomToRect message that ally is working on for double tap (bug 895581).
(Assignee)

Comment 9

4 years ago
Created attachment 812278 [details] [diff] [review]
v3: Reuse browser shift code from SelectionHandler to center findbar searches

Left AnimateZoom as it is for now. Bug 922337 and bug 922345 for issues related to AnimatedZoom.
Attachment #812203 - Attachment is obsolete: true
Attachment #812278 - Flags: review?(mbrubeck)
Attachment #812278 - Flags: review?(mbrubeck) → review+
Flags: needinfo?(mbrubeck)
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/1984edf457c1
(Assignee)

Updated

4 years ago
Whiteboard: feature=defect c=find_in_page_app_bar u=metro_firefox_user p=3 → [fixed-in-fx-team]feature=defect c=find_in_page_app_bar u=metro_firefox_user p=3
https://hg.mozilla.org/mozilla-central/rev/1984edf457c1
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]feature=defect c=find_in_page_app_bar u=metro_firefox_user p=3 → feature=defect c=find_in_page_app_bar u=metro_firefox_user p=3
Target Milestone: --- → Firefox 27
Mozilla/5.0 (Windows NT 6.2; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Windows NT 6.2; rv:27.0) Gecko/20100101 Firefox/27.0
Build ID: 20131016030202

Verified using the STR provided in comment 0, and the next word was highlight after pressing enter key until the last one (bottom of the page) and after that, pressing enter will start over from the top of the page.
Status: RESOLVED → VERIFIED
Created attachment 8333798 [details]
screenshot.png

While testing this for iteration #18, I've found the following issue:

- on Win 8 64-bit, with latest Nightly (build ID: 20131117030203), after pressing enter key until I get to the last word (at the bottom of the page), almost half of the lower part of the screen becomes black. Please see the attached screenshot for details.
Went through the following defect for iteration #20 testing without any issues. Used the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-22-03-02-04-mozilla-central/

I'm pretty certain that comment # 13 is not related to the original issue described in comment #0. We had several issues with the screen "going black" when users scrolled through websites. This issue was addressed in Bug 915811.

I went through this defect on a Windows 8.1 touch laptop without any of the repainting issues and ensured that the original issue was working as expected:

- Went through the original test case that has been added in comment #0
- 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.