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

RESOLVED FIXED in mozilla1.9.1b2

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
mozilla1.9.1b2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 333713 [details] [diff] [review]
patch
Attachment #333713 - Flags: review?(enndeakin)
(Assignee)

Updated

10 years ago
Blocks: 450477
(Assignee)

Comment 1

10 years ago
Created attachment 333714 [details] [diff] [review]
patch

s/_ancestorOfFocus/_currentFocus/
Attachment #333713 - Attachment is obsolete: true
Attachment #333714 - Flags: review?(enndeakin)
Attachment #333713 - Flags: review?(enndeakin)

Comment 2

10 years ago
There was a reason why this was done that way. Please ask Aaron about what it was. 
(Assignee)

Comment 3

10 years ago
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?
(Assignee)

Comment 4

10 years ago
Created attachment 333745 [details] [diff] [review]
patch

The return should be a break now, in order to correctly reset this._currentFocus and this._prevFocus at the end of the handler.
Attachment #333714 - Attachment is obsolete: true
Attachment #333745 - Flags: review?(enndeakin)
Attachment #333714 - Flags: review?(enndeakin)
(Assignee)

Updated

10 years ago
No longer blocks: 450477
(Assignee)

Comment 5

10 years ago
Created attachment 333802 [details] [diff] [review]
patch

I missed the other |return| in the |try| block.
This clears _currentFocus and _prevFocus in all cases.
Attachment #333745 - Attachment is obsolete: true
Attachment #333802 - Flags: review?(enndeakin)
Attachment #333745 - Flags: review?(enndeakin)
(Assignee)

Updated

10 years ago
No longer blocks: 416937

Comment 6

10 years ago
(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'.
(Assignee)

Comment 7

10 years ago
I was hoping that he would read this in his bug mail and comment. I'll try to reach him on IRC.
(Assignee)

Comment 8

10 years ago
Created attachment 340358 [details] [diff] [review]
without ancestorOfFocus
(Assignee)

Updated

10 years ago
Attachment #333802 - Attachment is obsolete: true
Attachment #333802 - Flags: review?(enndeakin)

Comment 9

10 years ago
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

10 years ago
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
(Assignee)

Comment 11

10 years ago
(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.
(Assignee)

Comment 12

10 years ago
Created attachment 341623 [details] [diff] [review]
with the suggested comment

The larry panel works as expected, with and without the patch from bug 416937. "Bookmark this page" is fine, too.
Attachment #340358 - Attachment is obsolete: true
Attachment #341623 - Flags: review?(enndeakin)
(Assignee)

Updated

10 years ago
Attachment #341623 - Flags: review?(enndeakin) → review?(neil)

Comment 13

10 years ago
(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

10 years ago
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.)
(Assignee)

Comment 15

10 years ago
Created attachment 342466 [details] [diff] [review]
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.
Attachment #341623 - Attachment is obsolete: true
Attachment #342466 - Flags: review?(neil)
Attachment #341623 - Flags: review?(neil)

Comment 16

10 years ago
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.
(Assignee)

Comment 17

10 years ago
(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.
(Assignee)

Comment 18

10 years ago
Created attachment 342562 [details] [diff] [review]
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.
Attachment #342466 - Attachment is obsolete: true
Attachment #342562 - Flags: review?(neil)
Attachment #342466 - Flags: review?(neil)

Comment 19

10 years ago
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.
Attachment #342562 - Flags: review?(neil) → review+
(Assignee)

Comment 20

10 years ago
I restored the old comment and made the new one briefer:

http://hg.mozilla.org/mozilla-central/rev/f7063c7906e5
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
You need to log in before you can comment on or make changes to this bug.