Open Bug 952042 Opened 12 years ago Updated 3 years ago

Focus ring remains forever, and indicate wrong element

Categories

(Toolkit :: Find Toolbar, defect, P5)

Product:
Toolkit
Component:
Find Toolbar
Version:
27 Branch
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox26 --- unaffected
firefox27 + wontfix
firefox28 + fixed
firefox29 + fixed

People

(Reporter: alice0775, Unassigned)

References

Details

(Keywords: regression, Whiteboard: [leave open])

Attachments

(3 files)

screenshot
11.28 KB, image/png
Details
remove-outline
978 bytes, patch
mikedeboer
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review
test
2.37 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
The problem happens since Firefox beta27 Steps To Reproduce: 1. Open this page 2. Key press "/" without "" (Quick find bar will opens) 3. Key press "home" without "" ---- observe "Home" link is focused 4. Key press Ecs ---- (Quick find bar will close) 5. Key press Tab Or Click <input>, <textarea> and/or other element so that focus gets other element Actual Results: The focus ring remains and indicate wrong element. Expected Results: The focus ring should disappear. Regression window(m-c) Good: http://hg.mozilla.org/mozilla-central/rev/753a91f79d6f Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131031170043 Bad: http://hg.mozilla.org/mozilla-central/rev/4813677c42bf Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131031180143 Psuhlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=753a91f79d6f&tochange=4813677c42bf Regression window(m-i) Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/4dc708bb39cb Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131029120449 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/f91d1ac2bc2c Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131029123031 Psuhlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4dc708bb39cb&tochange=f91d1ac2bc2c Regressed by: f91d1ac2bc2c Tom Schuster — Bug 928619 - Stop removing the findbar selection after it closes. r=mikedeboer
Severity: major → normal
Component: Layout: View Rendering → Find Toolbar
OS: Windows 7 → All
Product: Core → Toolkit
Hardware: x86_64 → All
Attached image screenshot —
Assignee: nobody → evilpies
I don't even know anymore what to think, in some case we don't want the highlight to go away, and now the focus ring never disappears. :( I already forgot how most of this works.
NI on Mike to help with the right path forward as this is tracking for release due to regression. Mike given you helped with review and ideas here to help before the ship of Firefox 27 ?
Flags: needinfo?(mdeboer)
I can remove the focus ring around the element everytime we blur or close the findbar, but that doesn't seem like the right thing to do?
(In reply to Tom Schuster [:evilpie] from comment #4) > I can remove the focus ring around the element everytime we blur or close > the findbar, but that doesn't seem like the right thing to do? I wouldn't recommend this, especially considering the quick find bar always closes, which would mean the focus ring would be always removed. If the user wants to hit enter, then he won't know where he's hitting enter at for example, if the FB closes before he sees it, which in turn means extra unnecessary F3/reopening FB/new search/etc steps.
I understand where Tom is coming from; the expected behavior is not well-defined, at least not from reading the code, nor from any type of (non-)existing documentation. I'd like to come up with a coherent spec here of the desired behavior, before we get into patching and yield more bugs in the process. Or does such a 'spec' exist already?
Flags: needinfo?(mdeboer)
Attached patch remove-outline — — Splinter Review
Oh damn! In bug 928619, we changed removeSelection to enableSelection. removeSelection used to restoreOriginalOutline, which is that fake outline that stays around after we now close the findbar. So we just have to call it in enableSelection.
Attachment #8362008 - Flags: review?(mdeboer)
Attached patch test — — Splinter Review
Attachment #8362021 - Flags: review?(mdeboer)
Comment on attachment 8362008 [details] [diff] [review] remove-outline Review of attachment 8362008 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks!
Attachment #8362008 - Flags: review?(mdeboer) → review+
Comment on attachment 8362021 [details] [diff] [review] test Review of attachment 8362021 [details] [diff] [review]: ----------------------------------------------------------------- Adding this test earns you 3 brownie points!
Attachment #8362021 - Flags: review?(mdeboer) → review+
Tom, I'm happy you caught this, as I was already going the spec-route! Do you have time to do the checkin and uplift-request dance? I'd be happy to take of that, if not.
Status: NEW → ASSIGNED
Request to make sure to land on inbound/central and request approval on this by tomorrow in preparation for Thursday Beta(final beta for Fx27) if low risk
The test seems to sadly fail on every platform: https://tbpl.mozilla.org/?tree=Try&rev=f1691fc80327. However for Mike and me it works locally. Probably something going wrong the focus.
Marking this as wontfix given this did not make the beta cut-off. Wontfixing for Fx27 at this point and lets get this uplifted to aurora so this can be resolved in Fx28.
Comment on attachment 8362008 [details] [diff] [review] remove-outline [Approval Request Comment] Bug caused by (feature/regressing bug #): 928619 User impact if declined: user may see an incorrectly positioned element focus ring after using quickfind. Testing completed (on m-c, etc.): multiple days on m-c Risk to taking this patch (and alternatives if risky): minor. String or IDL/UUID changes made by this patch: n/a.
Attachment #8362008 - Flags: approval-mozilla-aurora?
Attachment #8362008 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/12c9107dd050 So this is still open because we're waiting on a test?
status-firefox28: affected → fixed
status-firefox29: affected → fixed
Flags: needinfo?(evilpies)
Flags: in-testsuite?
Yes, any idea who could help with this test?
Flags: needinfo?(evilpies)
It just occurred to me that you might know how to fix this test.
Flags: needinfo?(bugs)
In which way does the test fail? Try results seem to be gone. Should something happen async in the test?
Flags: needinfo?(bugs)
Okay so the problem is that sending TAB doesn't focus the next element.
Would you mind?
Flags: needinfo?(bugs)
(In reply to Tom Schuster [:evilpie] from comment #24) > Okay so the problem is that sending TAB doesn't focus the next element. Is it? That tryserver error hints that is(focusedElement.textContent, "test link", "Link should be focused"); throws.
Flags: needinfo?(bugs)
I pushed the test to try again with the same result: https://tbpl.mozilla.org/?tree=Try&rev=470a3e14583f. I am not sure how to fix this.
Realistically I am not going to fix this up.
Assignee: evilpies → nobody
Status: ASSIGNED → NEW
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: