450554 - focus restoring for panels uses a wonky timeout during popuphiding, should wait for popuphidden instead

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

s/_ancestorOfFocus/_currentFocus/

Comment 2

[Neil Deakin]

There was a reason why this was done that way. Please ask Aaron about what it was. 

Comment 3

[Dão Gottwald [:dao]]

It's bug 407359 comment 37, and pretty vague:

> > Was there a reason you didn't change this to popuphidden (last point of comment 26)?
> Yes -- I tried that and it didn't work. This was a couple of weeks ago already
> but my recollection is that the event was cancelled in some cases and the code
> wouldn't run. I'm not 100% sure that's what it was, because it was a couple of
> weeks ago.

So is it reasonable that the event handler wouldn't run in some cases?

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

The return should be a break now, in order to correctly reset this._currentFocus and this._prevFocus at the end of the handler.

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

I missed the other |return| in the |try| block.
This clears _currentFocus and _prevFocus in all cases.

Comment 6

[Neil Deakin]

(In reply to comment #3)
> It's bug 407359 comment 37, and pretty vague:

But did you ask him? i think Aaron should at least take a look at this patch too.

Otherwise, it looks OK to me, although you could just reuse 'currentFocus'  instead of using 'ancestorOfFocus'.

Comment 7

[Dão Gottwald [:dao]]

I was hoping that he would read this in his bug mail and comment. I'll try to reach him on IRC.

Comment 8

[Dão Gottwald [:dao]]

Attachment: without ancestorOfFocus

Comment 9

[Aaron Leventhal]

My comment in bug 407359 was that the popuphidden event was cancelled in some cases and the code wouldn't run, but I no longer recall the details.

That bug was fixed to deal with sending focus back to where it was after clicking on a toolbar button to open the bookmarks drop down or the larry/verification dialog, and then hitting Escape to close.

So if you test with those and it seems to work in all cases, then perhaps this is fine.

If we're going to do this, why get the currentFocus in popuphiding? Why not move that to popuphidden as well?

Comment 10

[Aaron Leventhal]

Also, the method name restoreFocusIfInPanel was self-documenting before, and has been removed. I suggest putting in a comment to that affect so people seeing this know what it is doing.

// Restore focus to where it was before the panel opened, unless it's already been moved outside of the panel

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #9)
> My comment in bug 407359 was that the popuphidden event was cancelled in some
> cases and the code wouldn't run, but I no longer recall the details.

The only reasonable cause for this would be a capturing event listener doing event.stopPropagation(), I think, but that would be expected behavior, and it could happen for popuphiding as well.

> That bug was fixed to deal with sending focus back to where it was after
> clicking on a toolbar button to open the bookmarks drop down or the
> larry/verification dialog, and then hitting Escape to close.

The larry panel actually uses norestorefocus="true", so it wouldn't be affected, although I think that needs to change (bug 416937). I'll test the "Bookmark this page" panel.

> If we're going to do this, why get the currentFocus in popuphiding? Why not
> move that to popuphidden as well?

My thinking is that when the panel is hidden, document.commandDispatcher.focusedElement can't possibly return an element within the panel. I may be wrong, though.

Comment 12

[Dão Gottwald [:dao]]

Attachment: with the suggested comment

The larry panel works as expected, with and without the patch from bug 416937. "Bookmark this page" is fine, too.

Comment 13

[[e-mail]]

(In reply to comment #11)
> (In reply to comment #9)
> > If we're going to do this, why get the currentFocus in popuphiding? Why not
> > move that to popuphidden as well?
> My thinking is that when the panel is hidden,
> document.commandDispatcher.focusedElement can't possibly return an element
> within the panel. I may be wrong, though.
In mail FE code we used to check document.commandDispatcher.focusedElement after hiding the search toolbar in case the search widget had focus, otherwise focus would get lost. I don't know whether that's still the case, but the point is hiding a toolbar doesn't move the focus, so hiding a panel shouldn't either.

Comment 14

[[e-mail]]

Comment on attachment 341623 [details] [diff] [review]
with the suggested comment

>+        try {
>+          this._currentFocus = document.commandDispatcher.focusedElement;
>+        } catch (e) {}
Also, why the try/catch? (If you want this to work embedded in xml content consider using document.activeElement instead.)

Comment 15

[Dão Gottwald [:dao]]

Attachment: fall back to document.activeElement

(In reply to comment #13)
> In mail FE code we used to check document.commandDispatcher.focusedElement
> after hiding the search toolbar in case the search widget had focus, otherwise
> focus would get lost. I don't know whether that's still the case, but the point
> is hiding a toolbar doesn't move the focus, so hiding a panel shouldn't either.

Was the toolbar collapsed or really hidden? In any case, my testing suggests that this doesn't work for panels. But even if it did, I would consider this a bug and prefer to not rely on it.

> Also, why the try/catch? (If you want this to work embedded in xml content
> consider using document.activeElement instead.)

Yes, I think we try/catch for embedded xul. document.commandDispatcher.focusedElement works across documents, so we should still try to use it.

Comment 16

[[e-mail]]

Comment on attachment 342466 [details] [diff] [review]
fall back to document.activeElement

>+          this._prevFocus = document.activeElement;
>+        }
>+        if (!this._prevFocus) {
Is there a problem here in that document.activeElement never returns null? In fact if the current window doesn't have focus then it always returns the body or document element.

>       <handler event="popuphiding"><![CDATA[
The toolbar really was being hidden, not collapsed, but given that this old code had to use popuphiding anyway then it's not a problem that you weren't able to improve on that aspect of the code.

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> (From update of attachment 342466 [details] [diff] [review])
> >+          this._prevFocus = document.activeElement;
> >+        }
> >+        if (!this._prevFocus) {
> Is there a problem here in that document.activeElement never returns null? In
> fact if the current window doesn't have focus then it always returns the body
> or document element.

This seems fine to me. When this._prevFocus is null it's set to document.commandDispatcher.focusedWindow, which would fail just like document.commandDispatcher.focusedElement anyway.

Comment 18

[Dão Gottwald [:dao]]

Attachment: still fall back to document.activeElement

Given my previous comment, I think it makes more sense to write it this way. This shouldn't make a functional difference.

Comment 19

[[e-mail]]

Comment on attachment 342562 [details] [diff] [review]
still fall back to document.activeElement

>+          // Restore focus to where it was before the panel opened, unless it's already
>+          // been moved outside of the panel

>-              // Focus was set on an element inside this panel,
>-              // so we need to move it back to where it was previously
I think the old comment more accurately descibes the purpose of the code.

Comment 20

[Dão Gottwald [:dao]]

I restored the old comment and made the new one briefer:

http://hg.mozilla.org/mozilla-central/rev/f7063c7906e5