573689 - key events lost while page is loading

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Timothy Nikkel (:tnikkel)]

Click on a link, you will see the focus rectangle around the link disappear briefly before the next page loads. Try hitting ctrl-tab during this brief period. The keys have no effect.

Local bisection pinned this on bug 519913.

The PresShell for the "old" page handles the event. The problem is that the call to GetFocusedDOMWindowInOurWindow() in PresShell::HandleEvent just after "if (!sDontRetargetEvents) {" returns null because mDocument->GetWindow() returns null. The way of getting the nsPIDOMWindow to target the event to before bug 519913 instead returns the window for "next" page.

I don't really understand the problem that bug 519913 is fixing. Can we use the old way of getting the nsPIDOMWindow? Or failing that can we just RetargetEventToParent the event?

Comment 1

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

> Can we use the old way of getting the nsPIDOMWindow?

No, we cannot do.

> Or failing that can we just RetargetEventToParent the event?

I think that this is yes. And sounds better for me. I think that after the document is detached from window, we shouldn't handle any events in the document because event handlers may not assume such cases.

By the way, I think that PresShell::HandleEvent() shouldn't be called after the window was detached. Is the current behavior intentional?

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to comment #1)
> > Can we use the old way of getting the nsPIDOMWindow?
> 
> No, we cannot do.

What problems does it cause?

> > Or failing that can we just RetargetEventToParent the event?
> 
> I think that this is yes. And sounds better for me. I think that after the
> document is detached from window, we shouldn't handle any events in the
> document because event handlers may not assume such cases.
> 
> By the way, I think that PresShell::HandleEvent() shouldn't be called after the
> window was detached. Is the current behavior intentional?

I'm not sure. Bug 536926 comment 7 is about a similar problem and explains in a little more detail about this weird transition state between two documents.

Comment 3

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

Attachment: Patch v1.0

Probably, this fixes this bug.

But I cannot test this... How do you test? I cannot catch the short period...

Comment 4

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

(In reply to comment #2)
> (In reply to comment #1)
> > > Can we use the old way of getting the nsPIDOMWindow?
> > 
> > No, we cannot do.
> 
> What problems does it cause?

Because we need to look for the focused window in same top level window. The failure is a process of looking for the top level window.

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 453293 [details] [diff] [review]
Patch v1.0

This fixes the problem for me. Thanks!

Comment 6

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

Thanks, I'll request review after the patch passes all tests on tryserver.

Comment 7

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

Comment on attachment 453293 [details] [diff] [review]
Patch v1.0

looks good on tryserver.

If the document is zombie, this patch tries to find the root window with ancestor presShell's DOM window.

Comment 8

[Neil Deakin]

(In reply to comment #0)
> Click on a link, you will see the focus rectangle around the link disappear
> briefly before the next page loads.

You shouldn't be getting a focus indicator at all. What page/steps is this?

Comment 9

[Timothy Nikkel (:tnikkel)]

Most pages, I have to work to find one that doesn't. But if you want specifics: mxr and bugzilla. This happens on trunk on both windows and linux for me.

Comment 10

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

Comment on attachment 453293 [details] [diff] [review]
Patch v1.0


>+already_AddRefed<nsIPresShell>
>+PresShell::GetParentPresShell()
>+{
>+  NS_ENSURE_TRUE(mPresContext, nsnull);
>+  nsCOMPtr<nsISupports> container = mPresContext->GetContainer();
>+  if (!container) {
>+    container = do_QueryReferent(mForwardingContainer);
>+  }
>+
>+  // Now, find the parent pres shell and send the event there
>+  nsCOMPtr<nsIDocShellTreeItem> treeItem = do_QueryInterface(container);
>+  // Might have gone away, or never been around to start with
>+  NS_ENSURE_TRUE(treeItem, nsnull);
>+
>+  nsCOMPtr<nsIDocShellTreeItem> parentTreeItem;
>+  treeItem->GetParent(getter_AddRefs(parentTreeItem));
>+  nsCOMPtr<nsIDocShell> parentDocShell = do_QueryInterface(parentTreeItem);
>+  NS_ENSURE_TRUE(parentDocShell && treeItem != parentTreeItem, nsnull);
>+
>+  nsIPresShell* parentPresShell;
Please initialize parentPresShell to nsnull.

Comment 11

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

Attachment: Patch v1.1

Comment 12

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

http://hg.mozilla.org/mozilla-central/rev/443b653e6b2e