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

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Serge Gautherie (:sgautherie)]

{
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
}

Comment 1

[Graeme McCutcheon [:graememcc]]

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.

Comment 2

[Dão Gottwald [:dao]]

(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

[Graeme McCutcheon [:graememcc]]

Attachment: Check first textbox after timeout

> 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 4

[Dão Gottwald [:dao]]

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.

Comment 5

[Dão Gottwald [:dao]]

Attachment: avoid the timeouts

I couldn't reproduce the original reasons for the timeouts -- I assume those issues got fixed elsewhere.

Comment 6

[Dão Gottwald [:dao]]

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

Comment 7

[Dão Gottwald [:dao]]

Attachment: avoid the timeouts (with updated tests)

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 355776 [details] [diff] [review]
avoid the timeouts (with updated tests)

I just discovered bug 472515, which is critical for this patch...

Comment 9

[Dão Gottwald [:dao]]

Attachment: avoid the timeouts, with possible fix for bug 472515

I imagine this could fix bug 472515, but I still need to run the tests and actually test it myself.

Comment 10

[Dão Gottwald [:dao]]

Attachment: avoid the timeouts, with fix for bug 472515

Comment 11

[Neil Deakin]

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.

Comment 12

[Dão Gottwald [:dao]]

(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

[Neil Deakin]

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

Comment 14

[Dão Gottwald [:dao]]

Attachment: set the emptytextdelay attribute on the inputField

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

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6e4203c77ce9

Comment 16

[[e-mail]]

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?

Comment 17

[Dão Gottwald [:dao]]

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

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e37ddc9c85d3

Comment 19

[Robert Kaiser]

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

Comment 20

[Serge Gautherie (:sgautherie)]

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
}

Comment 21

[Serge Gautherie (:sgautherie)]

(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

Comment 22

[Legacy TBPL/Treeherder Robot]

jorendorff%mozilla.com
http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1275635415.1275636132.31248.gz
Linux tracemonkey opt test mochitest-other on 2010/06/04 00:10:15

s: moz2-linux-slave29
7444 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_bug418874.xul | undo correctly enabled when placeholder was not changed through property - got false, expected true
7445 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_bug418874.xul | undo correctly enabled when placeholder explicitly changed through property - got false, expected true
PROCESS-CRASH | automation.py | application crashed (minidump found)
Thread 0 (crashed)
PROCESS-CRASH | automation.py | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/places/tests/browser/browser_bookmarksProperties.js | Timed out