921308 - When find toolbar auto-hides it scrolls the page to the last search result

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: 2013-09-26 nightly, but I just updated from a few weeks back, so not sure yet when this got introduced

STEPS TO REPRODUCE:
1) Load http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp
2) Wait for the page to finish loading.
3) Type '/' and then type the string "constructsele" in the find toolbar that
   pops up.
4) Hit PgDown three times.
5) Wait for the find toolbar to hide itself

EXPECTED RESULTS: When find toolbar hides, the page does not scroll

ACTUAL RESULTS: When find toolbar hides, the page scrolls back up to the text that was found in step 3.

This is driving me bananas, needless to say: one moment I'm reading code, the next moment I'm 300 lines from where I used to be.  ;)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Regression range on m-c nightlies: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b9029b1de410&tochange=53d5e43e23cc

Comment 2

[Boris Zbarsky [:bzbarsky]]

Bisect says:

The first bad revision is:
changeset:   147026:3734bebc9bfb
user:        Tom Schuster <[e-mail]>
date:        Fri Sep 13 10:41:23 2013 -0400
summary:     Bug 666816 - Support findbar in e10s. r=mikedeboer

Comment 3

[Boris Zbarsky [:bzbarsky]]

The old code had:

-                // block scrolling on focus since find already scrolls, further
-                // scrolling is due to user action, so don't override this
...
-                  fm.setFocus(element, fm.FLAG_NOSCROLL);

which the new code has totally lost.

Comment 4

[Tom S. (please needinfo tschuster)]

I think this code is not really e10s compatible, we will have to come up with a different solution. I will try to work on this sometime next week.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

No need to track this for 26 given that we're tracking bug 916536.

Comment 6

[Tom S. (please needinfo tschuster)]

Would you mind taking a shoot at this, while I review your patch?

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Tom Schuster [:evilpie] from comment #6)
> Would you mind taking a shoot at this, while I review your patch?

Aye, will do.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: don't scroll when an element is focused on findbar close

Ahum. So, since we can't use the focus-manager anymore and use the DOM .focus() method, I needed to 'invent' the handler that implements the desired behavior myself.

Introducing `focusWithoutScrolling`! It's amazing, sweeter than unicorn piss, will cure cancer and make you a barista at the same time.

(sorry about that ;) )

Tom, is this e10s-proof?

Comment 9

[Tom S. (please needinfo tschuster)]

I think you should try using the FocusManager like the old code: http://mxr.mozilla.org/mozilla-beta/source/toolkit/content/widgets/findbar.xml#1183.

This should be e10s safe (minus some focusing problems that are not your fault)

Comment 10

[Tom S. (please needinfo tschuster)]

Especially not the NOSCROLL flag :)

Comment 11

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: don't scroll when an element is focused on findbar close

Note that this also fixes bug 921338.

Try push to make sure this'll turn up green:
https://tbpl.mozilla.org/?tree=Try&rev=72a86ee19693

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.2: don't scroll when an element is focused on findbar close

fixed whitespace nit.

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.3: don't scroll when an element is focused on findbar close

trying to reach nirvana, ehm, no wait, I meant e10s compat.

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.4: don't scroll when an element is focused on findbar close

Introducing the |onFocusContent| event.

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.5: don't scroll when an element is focused on findbar close

Removed a blank (new)line. Sorry for spamming.

Comment 16

[Tom S. (please needinfo tschuster)]

Comment on attachment 817810 [details] [diff] [review]
Patch v1.5: don't scroll when an element is focused on findbar close

Review of attachment 817810 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea! We we can send synchronous message from the child to the parent, so this is e10s compatible.

::: toolkit/modules/Finder.jsm
@@ +113,5 @@
>  
>    focusContent: function() {
> +    // Allow Finder listeners to cancel focusing the content.
> +    for (let l of this._listeners) {
> +      if (l.onFocusContent() === false)

I don't think this a good name. Maybe something like "canFocusContent" or "should"? "on" suggest something like an event listener. I am also not  a big fan of === false, I would prefer if (!

::: toolkit/modules/RemoteFinder.jsm
@@ +110,5 @@
>    },
>  
> +  //XXXmikedeboer-20131016: implement |onFocusContent| here to mitigate issues
> +  //                        like bug 921338 and bug 921308.
> +  onFocusContent: function () {},

I can implement this if you want.

Comment 17

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/3e103997950b

Comment 18

[Tom S. (please needinfo tschuster)]

Actually did you check if this introduces test failures? I added some tests that use browser.finder directly and those listener implementation don't have the shouldFocusContent property.

Comment 19

[Tom S. (please needinfo tschuster)]

Oh sorry never mind, those tests don't use focusContent.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e103997950b