The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.8beta4

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Brian Ryner (not reading))

Tracking

(4 keywords)

Trunk
mozilla1.8beta4
x86
Windows XP
fixed1.8, fixed1.8.0.4, fixed1.8.1, testcase
Dependency tree / graph
Bug Flags:
blocking1.7.13 -
blocking-aviary1.0.8 -
blocking1.8b5 +
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix])

Attachments

(6 attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
Created attachment 188245 [details]
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.
(Reporter)

Comment 2

12 years ago
Created attachment 188246 [details]
testcase2, using blur

Same testcase, but now using .blur() which has the same effect.
fgdfgdd
(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.
asda

Comment 6

12 years ago
observed with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050703 Firefox/1.0+
(Reporter)

Comment 7

12 years ago
Created attachment 189796 [details] [diff] [review]
patcha

This patch prevents the blur() method from stealing focus on the -already
patched for focus- elements.
(Reporter)

Comment 8

12 years ago
Created attachment 189797 [details] [diff] [review]
patchb

This prevents focus stealing on every other html element.
(Reporter)

Comment 9

12 years ago
Created attachment 189806 [details] [diff] [review]
patchc

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.
(Reporter)

Comment 10

12 years ago
Brian, basically Boris told me to cc you. Any suggestions?
(Reporter)

Updated

12 years ago
Attachment #189796 - Flags: review?(bryner)

Updated

12 years ago
Flags: blocking1.8b4?
Flags: blocking-aviary1.0.7?
Whiteboard: [sg:fix]

Updated

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

Updated

12 years ago
Blocks: 140346
(Assignee)

Updated

12 years ago
Attachment #189796 - Flags: review?(bryner) → review+
(Assignee)

Comment 12

12 years ago
cc'ing aaron in case he has any thoughts on these patches
(Assignee)

Updated

12 years ago
Attachment #189797 - Flags: review+
(Assignee)

Comment 13

12 years ago
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-
(Reporter)

Comment 14

12 years ago
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.
(Assignee)

Comment 15

12 years ago
Comment on attachment 189796 [details] [diff] [review]
patcha

jst, look ok? (you added the similar checks for focus())
Attachment #189796 - Flags: superreview?(jst)
(Assignee)

Updated

12 years ago
Attachment #189797 - Flags: superreview?(jst)

Comment 16

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

Updated

12 years ago
Attachment #189796 - Flags: superreview?(jst) → superreview+
(Reporter)

Updated

12 years ago
Attachment #189796 - Flags: approval1.8b4?
(Reporter)

Updated

12 years ago
Attachment #189797 - Flags: approval1.8b4?
(Reporter)

Comment 18

12 years ago
(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.
(Assignee)

Comment 19

12 years ago
(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.

Comment 20

12 years ago
please land this on the trunk and we'll evaluate for branch after it's proved
itself. 
(Assignee)

Comment 21

12 years ago
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.
(Assignee)

Comment 22

12 years ago
Verified that the patches landed fix the two testcases in the bug.  I think this
is safe to land on the branch.

Updated

12 years ago
Attachment #189796 - Flags: approval1.8b4? → approval1.8b4+

Updated

12 years ago
Attachment #189797 - Flags: approval1.8b4? → approval1.8b4+
Could this possibly have caused bug 306076?
(Assignee)

Comment 24

12 years ago
Checked in patcha and patchb on the branch.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
This might have caused bug 306173 (Mac only?)

Updated

12 years ago
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.
(Reporter)

Comment 27

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

Comment 29

11 years ago
Hmm, actually, I haven't been able to make a testcase that shows this bug in Mozilla1.7.12.

Comment 30

11 years ago
What did you try? -moz-user-focus?
(Reporter)

Comment 31

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

Comment 33

11 years ago
Ugh, I forgot to fix nsHTMLButtonElement.cpp. 
Status: RESOLVED → REOPENED
Flags: blocking1.8.0.3?
Resolution: FIXED → ---
(Reporter)

Comment 34

11 years ago
Created attachment 216353 [details] [diff] [review]
button patch

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)

Comment 35

11 years ago
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.
(Reporter)

Comment 37

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

Comment 39

11 years ago
jst- can we get a review on this!

Updated

11 years ago
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)
(Assignee)

Comment 40

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

Comment 42

11 years ago
follow-up button fix checked into trunk, 1.8 branch, and 1.8.0 branch.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Keywords: fixed1.8.0.3, fixed1.8.1
Resolution: --- → FIXED
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.