Closed Bug 323806 Opened 18 years ago Closed 15 years ago

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

Categories

(SeaMonkey :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bzbarsky, Unassigned)

References

()

Details

(Keywords: fixed-seamonkey1.0, fixed1.8.0.1, fixed1.8.1)

Attachments

(3 files, 1 obsolete file)

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.
Flags: blocking-seamonkey1.0?
Flags: blocking-seamonkey1.0? → blocking-seamonkey1.0+
Attached patch just try/catchSplinter Review
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.
Attachment #209323 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209323 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch use instanceof check (obsolete) — Splinter Review
Neil suggested this as an alternative for branch
Attachment #209427 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209427 - Flags: review?(neil.parkwaycc.co.uk)
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.
hmm... what null test would that be?
Comment on attachment 209427 [details] [diff] [review]
use instanceof check

>               if (this.mCurrentBrowser.focusedElement) {
Just replace this test with the improved test.
Attachment #209427 - Attachment is obsolete: true
Attachment #209463 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209463 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #209427 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209427 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #209463 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209463 - Flags: superreview+
Attachment #209463 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #209463 - Flags: review+
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.
Attachment #209463 - Flags: approval-seamonkey1.1?
Attachment #209463 - Flags: approval-seamonkey1.0?
Comment on attachment 209463 [details] [diff] [review]
instanceof check without the null check

first-a=me, need 1 more
Attachment #209463 - Flags: approval-seamonkey1.1? → approval-seamonkey1.1+
Attachment #209323 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209323 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 209463 [details] [diff] [review]
instanceof check without the null check

a=me for SM1.0 (2nd one)
Attachment #209463 - Flags: approval-seamonkey1.0? → approval-seamonkey1.0+
Comment on attachment 209463 [details] [diff] [review]
instanceof check without the null check

(assuming comment added and indentation fixed)
patch is in on the branches (with fixups), leaving open for a trunk fix
Whiteboard: fixed-seamonkey1.0
Flags: blocking-seamonkey1.1a?
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?
Flags: blocking-seamonkey1.1a?
Doesn't this cause bug 327139 in seamonkey too?
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.
(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.
Whiteboard: fixed-seamonkey1.0
Comment on attachment 272303 [details] [diff] [review]
Port from bug 323805

bah, :CTho stopped working
Attachment #272303 - Flags: review? → review?(cst)
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"
Attachment #272303 - Flags: review?(cst) → review?(iann_bugzilla)
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 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?
(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.
(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?
Attachment #272303 - Flags: review?(iann_bugzilla) → review+
Pushed changeset db3f0d4e9082 to comm-central.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.