Using blur() on some unfocused element also blurs the focused element

RESOLVED FIXED in mozilla1.8.1beta2

Status

()

Core
DOM: Events
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Martijn Wargers (dead))

Tracking

(Depends on: 1 bug, {testcase})

Trunk
mozilla1.8.1beta2
x86
Windows XP
testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 5 obsolete attachments)

(Assignee)

Description

12 years ago
See upcoming testcase.
With the testcase, I would expect to get a focused text input, with a green background-color, but that's not what I get.
Also happens in Mozilla1.7.
(Assignee)

Comment 1

12 years ago
Created attachment 215291 [details]
testcase
(Assignee)

Comment 2

12 years ago
Created attachment 216150 [details] [diff] [review]
patch

This makes it work for html elements, but it also needs to work for xul elements.
Maybe I should make a method in nsGenericElement called shouldBlur(), which has this code in it, and then I can change the existing code in nsHTMLElement::blur from shouldFocus to shouldBlur, perhaps.
(Assignee)

Comment 3

12 years ago
Created attachment 216354 [details]
testcase2, xul

Testcase2, a xul testcase.
Note that this testcase 'works' in current builds, because ::blur() for xul elements doesn't do anything. 
The patch that I'm attaching should fix that.
(Assignee)

Comment 4

12 years ago
Created attachment 216356 [details] [diff] [review]
patch2

Like this, I haven't tested the patch yet.
Also, I'd rather remove all the ::blur and ::focus functions in the various htmlelement files and just keep it in nsgenerichtmlelement.cpp.
They all look the same, so it seems to me that should be possible, not?
Attachment #216150 - Attachment is obsolete: true
(Assignee)

Comment 5

12 years ago
Comment on attachment 216356 [details] [diff] [review]
patch2

Ok, the patch works fine for the testcases.
Attachment #216356 - Flags: review?(jst)

Comment 6

11 years ago
In addition to being the right thing to do in my eyes, I'd like to see this fixed for 1.8.1 because it will provide a branch solution for bug 306173.
Flags: blocking1.8.1?

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+

Comment 7

11 years ago
-> martijn

Jst: please review as soon as possible.  thanks!
Assignee: events → martijn.martijn

Updated

11 years ago
Target Milestone: --- → mozilla1.8.1beta2
Comment on attachment 216356 [details] [diff] [review]
patch2

Sorry for not getting to this sooner, this one fell through the cracks and got lost.

The change looks good to me. The one change I'm not 100% sure we want on the branch is the removal of nsXULElement::SetFocus(). That change makes us inherit a SetFocus() method that's different, the difference is code that scrolls the focused frame into view (which seems like the right thing to do), and I'm not sure we want to change that on the branch this late.

r=jst, bz or dbaron should sr this one.
Attachment #216356 - Flags: review?(jst) → review+
(Assignee)

Updated

11 years ago
Attachment #216356 - Flags: superreview?(dbaron)

Comment 9

11 years ago
I'd love to see this patch in for 1.8.1 - but given it exists in 1.8 I'm pulling off the blocking list for now. If we can get a patch together that addresses comment 8, is SR'd and ready to go we'd consider.
Flags: blocking1.8.1+ → blocking1.8.1-
Comment on attachment 216356 [details] [diff] [review]
patch2

I don't actually understand the nsXULElement changes.  Could you explain those?

Everything else seems fine, although I'd like bryner to review the nsGenericElement.cpp changes.
Attachment #216356 - Flags: review?(bryner)
(Assignee)

Comment 11

