905808 - Defect - Enter key doesn't work for find bar

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Samvedana (:Samvedana)]

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.

Comment 1

[Matt Brubeck (:mbrubeck)]

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.

Comment 2

[Kamil Jozwiak [:kjozwiak]]

Quick note, tapping on the "Search/Enter" using the OSK has the same results as comment #0

Comment 3

[Samvedana (:Samvedana)]

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.

Comment 5

[Marina Samuel [:emtwo]]

Attachment: 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!

Comment 6

[Matt Brubeck (: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

Comment 7

[Marina Samuel [:emtwo]]

Attachment: 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!

Comment 8

[Matt Brubeck (: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).

Comment 9

[Marina Samuel [:emtwo]]

Attachment: 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.

Comment 10

[Marina Samuel [:emtwo]]

https://hg.mozilla.org/integration/fx-team/rev/1984edf457c1

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1984edf457c1

Comment 12

[Cornel Ionce [:noni] [Hubs QA]]

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.

Comment 13

[Manuela Muntean [Away]]

Attachment: 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.

Comment 14

[Kamil Jozwiak [:kjozwiak]]

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