Closed
Bug 323806
Opened 19 years ago
Closed 15 years ago
Fix for bug 175893 breaks tab switching if content is not HTML
Categories
(SeaMonkey :: General, defect)
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)
1.27 KB,
patch
|
Details | Diff | Splinter Review | |
1.43 KB,
patch
|
neil
:
review+
neil
:
superreview+
iannbugzilla
:
approval-seamonkey1.0+
csthomas
:
approval-seamonkey1.1a+
|
Details | Diff | Splinter Review |
1.59 KB,
patch
|
iannbugzilla
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•19 years ago
|
Flags: blocking-seamonkey1.0?
Updated•19 years ago
|
Flags: blocking-seamonkey1.0? → blocking-seamonkey1.0+
Comment 1•18 years ago
|
||
(bug 323805 has a patch for firefox)
Comment 2•18 years ago
|
||
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•18 years ago
|
||
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•18 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•18 years ago
|
||
hmm... what null test would that be?
Comment 6•18 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•18 years ago
|
||
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•18 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•18 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•18 years ago
|
Attachment #209323 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #209323 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 11•18 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•18 years ago
|
||
Comment on attachment 209463 [details] [diff] [review] instanceof check without the null check (assuming comment added and indentation fixed)
Comment 13•18 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
Reporter | ||
Updated•18 years ago
|
Flags: blocking-seamonkey1.1a?
Comment 14•18 years ago
|
||
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?
Reporter | ||
Comment 15•18 years ago
|
||
Doesn't this cause bug 327139 in seamonkey too?
Comment 16•18 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•18 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.
Updated•18 years ago
|
Keywords: fixed-seamonkey1.0
Whiteboard: fixed-seamonkey1.0
Comment 18•17 years ago
|
||
Attachment #272303 -
Flags: review?
Comment 19•17 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•15 years ago
|
Attachment #272303 -
Flags: review?(cst) → review?(iann_bugzilla)
Comment 21•15 years ago
|
||
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•15 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?
Comment 23•15 years ago
|
||
(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•15 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?
Attachment #272303 -
Flags: review?(iann_bugzilla) → review+
Comment 25•15 years ago
|
||
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.
Description
•