396652 - gecko scrolls on activate event (mouse click) with gtk embeds

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Iain Nicol]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en; rv:1.8.1.6) Gecko/20061201 Epiphany/2.18 Firefox/2.0.0.6 (Ubuntu-feisty)
Build Identifier: 

Epiphany has a bug for this: <http://bugzilla.gnome.org/show_bug.cgi?id=335226>

This bug effects all native GTK+ software that embeds Gecko e.g. Epiphany, Galeon, Kazehakase, but not software that doesn't use native GTK+ widgets e.g. Firefox on GNU/Linux.

Gecko is deactivated when the native (chrome?) widgets are activated. When Gecko is reactivated, it erroneously scrolls.

Reproducible: Always

Steps to Reproduce:
1. Open up Epiphany
2. Navigate to a long webpage, such as <http://www.mozillazine.org/>
3. Click on a widget on the webpage (e.g. search text box) to activate it
4. Deactivate Gecko by clicking on the Address Bar
5. Scroll the webpage using the Find feature, or a scroll mouse
6. Activate Gecko by trying to click on a link
Actual Results:  
The page--annoyingly--scrolls back to bring the previously focused widget into view.

Expected Results:  
No scrolling ;)

My proposed patch follows.

Comment 1

[Iain Nicol]

Attachment: proposed fix

What happens is nsEventStateManager::PreHandleEvent() is called iteratively with the event NS_ACTIVATE. The procedure to handle this currently uses SetSuppressFocusScroll, but does not return SuppressFocusScroll to its initial value at the end.

Caveat:
This is my first Mozilla patch, and I haven't signed any contributor agreement. I don't know if I have to; I'm absolutely happy licensing this (trivial) code, whether that's implicit or explicit...

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 281422 [details] [diff] [review]
proposed fix


>-        PRBool isSuppressed;
>-        focusController->GetSuppressFocus(&isSuppressed);
>-        while (isSuppressed) {
>+        PRBool isFocusSuppressed;
>+        focusController->GetSuppressFocus(&isFocusSuppressed);
>+        while (isFocusSuppressed) {
>           // Unsuppress and let the focus controller listen again.
>           focusController->SetSuppressFocus(PR_FALSE,
>                                             "Activation Suppression");
> 
>-          focusController->GetSuppressFocus(&isSuppressed);
>+          focusController->GetSuppressFocus(&isFocusSuppressed);

Could you not change the variable name here. Keeps the patch smaller and makes using
lxr/bonsai easier. IMO, in this context isSuppressed is just good enough name for the variable.

Have you made sure that focus and blur events still work properly with the patch?
(probably they do)

What causes the page to scroll here? I mean which method?

Comment 3

[Iain Nicol]

Attachment: less obtrusive patch

> Could you not change the variable name here.
OK.

> Have you made sure that focus and blur events still work properly with the
> patch?
I'm not certain how to test this exhaustively. Besides saying "I can't think why they wouldn't work", I created an HTML page with an input boxes, similar to <http://developer.mozilla.org/en/docs/DOM:element.onblur>. I used Javascript to update the input boxes' contents when they handled onfocus and onblur. Clicking between the boxes, the rest of the page, and Epiphany's address bar behaves the same with an without the patch. e.g. click the address bar, deactivating Gecko, and the boxes would all say "onblur"; click on one of the boxes, and the box clicked would say "onfocus".

> What causes the page to scroll here? I mean which method?
The scrolling happens in PresShell::ScrollContentIntoView(). See <http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#3867>. There's a comment there describing behaviour similar to that in this bug is unwanted. This procedure checks the value of SuppressFocusScroll before scrolling; the problem is that SuppressFocusScroll can have the wrong value.

Also, FWIW, there's code in the Find bar that's almost identical to what my patch does; see <http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/findbar.xml#737>. It sets SuppressFocusScroll to true, but restores the previous value at the end, saying "We MUST reset suppressFocusScroll."

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #3)
> The scrolling happens in PresShell::ScrollContentIntoView(). See
> <http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#3867>.
What I meant is that, what is calling that method?

>Also, FWIW, there's code in the Find bar that's almost identical to what my
>patch does; see
><http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/
>findbar.xml#737>.
Ah. Could something similar be used if someone wants to fix the bug
for gecko 1.8 based Epiphany?

Comment 5

[Iain Nicol]

Attachment: stack trace at PresShell::ScrollContentIntoView()

(In reply to comment #4)
> > The scrolling happens in PresShell::ScrollContentIntoView(). See
> > <http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#3867>.
> What I meant is that, what is calling that method?
I've uploaded a (long) stack trace because it might help show what's going on.

In nsEventStateManager::PreHandleEvent(NS_ACTIVATE) there's a call "focusContent->SetFocus(content)" on line 1125; <http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#1125>. That's actually the function nsHTMLInputElement::SetFocus. This then calls nsGenericHTMLFormElement::SetFocusAndScrollIntoView, which then calls PresShell::ScrollContentIntoView.

You may want to know why nsEventStateManager::PreHandleEvent is called recursively with the NS_ACTIVATE event. Here's the complete picture (sorry if I'm too verbose): So what happens is I click the link. This propagates an event to nsEventStateManager::PreHandleEvent, which is #39 in the stack trace. SetSuppressFocusScroll(PR_TRUE) is called. Then there's a call to focusedWindow->Focus() on line 1111 in this function; <http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#1111>. This generates an event that propagates until it (recursively) reaches nsEventStateManager::PreHandleEvent, at #3 in the stack trace. Execution then continues to line 1125, and then PresShell::ScrollIntoView is called, as above. PresShell::ScrollIntoView doesn't actually do any scrolling, because SuppressFocusScroll is set to true. This second call to nsEventStateManager::PreHandleEvent finishes, setting SuppressFocusScroll to false. The stack then unwinds to the initial call to nsEventStateManager::PreHandleEvent, at #36 in the stack trace. Line 1125 is reached, and so PresShell::ScrollContentIntoView is called, as above. This time the scrolling happens erroneously, because SuppressFocusScroll was previously set to false. Then at the end of nsEventStateManager::PreHandleEvent, SuppressFocusScroll is set to false again.

> Could something similar be used if someone wants to fix the bug
> for gecko 1.8 based Epiphany?
Yes. Attached to the Epiphany bug at <http://bugzilla.gnome.org/show_bug.cgi?id=335226> is a patch that I made against either Firefox 2.0.0.5 or 2.0.0.6. Surprisingly, the patch there is identical to the first patch I uploaded here, apart from the line numbers.

Testing the patch against Gecko 1.8 has the advantage that it's currently easy to build Epiphany against Gecko 1.8, but hard to build it against Gecko 1.9.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Good explanation. I think I want a bit different kind of fix, just to make sure
things are handled properly elsewhere too. I'll post a patch soon, and 
it would be great if you could then test it with Epiphany.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

This makes it easier to suppress and reset the previous state.
Could you test this with Epiphany?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better names for variables

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm not a formal reviewer... But I have a question:

Cannot fix with changing PRPackedBool to PRUint32 for nsFocusController::mSuppressFocusScroll?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

That could be one possibility, but are there any advantages in that comparing to 
the patch? Having a separate (stack) object forces symmetrical 
SetSuppressFocusScroll calls ('set new value' --- 'set back the old value' ).

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #9)
> I'm not a formal reviewer... 

I don't care about that here. You fixed bug 105894, so you might have a comment.
And you did have :)

Comment 12

[Iain Nicol]

(In reply to comment #8)
> Created an attachment (id=281818) [details]
> better names for variables
This patch fixes the problem in Epiphany. :)

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> This patch fixes the problem in Epiphany. :)
> 
Good. Well, it is your patch packed in a bit different way ;)

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 281818 [details] [diff] [review]
better names for variables

