Closed Bug 299677 Opened 16 years ago Closed 15 years ago

Almost every element can steal focus from another tab with focus or blur method

Categories

(SeaMonkey :: Tabbed Browser, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.8beta4

People

(Reporter: martijn.martijn, Assigned: bryner)

References

Details

(4 keywords, Whiteboard: [sg:fix])

Attachments

(6 files)

See upcoming testcase.
To reproduce:
- Open testcase in new tab in background.
- Try to type some text in the bugzilla textarea.
You'll see that the result of you typing shows up in the testcase's tab.
Attached file testcase
This uses the <span> element. But it doesn't matter which element I use,
because with tabindex="1" or overflow:auto or -moz-user-focus:normal for xul I
can make virtually every element focusable.
Attached file testcase2, using blur
Same testcase, but now using .blur() which has the same effect.
(In reply to comment #3)
> fgdfgdd

Sorry was trying the test case and cc myself.

With
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050703
Firefox/1.0+ 

I find that trying to type into the bugzilla text box also activates the FATY
toolbar, the text then disapears from the FAYT every second and is copied to the
other tab.
observed with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050703 Firefox/1.0+
Attached patch patchaSplinter Review
This patch prevents the blur() method from stealing focus on the -already
patched for focus- elements.
Attached patch patchbSplinter Review
This prevents focus stealing on every other html element.
Attached patch patchcSplinter Review
This patch adds a check if the window has focus when an element calls focus()
or blur().
I've more or less randomly copied it from somewhere else.

Patcha fixes bug 138646.
The combination of patcha and patchc fixes bug 258285. I suspect it could also
fix bug 187083, since I get the same phenomenon when I try out "minimized test
case" in bug 258285.
Brian, basically Boris told me to cc you. Any suggestions?
Attachment #189796 - Flags: review?(bryner)
Flags: blocking1.8b4?
Flags: blocking-aviary1.0.7?
Whiteboard: [sg:fix]
Flags: blocking1.8b4? → blocking1.8b4+
Re-assigning to bryner as per drivers' security bug pass.

Brian: can you take a look at the attached patch, and drive this in for 1.8b4?
Assignee: nobody → bryner
Target Milestone: --- → mozilla1.8beta4
Blocks: 140346
Attachment #189796 - Flags: review?(bryner) → review+
cc'ing aaron in case he has any thoughts on these patches
Attachment #189797 - Flags: review+
Comment on attachment 189806 [details] [diff] [review]
patchc

This seems like it would prevent, among other things, putting focus onto an
element in an iframe if the iframe isn't focused.  I can imagine that breaking
a lot of things.  I haven't applied this patch to test though... does what I
said actually happen?
Attachment #189806 - Flags: review-
Yes, I think you're right. That's probably what patchc does. So it isn't any
good. When I tested it at that time, I didn't test it very good.
I didn't test it for the situation you described.
Comment on attachment 189796 [details] [diff] [review]
patcha

jst, look ok? (you added the similar checks for focus())
Attachment #189796 - Flags: superreview?(jst)
Attachment #189797 - Flags: superreview?(jst)
Comment on attachment 189796 [details] [diff] [review]
patcha

Is the visibility check in nsIFrame::IsFocusable() not enough?
Comment on attachment 189797 [details] [diff] [review]
patchb

sr=jst
Attachment #189797 - Flags: superreview?(jst) → superreview+
Attachment #189796 - Flags: superreview?(jst) → superreview+
Attachment #189796 - Flags: approval1.8b4?
Attachment #189797 - Flags: approval1.8b4?
(In reply to comment #16)
> (From update of attachment 189796 [details] [diff] [review] [edit])
> Is the visibility check in nsIFrame::IsFocusable() not enough?
Aaron, I don't know. I hope Brian of jst can answer that question.
(In reply to comment #16)
> (From update of attachment 189796 [details] [diff] [review] [edit])
> Is the visibility check in nsIFrame::IsFocusable() not enough?

That doesn't get invoked for this code path, as far as I can tell (only seems 
to be used for tabbing).  Seems like we should unify these checks, but I'd go 
with consistency between focus() and blur() for the moment.
please land this on the trunk and we'll evaluate for branch after it's proved
itself. 
This ("patcha" and "patchb") landed on the trunk yesterday.  I'd like to verify
whether the original testcases are fixed without "patchc" which I think would
break other things.
Verified that the patches landed fix the two testcases in the bug.  I think this
is safe to land on the branch.
Attachment #189796 - Flags: approval1.8b4? → approval1.8b4+
Attachment #189797 - Flags: approval1.8b4? → approval1.8b4+
Could this possibly have caused bug 306076?
Checked in patcha and patchb on the branch.
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
This might have caused bug 306173 (Mac only?)
Depends on: 306173
Flags: blocking1.7.13?
Keywords: qawanted
Is this really a problem in ff1.0/1.7? The attached testcases don't seem to work there.
Yes, sorry, the testcase uses a feature that isn't in 1.7 builds, e.g. setting tabindex attribute makes the element focusable.
I could quite easily make a testcase that also works in 1.7 builds, though.
So this is really a problem in ff1.0/1.7.
(In reply to comment #27)
> I could quite easily make a testcase that also works in 1.7 builds, though.
> So this is really a problem in ff1.0/1.7.

Could you do that please? Would help ensure this fix gets tested that it's backported correctly
Flags: blocking1.7.13?
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8?
Flags: blocking-aviary1.0.8+
Hmm, actually, I haven't been able to make a testcase that shows this bug in Mozilla1.7.12.
What did you try? -moz-user-focus?
Yes, also tried it with anchors and select elements, I just can't get the bug to show in Mozilla1.7.12
per comments, not needed in old branches
Flags: blocking1.7.13-
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8-
Flags: blocking-aviary1.0.8+
Ugh, I forgot to fix nsHTMLButtonElement.cpp. 
Status: RESOLVED → REOPENED
Flags: blocking1.8.0.3?
Resolution: FIXED → ---
Attached patch button patchSplinter Review
Patch that should fix the button blur case.
I'm wondering why nshtmlbuttonelement.cpp and others have ::blur and ::focus methods, since they all look the same as nsgenerichtmlelement.cpp.
I think they could safely be removed, couldn't they?
Attachment #216353 - Flags: review?(jst)
Removed QAWanted.  This was from dveditz on 2006-02-01 and we have since minused it for 1.0.x/1.7.x.  Let us know if anyone still wanted QAWanted.
Keywords: qawanted
Could we get more guidance on why this should go into a .0.x release? The benefit vs. risk picture is unclear.
(In reply to comment #36)
> Could we get more guidance on why this should go into a .0.x release? The
> benefit vs. risk picture is unclear.
There should be no risk here. As you can see in "patcha" and "patchb", the other elements alrady got the same treatment.
Without this patch, there is some risk that some website could steal sensitive information.

Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment on attachment 216353 [details] [diff] [review]
button patch

sr=dveditz
Attachment #216353 - Flags: superreview+
Attachment #216353 - Flags: approval1.8.0.3?
Attachment #216353 - Flags: approval-branch-1.8.1?(jst)
jst- can we get a review on this!
Attachment #216353 - Flags: review?(jst)
Attachment #216353 - Flags: review?(bryner)
Attachment #216353 - Flags: approval-branch-1.8.1?(jst)
Attachment #216353 - Flags: approval-branch-1.8.1?(bryner)
Comment on attachment 216353 [details] [diff] [review]
button patch

looks good
Attachment #216353 - Flags: review?(bryner)
Attachment #216353 - Flags: review+
Attachment #216353 - Flags: approval-branch-1.8.1?(bryner)
Attachment #216353 - Flags: approval-branch-1.8.1+
Comment on attachment 216353 [details] [diff] [review]
button patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #216353 - Flags: approval1.8.0.3? → approval1.8.0.3+
follow-up button fix checked into trunk, 1.8 branch, and 1.8.0 branch.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.