Closed
Bug 330705
Opened 19 years ago
Closed 18 years ago
Using blur() on some unfocused element also blurs the focused element
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8.1beta2
People
(Reporter: martijn.martijn, Assigned: martijn.martijn)
References
(Depends on 1 open bug)
Details
(Keywords: testcase)
Attachments
(7 files, 5 obsolete files)
493 bytes,
text/html
|
Details | |
591 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
9.01 KB,
patch
|
jst
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
9.96 KB,
patch
|
Details | Diff | Splinter Review | |
1.95 KB,
patch
|
Details | Diff | Splinter Review | |
7.60 KB,
patch
|
asqueella
:
review+
|
Details | Diff | Splinter Review |
7.58 KB,
patch
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Assignee | ||
Comment 2•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
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•19 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•19 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•19 years ago
|
Flags: blocking1.8.1? → blocking1.8.1+
Comment 7•19 years ago
|
||
-> martijn
Jst: please review as soon as possible. thanks!
Assignee: events → martijn.martijn
Updated•19 years ago
|
Target Milestone: --- → mozilla1.8.1beta2
Comment 8•19 years ago
|
||
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•19 years ago
|
Attachment #216356 -
Flags: superreview?(dbaron)
Comment 9•18 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•18 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.
Updated•18 years ago
|
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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
||
Sorry, uploaded wrong patch.
Attachment #248646 -
Attachment is obsolete: true
Assignee | ||
Comment 16•18 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
Closed: 18 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Assignee | ||
Comment 17•18 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•18 years ago
|
||
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•18 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.
Comment 20•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
Attachment #255632 -
Attachment is patch: true
Attachment #255632 -
Attachment mime type: application/octet-stream → text/plain
Comment 22•18 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•18 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•18 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•18 years ago
|
||
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•18 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•18 years ago
|
||
Thanks, Nickolay.
Assignee | ||
Comment 28•18 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•18 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•18 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•18 years ago
|
||
Shouldn't the makefile change use tabs instead of spaces?
Assignee | ||
Comment 32•18 years ago
|
||
Sorry! I thought I had that fixed, but clearly not :(
I've just fixed it.
Comment 33•18 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•18 years ago
|
||
What about the occasional/first-time contributor? Should he be writing unit tests too?
Comment 35•18 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•18 years ago
|
||
Could we move this discussion to the newsgroups? It has little to do with DOM events...
You need to log in
before you can comment on or make changes to this bug.
Description
•