11 years ago
(In reply to comment #10)
> (From update of attachment 216356 [details] [diff] [review] [edit])
> I don't actually understand the nsXULElement changes.  Could you explain those?

nsXULElement::RemoveFocus wasn't implemented for xul elements, this patch is adding that support.
nsXULElement::SetFocus can be removed, because there is a  nsGenericElement::SetFocus.
But like Johnny mentioned in comment 8, that function is slightly different, in the sense that it also scrolls the frame into view. But I think, this should also be the behavior for xul elements.
Attachment #216356 - Flags: review?(bryner)
Comment on attachment 216356 [details] [diff] [review]
patch2

ok, sr=dbaron.  Sorry for the delay.
Attachment #216356 - Flags: superreview?(dbaron) → superreview+
(Assignee)

Comment 13

11 years ago
Created attachment 248592 [details] [diff] [review]
Updated patch2

Updated to current trunk.
I forgot the diff from nsXULElement.h in my previous patch, but that's nothing really new, so I'll check this in tomorrow.
(Assignee)

Comment 14

11 years ago
Created attachment 248646 [details] [diff] [review]
updated patch2

Sorry, there was an error in my updated patch (changed code in nsHTMLButtonElement::Focus() instead of nsHTMLButtonElement::Blur()), this is correct and I will check this in shortly.
Attachment #248592 - Attachment is obsolete: true
(Assignee)

Comment 15

11 years ago
Created attachment 248652 [details] [diff] [review]
updated patch2

Sorry, uploaded wrong patch.
Attachment #248646 - Attachment is obsolete: true
(Assignee)

Comment 16

11 years ago
Checking in content/base/src/nsGenericElement.cpp;
/cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v  <--  nsGenericElement.
cpp
new revision: 3.522; previous revision: 3.521
done
Checking in content/base/src/nsGenericElement.h;
/cvsroot/mozilla/content/base/src/nsGenericElement.h,v  <--  nsGenericElement.h
new revision: 3.237; previous revision: 3.236
done
Checking in content/html/content/src/nsGenericHTMLElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v  <--  nsGen
ericHTMLElement.cpp
new revision: 1.683; previous revision: 1.682
done
Checking in content/html/content/src/nsHTMLAnchorElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLAnchorElement.cpp,v  <--  nsHTML
AnchorElement.cpp
new revision: 1.148; previous revision: 1.147
done
Checking in content/html/content/src/nsHTMLButtonElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLButtonElement.cpp,v  <--  nsHTML
ButtonElement.cpp
new revision: 1.153; previous revision: 1.152
done
Checking in content/html/content/src/nsHTMLSelectElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLSelectElement.cpp,v  <--  nsHTML
SelectElement.cpp
new revision: 1.260; previous revision: 1.259
done
Checking in content/html/content/src/nsHTMLTextAreaElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLTextAreaElement.cpp,v  <--  nsHT
MLTextAreaElement.cpp
new revision: 1.193; previous revision: 1.192
done
Checking in content/xul/content/src/nsXULElement.cpp;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.cpp,v  <--  nsXULElement.c
pp
new revision: 1.672; previous revision: 1.671
done
Checking in content/xul/content/src/nsXULElement.h;
/cvsroot/mozilla/content/xul/content/src/nsXULElement.h,v  <--  nsXULElement.h
new revision: 1.229; previous revision: 1.228
done

Checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Depends on: 363881
Depends on: 363884
(Assignee)

Comment 17

11 years ago
Argh! The regression are caused because it used to be possible for xul elements to be always focused when the focus() method was called. That is bug 324156. I opened this bug to handle the blur() issue, but somehow I forgot about that bug and the removal of ::setfocus crept in the patch again. I should have set a dependancy on that bug, so I wouldn't forget about that one.
So I think if I would put back the nsXULElement::SetFocus method in, then the regressions will go away.
Depends on: 324156
(Assignee)

Comment 18

11 years ago
Created attachment 248703 [details] [diff] [review]
Put setfocus back in

I think this needs be put back in, it fixes the regressions. 
It can be removed eventually, but that should be done in bug 324156, and more code needs to be changed to prevent the regression as seen here.
(Assignee)

Comment 19

11 years ago
Ok, the (In reply to comment #18)
> Created an attachment (id=248703) [edit]
> Put setfocus back in

Ok, this is put back in (approved by dbaron). The regressions should be fixed now.
This bug remains fixed, the removal of nsxulelement::setfocus will be dealt with in 324156.
Depends on: 363934

Updated

11 years ago
Depends on: 364969

Comment 20

11 years ago
Created attachment 255519 [details]
MochiTest-ified version of "testcase"

Martijn, here is my attempt to make your testcase into a MochiTest testcase. If it looks OK to you, please check it in. :)

On a side note, I do not pass the XUL testcase (attachment 216354 [details]) on a current Linux trunk build? Is this to be expected?
Attachment #255519 - Flags: review?(martijn.martijn)
(Assignee)

Comment 21

11 years ago
Created attachment 255632 [details] [diff] [review]
mochitests for xul and html version

Thanks, Adam, your mochitest-ified version looks fine to me. I only changed the wording inside the ok() function.
I don't know much about writing testcases for mochikit, so I would appreciate it if Nickolay (or someone else) could take a look at this.
Attachment #255632 - Flags: review?(asqueella)
(Assignee)

Updated

11 years ago
Attachment #255632 - Attachment is patch: true
Attachment #255632 - Attachment mime type: application/octet-stream → text/plain

Comment 22

11 years ago
Comment on attachment 255632 [details] [diff] [review]
mochitests for xul and html version

>+                test_bug330705-2.xul \

This looks slightly weird in html/content.

>+  document.getElementsByTagName('input')[0].focus();
>+  document.getElementsByTagName('button')[0].blur();

Why do you do this before onload in the HTML testcase but after the document is loaded in the XUL case?

>+  <box tabindex="1" style="-moz-user-focus:normal;"/>
>+  <box tabindex="1" style="-moz-user-focus:normal;"/>

I think the idea is that these should go into the <p id="display"></p>. That's what the other testcases do anyway.

>+        var isFocused = false;
>+        var box = document.getElementsByTagName('box')[0];
>+        box.addEventListener('focus', function() { isFocused = true; }, true);
>+        box.addEventListener('blur', function() { isFocused = false;}, true);
>+
Is there a reason this is not in the onload handler?

>+        function doe() {
No that it matters, but why not "function onLoad"? Also I think you should use SimpleTest.waitForExplicitFinish() / SimpleTest.finish(), even though it appears to work as is. And the mochikit way to specify load handlers is via addLoadEvent(func)

Note that the last time I added a mochitest I was told to get a review from the peer of the code involved. (But today in #developers it was said you don't need review for reftests... We definitely need a policy on this written down.)
(Assignee)

Comment 23

11 years ago
(In reply to comment #22)
> (From update of attachment 255632 [details] [diff] [review])
> >+                test_bug330705-2.xul \
> 
> This looks slightly weird in html/content.

Well, there isn't a xul/content/test directory, as far as I can see. Where should I put it then?

> >+  document.getElementsByTagName('input')[0].focus();
> >+  document.getElementsByTagName('button')[0].blur();
> 
> Why do you do this before onload in the HTML testcase but after the document is
> loaded in the XUL case?

I wanted to do it before onload in the XUL case, but that didn't seem to work, the box element didn't seem to get focused then. (seems like a bug)

> Is there a reason this is not in the onload handler?

No specific reason, I didn't know that was preferred.

> >+        function doe() {
> No that it matters, but why not "function onLoad"? Also I think you should use
> SimpleTest.waitForExplicitFinish() / SimpleTest.finish(), even though it
> appears to work as is. And the mochikit way to specify load handlers is via
> addLoadEvent(func)

Ok, I'll use SimpleTest.waitForExplicitFinish() / SimpleTest.finish(). Can I find info on what those functions are doing? I couldn't really make sense of the mochikit js files.

Comment 24

11 years ago
(In reply to comment #23)
> (In reply to comment #22)
> > (From update of attachment 255632 [details] [diff] [review] [details])
> > >+                test_bug330705-2.xul \
> > 
> > This looks slightly weird in html/content.
> 
> Well, there isn't a xul/content/test directory, as far as I can see. Where
> should I put it then?
> 
You could create one :) Or check it in html/content if no-one minds, I suppose.

> > Is there a reason this is not in the onload handler?
> 
> No specific reason, I didn't know that was preferred.
> 
> > >+        function doe() {
> > No that it matters, but why not "function onLoad"? Also I think you should use
> > SimpleTest.waitForExplicitFinish() / SimpleTest.finish(), even though it
> > appears to work as is. And the mochikit way to specify load handlers is via
> > addLoadEvent(func)
> 
> Ok, I'll use SimpleTest.waitForExplicitFinish() / SimpleTest.finish(). Can I
> find info on what those functions are doing? I couldn't really make sense of
> the mochikit js files.
> 
Just see what other tests do. I should write some docs on this, but basically mochikit's SimpleTest "stops" the test onload (whatever that means), so if you want to do some asynchronous testing after onload, you should call waitForExplicitFinish before onload and then call finish() when you're done testing.
(Assignee)

Comment 25

11 years ago
Created attachment 255679 [details] [diff] [review]
attempt 2

Ok, I think this addresses all of your comments. I changed test_bug330705-1.html too, to be more in line with mochikit convention.
Attachment #255519 - Attachment is obsolete: true
Attachment #255632 - Attachment is obsolete: true
Attachment #255679 - Flags: review?(asqueella)
Attachment #255632 - Flags: review?(asqueella)
Attachment #255519 - Flags: review?(martijn.martijn)

Comment 26

11 years ago
Comment on attachment 255679 [details] [diff] [review]
attempt 2

>Index: content/html/content/test/Makefile.in

>+                test_bug330705-1.html \

Use tab characters here for consistency.

>+        addLoadEvent(SimpleTest.finish);

Better call it in the onLoad method. Passing methods like this is error prone.

>Index: content/xul/content/test/test_bug330705-2.xul

>+        addLoadEvent(SimpleTest.finish);

Same.

Other than that, looks fine.
Attachment #255679 - Flags: review?(asqueella) → review+
(Assignee)

Comment 27

11 years ago
Created attachment 255682 [details] [diff] [review]
for checkin

Thanks, Nickolay.
(Assignee)

Comment 28

11 years ago
Checking in html/content/test/Makefile.in;
/cvsroot/mozilla/content/html/content/test/Makefile.in,v  <--  Makefile.in
new revision: 1.7; previous revision: 1.6
done
RCS file: /cvsroot/mozilla/content/html/content/test/test_bug330705-1.html,v
done
Checking in html/content/test/test_bug330705-1.html;
/cvsroot/mozilla/content/html/content/test/test_bug330705-1.html,v  <--  test_bu
g330705-1.html
initial revision: 1.1
done
Checking in xul/content/Makefile.in;
/cvsroot/mozilla/content/xul/content/Makefile.in,v  <--  Makefile.in
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvsroot/mozilla/content/xul/content/test/Makefile.in,v
done
Checking in xul/content/test/Makefile.in;
/cvsroot/mozilla/content/xul/content/test/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/content/xul/content/test/test_bug330705-2.xul,v
done
Checking in xul/content/test/test_bug330705-2.xul;
/cvsroot/mozilla/content/xul/content/test/test_bug330705-2.xul,v  <--  test_bug3
30705-2.xul
initial revision: 1.1
done
Flags: in-testsuite? → in-testsuite+

Comment 29

11 years ago
(In reply to comment #22)

> 
> Note that the last time I added a mochitest I was told to get a review from the
> peer of the code involved. (But today in #developers it was said you don't need
> review for reftests... We definitely need a policy on this written down.)
> 

We really need some level of review.  Here on the Mozilla project, more than most software projects, we rely on developer unit tests to ensure the feature/bug is in good shape and to detect regressions in the future.  Reviewing the tests is an important part of that. Tests are critical and should be reviewed just like product code and just like tool changes.  I can get that written down.  Any suggestions about where?  I'll check on this in #developers also.

Comment 30

11 years ago
Tim: you could put it on wiki.m.o for starters and post an announcement to the newsgroups. It could then be added to one of our pages on mozilla.org about the review process.

Comment 31

11 years ago
Shouldn't the makefile change use tabs instead of spaces?
(Assignee)

Comment 32

11 years ago
Sorry! I thought I had that fixed, but clearly not :(
I've just fixed it.

Comment 33

11 years ago
(In reply to comment #29)
> (In reply to comment #22)
> 
> > 
> > Note that the last time I added a mochitest I was told to get a review from the
> > peer of the code involved. 
> 
> We really need some level of review.  

The person who writes the patch should be adding automated testcases in the same patch. An easy solution to this problem is not to grant review to patches without unit tests--that is my policy on things I own.

(Assignee)

Comment 34

11 years ago
What about the occasional/first-time contributor? Should he be writing unit tests too?

Comment 35

11 years ago
(In reply to comment #34)
> What about the occasional/first-time contributor? Should he be writing unit
> tests too?
> 

Yes (or the module owner should write the tests for them). I am not sure why we would want untested code from infrequent or first-time contributors.
Because we want to encourage them to keep contributing, and because figuring out how a test harness works is not always easy.
(I'm not disagreeing with you, just answering the question.)

Comment 38

11 years ago
Could we move this discussion to the newsgroups? It has little to do with DOM events...
Depends on: 665677
You need to log in before you can comment on or make changes to this bug.