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

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Martijn Wargers (dead)]

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.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: 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.

Comment 3

[Martijn Wargers (dead)]

Attachment: 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.

Comment 4

[Martijn Wargers (dead)]

Attachment: 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?

Comment 5

[Martijn Wargers (dead)]

Comment on attachment 216356 [details] [diff] [review]
patch2

Ok, the patch works fine for the testcases.

Comment 6

[Mark Mentovai]

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.

Comment 7

[Darin Fisher]

-> martijn

Jst: please review as soon as possible.  thanks!

Comment 8

[Johnny Stenback (:jst)]

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.

Comment 9

[Mike Schroepfer]

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 11

[Martijn Wargers (dead)]

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

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 216356 [details] [diff] [review]
patch2

ok, sr=dbaron.  Sorry for the delay.

Comment 13

[Martijn Wargers (dead)]

Attachment: 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.

Comment 14

[Martijn Wargers (dead)]

Attachment: 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.

Comment 15

[Martijn Wargers (dead)]

Attachment: updated patch2

Sorry, uploaded wrong patch.

Comment 16

[Martijn Wargers (dead)]

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.

Comment 17

[Martijn Wargers (dead)]

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.

Comment 18

[Martijn Wargers (dead)]

Attachment: 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.

Comment 19

[Martijn Wargers (dead)]

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.

Comment 20

[Adam Guthrie]

Attachment: 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?

Comment 21

[Martijn Wargers (dead)]

Attachment: 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.

Comment 22

[Nickolay_Ponomarev]

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

Comment 23

[Martijn Wargers (dead)]

(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

[Nickolay_Ponomarev]

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

Comment 25

[Martijn Wargers (dead)]

Attachment: 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.

Comment 26

[Nickolay_Ponomarev]

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.

Comment 27

[Martijn Wargers (dead)]

Attachment: for checkin

Thanks, Nickolay.

Comment 28

[Martijn Wargers (dead)]

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

Comment 29

[Tim Riley [:timr]]

(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

[Nickolay_Ponomarev]

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

[Boris Zbarsky [:bzbarsky]]

Shouldn't the makefile change use tabs instead of spaces?

Comment 32

[Martijn Wargers (dead)]

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

Comment 33

[Robert Sayre]

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

Comment 34

[Martijn Wargers (dead)]

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

Comment 35

[Robert Sayre]

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

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Because we want to encourage them to keep contributing, and because figuring out how a test harness works is not always easy.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I'm not disagreeing with you, just answering the question.)

Comment 38

[Nickolay_Ponomarev]

Could we move this discussion to the newsgroups? It has little to do with DOM events...