Closed
Bug 472302
Opened 16 years ago
Closed 16 years ago
get rid of fragile timeouts in textbox binding (intermittent test_bug418874.xul failure)
Categories
(Toolkit :: UI Widgets, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: sgautherie, Assigned: dao)
References
Details
(Keywords: fixed1.9.1, intermittent-failure)
Attachments
(1 file, 5 obsolete files)
8.62 KB,
patch
|
enndeakin
:
review+
benjamin
:
approval1.9.1+
|
Details | Diff | Splinter Review |
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1231133054.1231137464.20650.gz
Linux mozilla-central unit test on 2009/01/04 21:24:14
*** 661 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_bug418874.xul | undo correctly enabled when emptyText was not changed through property
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1231247867.1231251851.2268.gz
Linux comm-central dep unit test on 2009/01/06 05:17:47
}
Flags: wanted1.9.1?
Comment 1•16 years ago
|
||
At first, I was wondering if it was maybe a focus issue, with t1 not getting the key event.
However, until bug 471776 lands, undo would be enabled as soon as the emptytext is inserted, even if nothing has been typed. So, could this be a timing issue - are we reaching the code at http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/test_bug418874.xul#80 before the timeout in the textbox's constructor has run?
It may be useful to see the value of t1.inputField.value immediately before that call to check if it is indeed the problem.
Assignee | ||
Comment 2•16 years ago
|
||
(In reply to comment #1)
> So, could this be a timing issue -
> are we reaching the code at
> http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/test_bug418874.xul#80
> before the timeout in the textbox's constructor has run?
That wouldn't cause undo to be disabled, but it could be that the timeout within _updateVisibleText hasn't been reached.
Comment 3•16 years ago
|
||
> That wouldn't cause undo to be disabled, but it could be that the timeout
> within _updateVisibleText hasn't been reached.
Which would cause undo to be disabled - if indeed the emptytext has never been inserted, then there wouldn't be anything to undo...
Dão, are you able to review this at all? I'm sure I've seen you r+ things before, but couldn't see you on the toolkit review requirments wiki?.
Assignee | ||
Comment 4•16 years ago
|
||
Comment on attachment 355755 [details] [diff] [review]
Check first textbox after timeout
There's is a bug in the implementation, not in the test, so this doesn't seem right.
Assignee | ||
Comment 5•16 years ago
|
||
I couldn't reproduce the original reasons for the timeouts -- I assume those issues got fixed elsewhere.
Attachment #355771 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 6•16 years ago
|
||
(In reply to comment #3)
> Dão, are you able to review this at all? I'm sure I've seen you r+ things
> before, but couldn't see you on the toolkit review requirments wiki?.
I'm not a peer, but you can always ask me for a first review.
Assignee | ||
Comment 7•16 years ago
|
||
Attachment #355771 -
Attachment is obsolete: true
Attachment #355776 -
Flags: review?(enndeakin)
Attachment #355771 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 8•16 years ago
|
||
Comment on attachment 355776 [details] [diff] [review]
avoid the timeouts (with updated tests)
I just discovered bug 472515, which is critical for this patch...
Attachment #355776 -
Flags: review?(enndeakin)
Assignee | ||
Comment 9•16 years ago
|
||
I imagine this could fix bug 472515, but I still need to run the tests and actually test it myself.
Attachment #355755 -
Attachment is obsolete: true
Attachment #355776 -
Attachment is obsolete: true
Assignee | ||
Comment 10•16 years ago
|
||
Attachment #355783 -
Attachment is obsolete: true
Attachment #355820 -
Flags: review?(enndeakin)
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → dao
Status: NEW → ASSIGNED
Summary: test_bug418874.xul intermittently fails → get rid of fragile timeouts in textbox binding (intermittent test_bug418874.xul failure)
Version: Trunk → 1.9.1 Branch
Comment 11•16 years ago
|
||
So to clarify, from the patch is looks to:
1. remove the timeout from the constructor
2. no longer calls _clearEmptyText when the <textbox> is focused, or one of its non-<input> descendants, only when the <input> is focused.
Assignee | ||
Comment 12•16 years ago
|
||
(In reply to comment #11)
> So to clarify, from the patch is looks to:
> 1. remove the timeout from the constructor
Yes, and from _updateVisibleText (also called from the constructor).
> 2. no longer calls _clearEmptyText when the <textbox> is focused, or one of its
> non-<input> descendants, only when the <input> is focused.
Yeah... I did this to mitigate the flashing emptytext when the textbox is focused shortly after _updateVisibleText, but that wasn't fully successful, which led to "emptytextdelay".
I think I originally avoided that (calling _clearEmptyText only when the <input> is focused) just because it didn't fit into the "if, else if, else if, else if" construct, and because I assumed that calling it for all focus events would be harmless.
Comment 13•16 years ago
|
||
(In reply to comment #12)
> Yeah... I did this to mitigate the flashing emptytext when the textbox is
> focused shortly after _updateVisibleText
When would this situation occur?
> which led to "emptytextdelay".
I think I'd rather have this attribute on the inner element such as the hbox so that isn't visible on the textbox itself, and that setting emptytextdelay on the textbox doesn't make the text invisible.
Assignee | ||
Comment 14•16 years ago
|
||
(In reply to comment #13)
> > Yeah... I did this to mitigate the flashing emptytext when the textbox is
> > focused shortly after _updateVisibleText
>
> When would this situation occur?
For instance in the location bar when opening a new tab.
Attachment #355820 -
Attachment is obsolete: true
Attachment #355851 -
Flags: review?(enndeakin)
Attachment #355820 -
Flags: review?(enndeakin)
Updated•16 years ago
|
Attachment #355851 -
Flags: review?(enndeakin) → review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 15•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Updated•16 years ago
|
Attachment #355851 -
Flags: approval1.9.1?
Comment 16•16 years ago
|
||
Comment on attachment 355851 [details] [diff] [review]
set the emptytextdelay attribute on the inputField
>+textbox[empty="true"] html|*.textbox-input[emptytextdelay="true"] ,
>+textbox[empty="true"] html|*.textbox-textarea[emptytextdelay="true"] {
>+ color: transparent !important;
>+}
Wouldn't it be better to add empty to the xbl:inherits list for all textbox-input/textareas to avoid the descendent selector?
Assignee | ||
Comment 17•16 years ago
|
||
(In reply to comment #16)
> Wouldn't it be better to add empty to the xbl:inherits list for all
> textbox-input/textareas to avoid the descendent selector?
One disadvantage would be that bindings that extend the textbox binding and define their own content would have to catch up. emptytext currently works for pretty much any textbox out there.
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite+
Updated•16 years ago
|
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Updated•16 years ago
|
Attachment #355851 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 18•16 years ago
|
||
Keywords: checkin-needed → fixed1.9.1
Comment 19•16 years ago
|
||
Looks like this patch caused a test to leak on SeaMonkey that just opens the browser window and checks for duplicate XUL ids, see bug 473686 comment #9
Reporter | ||
Comment 20•16 years ago
|
||
It looks like the test failure is not fully fixed:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1233065517.1233069849.5053.gz
Linux comm-central dep unit test on 2009/01/27 06:11:57
{
*** 5289 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_bug418874.xul | undo correctly enabled when emptyText was not changed through property
*** 5291 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_bug418874.xul | undo correctly enabled when emptyText explicitly changed through property
}
Reporter | ||
Comment 21•16 years ago
|
||
(In reply to comment #20)
Another occurrence:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1233525079.1233529580.26807.gz
Linux comm-central dep unit test on 2009/02/01 13:51:19
Updated•16 years ago
|
Whiteboard: [orange]
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•12 years ago
|
Keywords: intermittent-failure
Updated•12 years ago
|
Whiteboard: [orange]
You need to log in
before you can comment on or make changes to this bug.
Description
•