323805 - Fix for bug 175893 breaks tab switching if content is not HTML

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The fix for bug 175893 introduced the following line into updateCurrentBrowser():

  this.mCurrentBrowser.focusedElement.blur();

This throws if this.mCurrentBrowser.focusedElement is not an HTML element, which breaks tab-switching pretty badly.  In particular, this breaks tab switching if an SVG anchor has focus, which is just not cool.

The solution is probably to put a try/catch around this line, but perhaps we should look for a not-only-HTML way to do this too.

To test, load URL in the URL bar, right click the anchor, dismiss the context menu while keeping the anchor focuse, and try switching tabs.

I'll file a separate bug for seamonkey, but make it depend on this one.

Comment 1

[Martijn Wargers (dead)]

Attachment: patch

This seems to fix it.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

This is the same as bug 316191.

Comment 3

[Andrew Schultz]

Comment on attachment 208776 [details] [diff] [review]
patch

I tested a SeaMonkey version of this and found that if an element in one tab had focus, then if I switched to another tab I was unable to use keyboard navigation until I explicitly clicked to give the new tab (or some content in the new tab) focus.

Comment 4

[Martijn Wargers (dead)]

Ah sorry, I thought that DOMFocusIn and DOMFocusOut were working in Mozilla, but that's not the case yet, see bug 60212, comment 12:
"This patch implements DOMActivate for HTML input, button, and anchor elements,
and adds the infrastructure to support DOMFocusIn and DOMFocusOut (those events
are not dispatched currently, but that's next)."

That means that I don't see any other way to fix this then to put try..catch around the code.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: mirror branch patch from bug 323806 + focus() protection

Comment 6

[Andrew Schultz]

Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection

FYI, I didn't include focus() protection in bug 323806 because the code that calls setFocus() checks to see if the element is null and it should already be null because of:

>+                this.mCurrentBrowser.focusedElement = null;

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection

looks ok... it might be cleaner if we could just do 

if "blur" in this.mCurrentBrowser.focusedElement

but I'm not sure what the security implications of that might be.

Comment 8

[Jonathan Watt [:jwatt]]

Ah, thanks Andrew. That bit of the patch can be dropped on checkin then.

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection

Requesting approval for 1.8.0.2 and 1.8.1. This should be low risk. It's the same patch as was landed for SeaMonkey.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection

I'm not a module owner or peer for this code, so I can't grant branch-1.8.1.  Please ask someone who can?

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection

Sure. Requesting branch-1.8.1 from Neil instead.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection

a=dveditz, platform parity, suite version already part of 1.8.0.1

Comment 13

[:Gavin Sharp [email: [e-mail]]]

*** Bug 324822 has been marked as a duplicate of this bug. ***

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 209727 [details] [diff] [review]
mirror branch patch from bug 323806 + focus() protection

Can someone check this in on both branches please? I'll be away until the 22 Feb.

Comment 15

[Daniel Veditz [:dveditz]]

Reassigning to Boris for checkin on the logic that since he nominated this for 1.8.0.2 he must care about this bug.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed on branches.  Leaving for trunk fix.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Er... apparently whoever posted that patch didn't so much test it. This caused bug 327139.

Comment 18

[Jonathan Watt [:jwatt]]

Sorry, that was my rushed, pre-vacation testing. :-(

Comment 19

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060306 Firefox/1.5.0.2.

Looks like this patch also fixes bug 316191.  With the same build above, I was able to verify that tab focus works as expected with testcases there as well (See https://bugzilla.mozilla.org/show_bug.cgi?id=316191#c15).

We still need to patch the Trunk.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Is there a reason that this patch didn't land on the trunk? It should be OK to land as is, with the fix from bug 327139, right?

Comment 21

[[e-mail]]

(In reply to comment #20)
> Is there a reason that this patch didn't land on the trunk?
"perhaps we should look for a not-only-HTML way to do this too."

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 316191 has been marked as a duplicate of this bug. ***

Comment 23

[Mike Connor [:mconnor]]

need this fixed on trunk, carrying over blocking flag...

Comment 25

[Aaron Leventhal]

> this.mCurrentBrowser.focusedElement.blur();
Do we actually still need that line? It seems like a hack in order to avoid focus outline ghosting or something.

It's also causing bug 382007.

Comment 26

[[e-mail]]

(In reply to comment #25)
>It seems like a hack in order to avoid focus outline ghosting or something.
Yes, the problem being that content.focus() still doesn't properly blur the previous element so we have to do it manually.

Comment 27

[Aaron Leventhal]

Is there a bug open for that?

Comment 28

[Neil Deakin]

Since for some reason this is assigned to me, what exactly still needs to be done here?

Comment 29

[jonathan chetwynd]

Neil,

Bug 316191 changed status and is marked as a dupe of this bug.
whilst the original bug 316191 is 'fixed' the fix may not be ideal:

Should the focus state of tab be retained on blur?

open https://bugzilla.mozilla.org/attachment.cgi?id=202809
navigate with keyboard and notice red ring
open an extra tab, change URI
select original tab

-- Notice focus has been lost...

would expect focus to be retained?

in this somewhat trivial example this may seem insignificant, however one may have navigated a fairly complex map, then checked a detail and be returning....

cheers

Comment 30

[jonathan chetwynd]

#29 in the general HTML case focus state is retained for each tab

Comment 31

[jonathan chetwynd]

#29 to compare:

open a new window, return to original, focus is retained, 
so this - is - the expected behaviour.

Opera retains focus for each tab.

Clearly the current fix is broken in respect of this issue

Comment 32

[[e-mail]]

(In reply to comment #28)
>Since for some reason this is assigned to me, what exactly still needs to be
>done here?
As I recall, you have a number of options ;-)
1. Make it so focusing the new tab (via content.focus() or some such) properly removes the focus from the currently focused element in the current tab.
2. Make it possible to remove the focus from the currently focused element without requiring focus or blur methods which only HTML or XUL elements have.
3. Use a hack similar to the compose window (which I believe advances focus into the frame element rather than focusing the content window).

Comment 33

[jonathan chetwynd]

Re: #32

Neil,

1. the current behaviour seems to mimic this suggestion, and as per #29-31 this appears wrong.

2. we would like to retain focus see (1), not remove, so this seems wrong.

3. no comment as it's not explicit, to me at least...

notwithstanding this there seems to be some confusion regarding use of terms such as blur and focus:

Navigating the content, or the application?

Comment 34

[[e-mail]]

(In reply to comment #33)
>notwithstanding this there seems to be some confusion regarding use of terms
>such as blur and focus:
OK, so here's a quick summary. Only one element (or window, if no element in the window has focus) can have focus at a time. Each window remembers which element last had focus so that when you switch window it automatically restores focus to that element. However the tabbed browser can't emulate this, because only some elements have a scriptable method to restore focus.
What I actually touched on in comment #32 was that tabbed browser can't handle removing focus from the previous tab either, because in certain cases restoring focus doesn't remove the previous focus correctly.

Comment 35

[jonathan chetwynd]

#34

Neil,

"However the tabbed browser can't emulate this"

If the can't is absolute then this is a 'major' design fault. And needs to be marked and addressed as such.

tab emulates new window.
if tab cannot emulate new window then it isn't worth implementing.
because the general user will have to learn a new routine.

for instance I also tested by chance with http://www.peepo.co.uk where using CSS visually it appears that focus is retained, but it isn't. This will easily lead to a mess, unless the application behaves as expected. 

to repeat:
as per #30 html already maintains focus within tabs
as per #31 opera maintains focus in svg and html

thanks for your explanation of comment #32 are there additional "in certain cases" bugs that depend on this one?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Perhaps window utils should have a method for focusing any element or something?

Comment 37

[[e-mail]]

(In reply to comment #35)
>"However the tabbed browser can't emulate this"
> 
>If the can't is absolute then this is a 'major' design fault. And needs to be
>marked and addressed as such.
Well, that was bug 175893 but we've since discovered it isn't foolproof.

(In reply to comment #36)
>Perhaps window utils should have a method for focusing any element or something?
Sounds like a plan ;-)

Comment 38

[Neil Deakin]

Attachment: like so

Add a windowutils focus method, and call it as a backup if focus/blur methods are not available.

Comment 39

[[e-mail]]

Comment on attachment 271832 [details] [diff] [review]
like so

>+  void focus(in nsIDOMNode aNode);
Are all nodes focusable, or only elements?

>+NS_IMETHODIMP
>+nsDOMWindowUtils::Focus(nsIDOMNode* aNode)
>+{
>+  if (mWindow) {
>+    nsIDocShell *docShell = mWindow->GetDocShell();
>+    if (docShell) {
>+      nsCOMPtr<nsIPresShell> presShell;
>+      docShell->GetPresShell(getter_AddRefs(presShell));
>+      if (presShell) {
>+        nsPresContext *pc = presShell->GetPresContext();
>+        if (pc) {
>+          nsCOMPtr<nsIContent> content = do_QueryInterface(aNode);
>+          pc->EventStateManager()->ChangeFocusWith(content,
>+              nsIEventStateManager::eEventFocusedByApplication);
>+        }
>+      }
>+    }
>+  }
>+}
No return value. (On my compiler this is a compile error...)

>+                if ("blur" in elem) {
>+                  elem.blur();
>+                }
I guess you're doing this as a quick shortcut?

>+                else {
>+                  var content = window.content;
>+                  if (content instanceof Components.interfaces.nsIInterfaceRequestor)
>+                    content.getInterface(Components.interfaces.nsIDOMWindowUtils).focus(null);
This doesn't work e.g. in a frame; the window you need to use is the one containing the element i.e. element.ownerDocument.defaultView

Comment 40

[Neil Deakin]

Attachment: address issues

> I guess you're doing this as a quick shortcut?

Yes, but html elements seem to like it better.

Comment 41

[[e-mail]]

Comment on attachment 272020 [details] [diff] [review]
address issues

>+                var content = window.content;
Nit: you forgot to change this one too.
You might also want to change the variable name ;-)

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272020 [details] [diff] [review]
address issues

>Index: dom/public/idl/base/nsIDOMWindowUtils.idl
>+   * @param aElement the element to focus

Document that this should be an element that comes from the document in question?

>Index: dom/src/base/nsDOMWindowUtils.cpp

>+ pc->EventStateManager()->ChangeFocusWith(content,
>+              nsIEventStateManager::eEventFocusedByApplication);

Is there a problem if the node is not actually in the document?  If so, check here and bail?

>Index: toolkit/content/widgets/tabbrowser.xml

Does seamonkey need a similar change?  If so, file it, please?

>+                if ("blur" in elem) {
>+                  elem.blur();

I think you want to explicitly check for the right interfaces here; otherwise you could be calling some content-defined blur function, no?

Not like this wasn't a problem before...

Same thing for focus.

sr=bzbarsky with those nits.

Comment 43

[Neil Deakin]

Attachment: fix comments

> Does seamonkey need a similar change?  If so, file it, please?

That's bug 323806

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272072 [details] [diff] [review]
fix comments

Looks great!

Comment 45

[jonathan chetwynd]

I would reopen right away, but...

https://bugzilla.mozilla.org/attachment.cgi?id=202809
the elements on this attachments no longer accept focus.
older versions show changes in fill when navigating with keyboard.

I should probably provide css versions that don't rely on script :-(

http://www.peepo.co.uk appears unchanged to #35

Comment 46

[[e-mail]]

(In reply to comment #45)
>https://bugzilla.mozilla.org/attachment.cgi?id=202809
>the elements on this attachments no longer accept focus.
That looks like a recent regression - it works in a build I have from 29th May. Would you mind filing a new bug for that? Thanks.

Comment 47

[jonathan chetwynd]

#46

Neil

please can you confirm whether the issue in #35 is resolved?

Comment 48

[[e-mail]]

(In reply to comment #47)
>please can you confirm whether the issue in #35 is resolved?
The issue in #35 should be resolved in the latest Firefox nightlies.

Comment 49

[jonathan chetwynd]

#48

the issue in #35 is at best only part fixed.

open http://www.peepo.co.uk in one tab
use keyboard to navigate through say 4 links.
open another tab http://www.peepo.co.uk
return to the first tab, and it appears that focus is retained.

however if instead one navigates through the second tab before returning the situation is once again a mess.

I propose to reopen this bug unless anyone has convincing arguments and test cases which demonstrate that keyboard navigation across tabs is fixed.

cheers


also please note in the absence of other evidence that #46 "looks like a recent regression"
is this not likely to be related to patches on this bug?
and if so may it be a bad idea to open another bug?

Comment 50

[[e-mail]]

(In reply to comment #49)
>however if instead one navigates through the second tab before returning the
>situation is once again a mess.
Although I'm not using an official build I don't see any issues on this page.

>also please note in the absence of other evidence that #46 "looks like a recent
>regression"
>is this not likely to be related to patches on this bug?
Absolutely no relation at all.

Comment 51

[jonathan chetwynd]

#50

Neil OS X using most recent build from here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

more explicitely:

open http://www.peepo.co.uk in one tab
use keyboard to navigate through say 4 links.
open another tab http://www.peepo.co.uk
use keyboard to navigate through say 4 links.
return to the first tab, and it appears that focus is retained.

BUT now try to move focus...
in fact focus is with the application, not in the content window
and when focus moves to content window it starts with first link.

This contrasts with the first case in #49 where focus is in the content window on the link that had focus last.

Comment 52

[jonathan chetwynd]

#46

Neil,

Can this bug be fixed?
isn't it dependent on the bug that you are asking me to file.

if this bug is fixed does that mean keyboard navigation should behave in a predicatable and expected manner?

Perhaps I'm involuntarily expanding the scope of this bug, apologies.

Comment 53

[Neil Deakin]

When one navigates by using the keyboard to switch tabs, the focus is retained, but when clicking the tab labels, focus is switched to the window in the tab. I don't know why this is the behaviour used, but I didn't change it.

This bug is about tab switching while a non-html-or-xul element (such as svg) is focused, and that is fixed.

Comment 54

[jonathan chetwynd]

Neil Deakin,

Neil wrote in comment #46:

"Each window remembers which
element last had focus so that when you switch window it automatically restores
focus to that element."

this is not the behaviour at present.
as you appear not to be considering this issue in this bug I am reverting comments to my original bug which whilst marked as a dupe remains open.

cheers

Comment 55

[jonathan chetwynd]

#49

these outstanding issues have been moved to:
Bug 388146

Comment 56

[[e-mail]]

(In reply to comment #53)
>When one navigates by using the keyboard to switch tabs, the focus is retained,
>but when clicking the tab labels, focus is switched to the window in the tab. I
>don't know why this is the behaviour used, but I didn't change it.
Obviously SeaMonkey behaviour (with the latest patch in bug 323806) is different, because I always see the focus being retained, except by design when switching tabs by focusing the tab and using the arrow keys.

>This bug is about tab switching while a non-html-or-xul element (such as svg)
>is focused, and that is fixed.
Agreed.