Closed Bug 1285464 Opened 3 years ago Closed 3 years ago

When find toolbar hides, it focuses the window

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 + fixed

People

(Reporter: bzbarsky, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(1 file)

STEPS TO REPRODUCE:

1)  Ensure that "accessibility.typeaheadfind" is true in about:config.
2)  Open two windows.
3)  Load https://bugzilla.mozilla.org/show_bug.cgi?id=1280978 in
    the first window.
4)  Focus the first window.
5)  Type "step" (this will search for that string)
6)  While the yellow highlight and grayed-out bits are going on,
    focus the second window.
7)  Wait for the findbar to hide itself.

EXPECTED RESULTS: When findbar hides, second window remains focused.

ACTUAL RESULTS: When findbar hides, first window is suddenly focused.

This has nearly led to me typing a password into the wrong website...

Mike, could this be fallout from bug 384458 too?  If not, I can try to find a regression range...
Flags: needinfo?(mdeboer)
Possibly, I'll check it out. Leaving n-i.
[Tracking Requested - why for this release]:

Regression range on nightlies: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3ccccf8e5036179a3178437cabc154b5e04b333d&tochange=b6f7d0eb61b1878d3d906bd231edf225463ece3f

This corresponds to the landing of bug 384458 but setting "findbar.modalHighlight" to false does NOT fix the problem.

Oh, and I thought toolkit/content/tests/chrome/bug360437_window.xul was supposed to catch this....
Blocks: 384458
Keywords: regression
Note, bz also disabled 'findbar.highlightAll' in the testing of comment #2.
Bisect says:

The first bad revision is:
changeset:   301431:73176395400e
user:        Mike de Boer <mdeboer@mozilla.com>
date:        Thu Jun 09 13:30:49 2016 +0200
summary:     Bug 384458 - part 4: implement modal highlighting using the AnonymousContent API and SVG masks. r=jaws
Aha!  I figured out what happened here.

The old code in findbar.xml's close() method did this:

  this.browser.finder.focusContent();

This landed in RemoteFinder.prototype.focusContent, which walked the listeners calling shouldFocusContent on them and one of the listeners returned false, because the window is not the focused one.  In fact, this is presumably the shouldFocusContent listener in findbar.xml.

The new code does this:

  this.browser.finder.onFindbarClose();

This lands in RemoteFinder.prototype.onFindbarClose which sends the "Finder:FindbarClose" message to the other process.  When we get that, we call this._finder.onFindbarClose(), which lands in Finder.prototype.onFindbarClose, which calls this.focusContent().  So we land in Finder.prototype.focusContent.  This does the listener walk again, but it's in the wrong process; the listeners aren't here.  So it goes ahead and does focusing stuff, when it shouldn't.

Jared, do you have time to look into the best way to fix this, or should we wait for Mike to get back?
Flags: needinfo?(jaws)
I'm looking in to this.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
https://reviewboard.mozilla.org/r/65342/#review62414

::: toolkit/content/widgets/findbar.xml:765
(Diff revision 1)
>  
>            if (aNoAnim)
>              this.setAttribute("noanim", true);
>            this.hidden = true;
>  
> -          this.browser.finder.onFindbarClose();
> +          this.browser.finder.focusContent();

Might be worth adding a comment so someone won't come along and refactor this again....
Comment on attachment 8772597 [details]
Bug 1285464 - Keep focus handling in the chrome process so focus will move back to the content out of the typeahead findbar but not change which chrome window has focus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65342/diff/1-2/
Comment on attachment 8772597 [details]
Bug 1285464 - Keep focus handling in the chrome process so focus will move back to the content out of the typeahead findbar but not change which chrome window has focus.

https://reviewboard.mozilla.org/r/65342/#review62498

I'm not at home in this code, but:

- AFAICT with this change there are zero callers for this method, so maybe it should just disappear. There's a parent-to-child forwarding infra that uses identical method names, but nothing will ever call this in the parent because you're removing the one callsite.
- that then makes me think that we should just get rid of the forwarding infra and just have RemoteFinder run the same code Finder is running, and ensure in the single implementation of `onFindBarClose` that it always runs in the parent/default process.
- `focusContent` has the same problem: it should AFAICT never run in the child, and so we should make sure it doesn't.
Attachment #8772597 - Flags: review?(gijskruitbosch+bugs)
Tracking 50+ for this regression focus issue.
Flags: needinfo?(mdeboer)
Comment on attachment 8772597 [details]
Bug 1285464 - Keep focus handling in the chrome process so focus will move back to the content out of the typeahead findbar but not change which chrome window has focus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65342/diff/2-3/
Attachment #8772597 - Attachment description: Bug 1285464 - Handle re-focusing content and disabling the highlighter from the findbar. → Bug 1285464 - Keep focus handling in the content process so focus will move back to the content out of the typeahead findbar but not change which chrome window has focus.
Attachment #8772597 - Flags: review?(mdeboer)
Comment on attachment 8772597 [details]
Bug 1285464 - Keep focus handling in the chrome process so focus will move back to the content out of the typeahead findbar but not change which chrome window has focus.

https://reviewboard.mozilla.org/r/65342/#review64122
Attachment #8772597 - Flags: review?(mdeboer) → review+
https://reviewboard.mozilla.org/r/65342/#review64122

A comment might indeed be good. What do you think of `// 'focusContent()' iterates over all listeners in the chrome process, so we need to call it from here.`
Comment on attachment 8772597 [details]
Bug 1285464 - Keep focus handling in the chrome process so focus will move back to the content out of the typeahead findbar but not change which chrome window has focus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65342/diff/3-4/
Attachment #8772597 - Attachment description: Bug 1285464 - Keep focus handling in the content process so focus will move back to the content out of the typeahead findbar but not change which chrome window has focus. → Bug 1285464 - Keep focus handling in the chrome process so focus will move back to the content out of the typeahead findbar but not change which chrome window has focus.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d72abc1d007
Keep focus handling in the chrome process so focus will move back to the content out of the typeahead findbar but not change which chrome window has focus. r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/7d72abc1d007
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.