Last Comment Bug 323806 - Fix for bug 175893 breaks tab switching if content is not HTML
: Fix for bug 175893 breaks tab switching if content is not HTML
Status: RESOLVED FIXED
: fixed-seamonkey1.0, fixed1.8.0.1, fixed1.8.1
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
data:application/xml,<html:html xmlns...
: 485898 (view as bug list)
Depends on: 323805
Blocks: 175893 282178
  Show dependency treegraph
 
Reported: 2006-01-17 13:07 PST by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2009-09-22 06:18 PDT (History)
8 users (show)
cbiesinger: blocking‑seamonkey1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
just try/catch (1.27 KB, patch)
2006-01-22 19:38 PST, Andrew Schultz
no flags Details | Diff | Review
use instanceof check (1.42 KB, patch)
2006-01-23 23:18 PST, Andrew Schultz
no flags Details | Diff | Review
instanceof check without the null check (1.43 KB, patch)
2006-01-24 11:20 PST, Andrew Schultz
neil: review+
neil: superreview+
iann_bugzilla: approval‑seamonkey1.0+
csthomas: approval‑seamonkey1.1a+
Details | Diff | Review
Port from bug 323805 (1.59 KB, patch)
2007-07-14 04:06 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2006-01-17 13:07:06 PST
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.
Comment 1 Christian :Biesinger (don't email me, ping me on IRC) 2006-01-18 15:21:06 PST
(bug 323805 has a patch for firefox)
Comment 2 Andrew Schultz 2006-01-22 19:38:33 PST
Created attachment 209323 [details] [diff] [review]
just try/catch

This should restore pre-175893 behavior.  It fixes the testcase for this bug and shouldn't break anything.

At this point, I'm more concerned with unbreaking the branch.  If there's a more correct way to handle this, we can work on that for trunk.
Comment 3 Andrew Schultz 2006-01-23 23:18:45 PST
Created attachment 209427 [details] [diff] [review]
use instanceof check

Neil suggested this as an alternative for branch
Comment 4 neil@parkwaycc.co.uk 2006-01-24 08:07:47 PST
Comment on attachment 209427 [details] [diff] [review]
use instanceof check

Thinking about it, for both patches, you don't need the null test as the inner test already works "correctly" for null.
Comment 5 Andrew Schultz 2006-01-24 08:24:49 PST
hmm... what null test would that be?
Comment 6 neil@parkwaycc.co.uk 2006-01-24 09:25:47 PST
Comment on attachment 209427 [details] [diff] [review]
use instanceof check

>               if (this.mCurrentBrowser.focusedElement) {
Just replace this test with the improved test.
Comment 7 Andrew Schultz 2006-01-24 11:20:40 PST
Created attachment 209463 [details] [diff] [review]
instanceof check without the null check
Comment 8 neil@parkwaycc.co.uk 2006-01-24 15:40:25 PST
Comment on attachment 209463 [details] [diff] [review]
instanceof check without the null check

It might be an idea to put the comment back in though.
Comment 9 Andrew Schultz 2006-01-24 15:45:07 PST
Comment on attachment 209463 [details] [diff] [review]
instanceof check without the null check

I think the plan here was to put this on the branches and see what happens in bug 323806 for trunk.
Comment 10 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-01-24 15:48:35 PST
Comment on attachment 209463 [details] [diff] [review]
instanceof check without the null check

first-a=me, need 1 more
Comment 11 Ian Neal 2006-01-24 15:57:04 PST
Comment on attachment 209463 [details] [diff] [review]
instanceof check without the null check

a=me for SM1.0 (2nd one)
Comment 12 Ian Neal 2006-01-24 15:57:55 PST
Comment on attachment 209463 [details] [diff] [review]
instanceof check without the null check

(assuming comment added and indentation fixed)
Comment 13 Andrew Schultz 2006-01-24 18:08:22 PST
patch is in on the branches (with fixups), leaving open for a trunk fix
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2006-02-09 04:37:40 PST
1.1 is from the 1.8 branch, where this is (supposedly?) fixed, no need to block

hm. should we have a 1.5 blocking flag too?
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-02-14 09:05:50 PST
Doesn't this cause bug 327139 in seamonkey too?
Comment 16 Andrew Schultz 2006-02-14 19:15:27 PST
No, I'm thoroughly unable to reproduce that bug.  I grabbed a branch firefox build to make sure I was doing the appropriate steps.

But it does suggest the approach might be a bit fragile.  Perhaps try/catch or bryner's suggestion in bug 323805 Comment 7 would be better.
Comment 17 neil@parkwaycc.co.uk 2006-02-18 13:11:41 PST
(In reply to comment #15)
>Doesn't this cause bug 327139 in seamonkey too?
No, because we only protect the blur, and rely on that to protect the focus.
Comment 18 neil@parkwaycc.co.uk 2007-07-14 04:06:53 PDT
Created attachment 272303 [details] [diff] [review]
Port from bug 323805
Comment 19 neil@parkwaycc.co.uk 2007-07-14 04:07:54 PDT
Comment on attachment 272303 [details] [diff] [review]
Port from bug 323805

bah, :CTho stopped working
Comment 20 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-07-14 08:41:38 PDT
Comment on attachment 272303 [details] [diff] [review]
Port from bug 323805

I can't figure out how to use the testcase in the URL field, and I don't understand this patch, so I can't review it.  Sorry.

FYI, it's "(CTho)", not ":CTho"
Comment 21 neil@parkwaycc.co.uk 2009-08-14 05:33:53 PDT
Comment on attachment 272303 [details] [diff] [review]
Port from bug 323805

Ironically this code is already obsolete on trunk because of focus manager, but it's advised for 1.9.1
Comment 22 Ian Neal 2009-08-23 13:20:56 PDT
Comment on attachment 272303 [details] [diff] [review]
Port from bug 323805

>Index: suite/browser/tabbrowser.xml
>===================================================================
>RCS file: /cvsroot/mozilla/suite/browser/tabbrowser.xml,v
>retrieving revision 1.182
>diff -u -r1.182 tabbrowser.xml
>--- suite/browser/tabbrowser.xml	6 Jul 2007 13:55:11 -0000	1.182
>+++ suite/browser/tabbrowser.xml	14 Jul 2007 10:53:04 -0000
>@@ -734,7 +734,10 @@
>                 // Clear focus outline before we draw on top of it.
>                 // Only blur the focused element if it isn't a tab, 
>                 // to avoid breaking keyboard tab navigation
Is this comment still correct? I presume the differences between this patch and the one for toolkit is how tabbrowser.xml works on suite?
Comment 23 neil@parkwaycc.co.uk 2009-08-24 02:03:49 PDT
(In reply to comment #22)
> (From update of attachment 272303 [details] [diff] [review])
> >                 // Clear focus outline before we draw on top of it.
> >                 // Only blur the focused element if it isn't a tab, 
> >                 // to avoid breaking keyboard tab navigation
> Is this comment still correct?
Sorry, the tab test is unfortunately just outside the limited diff context.

> I presume the differences between this patch and
> the one for toolkit is how tabbrowser.xml works on suite?
No, I just thought their code was a bit redundant, seeing as nsIDOMWindowUtils works even on elements that have their own blur() and focus() methods.
Comment 24 Ian Neal 2009-08-24 16:52:19 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 272303 [details] [diff] [review])
> > >                 // Clear focus outline before we draw on top of it.
> > >                 // Only blur the focused element if it isn't a tab, 
> > >                 // to avoid breaking keyboard tab navigation
> > Is this comment still correct?
> Sorry, the tab test is unfortunately just outside the limited diff context.
> 
> > I presume the differences between this patch and
> > the one for toolkit is how tabbrowser.xml works on suite?
> No, I just thought their code was a bit redundant, seeing as nsIDOMWindowUtils
> works even on elements that have their own blur() and focus() methods.
Is it worth mentioning that in the comment then?
Comment 25 neil@parkwaycc.co.uk 2009-09-01 16:05:47 PDT
Pushed changeset db3f0d4e9082 to comm-central.
Comment 26 Tomas Janousek 2009-09-22 06:18:27 PDT
*** Bug 485898 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.