hmm... O.K.

> @@ -3192,23 +3192,19 @@ nsEventStateManager::ChangeFocusWith(nsI

> +  if (aFocusedWith == eEventFocusedByMouse) {
> +    scrollSuppressor.Init(focusController);
>    }

Remove "{}"

> @@ -289,33 +289,23 @@ nsXULPopupListener::FireFocusOnTargetCon

>        if (ourWindow) {
> -        focusController = ourWindow->GetRootFocusController();
> -        if (focusController) {
> -          focusController->GetSuppressFocusScroll(&isAlreadySuppressed);
> -          if (!isAlreadySuppressed)
> -            focusController->SetSuppressFocusScroll(PR_TRUE);
> -        }
> +        scrollSuppressor.Init(ourWindow->GetRootFocusController());
>        }

same.

> +class nsFocusScrollSuppressor
> +{

'{' should be end of the class name. See the coding style of nsFocusSuppressor.

> +public:
> +  nsFocusScrollSuppressor(nsIFocusController* aController = nsnull)
> +  : mWasSuppressed(PR_FALSE)

this line should be indented.

> +  {
> +    Init(aController);
> +  }
> +
> +  ~nsFocusScrollSuppressor()
> +  {
> +    Init(nsnull);
> +  }

You can write them in one line.

> +
> +  void Init(nsIFocusController* aController)
> +  {
> +    if (mController) {
> +      mController->SetSuppressFocusScroll(mWasSuppressed);
> +    }

Remove "{}". And don't you need to call it only when mWasSuppressed is false?

> +
> +    mController = aController;
> +    if (mController) {

if aController is null, you should return here. And you can remove this if statement. (You can save the indent.)

> +      mController->GetSuppressFocusScroll(&mWasSuppressed);
> +      if (!mWasSuppressed) {
> +        mController->SetSuppressFocusScroll(PR_TRUE);
> +      }

Remove "{}".

> +    }
> +  }

Insert an empty line, here.

> +private:
> +  nsCOMPtr<nsIFocusController> mController;
> +  PRBool                       mWasSuppressed;

Use PRPackedBool.

> +};
> +

FYI: http://www.mozilla-japan.org/hacking/mozilla-style-guide.html

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Um, I should already know that style guide :p
Anyway, roc could possibly sr?