Closed Bug 905808 Opened 11 years ago Closed 11 years ago

Defect - Enter key doesn't work for find bar

Categories

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

All
Windows 8.1
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 27

People

(Reporter: Samvedana, Assigned: emtwo)

References

Details

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

Attachments

(2 files, 2 obsolete files)

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.
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
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
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.
Blocks: 909477
Assignee: nobody → msamuel
Blocks: metrov1it15
No longer blocks: metrov1backlog
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
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
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+
Blocks: metrov1it16
No longer blocks: MetroPreviewRelease, metrov1it15
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
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
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).
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)
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
Status: ASSIGNED → RESOLVED
Closed: 11 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
Attached image screenshot.png (deleted) —
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.

Attachment

General

Created:
Updated:
Size: