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)
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)
(deleted),
patch
|
mbrubeck
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
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•11 years ago
|
Whiteboard: [preview-triage] feature=defect c=tbd u=tbd p=0
Comment 1•11 years ago
|
||
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•11 years ago
|
||
Quick note, tapping on the "Search/Enter" using the OSK has the same results as comment #0
Updated•11 years ago
|
Blocks: MetroPreviewRelease
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•11 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.
Updated•11 years ago
|
Assignee: nobody → msamuel
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•11 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•11 years ago
|
||
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 6•11 years ago
|
||
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•11 years ago
|
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•11 years ago
|
||
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•11 years ago
|
Flags: needinfo?(mbrubeck)
Comment 8•11 years ago
|
||
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•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #812278 -
Flags: review?(mbrubeck) → review+
Updated•11 years ago
|
Flags: needinfo?(mbrubeck)
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Updated•11 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
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
Comment 12•11 years ago
|
||
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
Comment 13•11 years ago
|
||
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•11 years ago
|
||
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
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•