Closed Bug 472302 Opened 12 years ago Closed 12 years ago

get rid of fragile timeouts in textbox binding (intermittent test_bug418874.xul failure)

Categories

(Toolkit :: XUL Widgets, defect)

1.9.1 Branch
defect
Not set
normal

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)

{
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?
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.
(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.
> 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?.
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.
Attached patch avoid the timeouts (obsolete) — Splinter Review
I couldn't reproduce the original reasons for the timeouts -- I assume those issues got fixed elsewhere.
Attachment #355771 - Flags: review?(gavin.sharp)
(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.
Attachment #355771 - Attachment is obsolete: true
Attachment #355776 - Flags: review?(enndeakin)
Attachment #355771 - Flags: review?(gavin.sharp)
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)
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
Blocks: 472515
Attachment #355783 - Attachment is obsolete: true
Attachment #355820 - Flags: review?(enndeakin)
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
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.
(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.
(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.
(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)
Attachment #355851 - Flags: review?(enndeakin) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/6e4203c77ce9
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Attachment #355851 - Flags: approval1.9.1?
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?
(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.
Flags: in-testsuite+
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1-
Attachment #355851 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
Depends on: 473686
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
No longer depends on: 473686
Depends on: 475273
No longer depends on: 475273
Depends on: 475273
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
}
(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
Depends on: 477631
Whiteboard: [orange]
No longer depends on: 477631
Depends on: 494373
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.