The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

SeaMonkey
General
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: bz, Unassigned)

Tracking

({fixed-seamonkey1.0, fixed1.8.0.1, fixed1.8.1})

Trunk
x86
Linux
fixed-seamonkey1.0, fixed1.8.0.1, fixed1.8.1
Dependency tree / graph
Bug Flags:
blocking-seamonkey1.0 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

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+
(bug 323805 has a patch for firefox)

Comment 2

11 years ago
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.
Attachment #209323 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209323 - Flags: review?(neil.parkwaycc.co.uk)

Comment 3

11 years ago
Created attachment 209427 [details] [diff] [review]
use instanceof check

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 4

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

11 years ago
hmm... what null test would that be?

Comment 6

11 years ago
Comment on attachment 209427 [details] [diff] [review]
use instanceof check

>               if (this.mCurrentBrowser.focusedElement) {
Just replace this test with the improved test.

Comment 7

11 years ago
Created attachment 209463 [details] [diff] [review]
instanceof check without the null check
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 8

11 years ago
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 9

11 years ago
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+

Updated

11 years ago
Attachment #209323 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209323 - Flags: review?(neil.parkwaycc.co.uk)

Comment 11

11 years ago
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 12

11 years ago
Comment on attachment 209463 [details] [diff] [review]
instanceof check without the null check

(assuming comment added and indentation fixed)

Comment 13

11 years ago
patch is in on the branches (with fixups), leaving open for a trunk fix
Keywords: fixed1.8.0.1, fixed1.8.1
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?

Comment 16

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

11 years ago
(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.
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0
Blocks: 282178

Comment 18

10 years ago
Created attachment 272303 [details] [diff] [review]
Port from bug 323805
Attachment #272303 - Flags: review?

Comment 19

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

Updated

8 years ago
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 22

8 years ago
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.

Comment 24

8 years ago
(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?

Updated

8 years ago
Attachment #272303 - Flags: review?(iann_bugzilla) → review+
Pushed changeset db3f0d4e9082 to comm-central.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Duplicate of this bug: 485898
You need to log in before you can comment on or make changes to this bug.