Closed Bug 573689 Opened 10 years ago Closed 10 years ago

key events lost while page is loading

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b2

People

(Reporter: tnikkel, Assigned: masayuki)

References

Details

Attachments

(1 file, 1 obsolete file)

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?
> 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?
(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.
Attached patch Patch v1.0 (obsolete) — Splinter Review
Probably, this fixes this bug.

But I cannot test this... How do you test? I cannot catch the short period...
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
(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 on attachment 453293 [details] [diff] [review]
Patch v1.0

This fixes the problem for me. Thanks!
Thanks, I'll request review after the patch passes all tests on tryserver.
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.
Attachment #453293 - Flags: review?(Olli.Pettay)
(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?
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 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.
Attachment #453293 - Flags: review?(Olli.Pettay) → review+
Attached patch Patch v1.1Splinter Review
Attachment #453293 - Attachment is obsolete: true
Attachment #454787 - Flags: superreview?(roc)
Attachment #454787 - Flags: review+
Attachment #454787 - Flags: superreview?(roc) → superreview+
http://hg.mozilla.org/mozilla-central/rev/443b653e6b2e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3b2
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.