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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: sgautherie, Assigned: dao)

Tracking

({fixed1.9.1, intermittent-failure})

1.9.1 Branch
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

11 years ago
{
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.
Assignee

Comment 2

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

11 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

11 years ago
Posted 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)
Assignee

Comment 6

11 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

11 years ago
Attachment #355771 - Attachment is obsolete: true
Attachment #355776 - Flags: review?(enndeakin)
Attachment #355771 - Flags: review?(gavin.sharp)
Assignee

Comment 8

11 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

11 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

Updated

11 years ago
Blocks: 472515
Assignee

Comment 10

11 years ago
Attachment #355783 - Attachment is obsolete: true
Attachment #355820 - Flags: review?(enndeakin)
Assignee

Updated

11 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

11 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

11 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

11 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

11 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

11 years ago
Attachment #355851 - Flags: review?(enndeakin) → review+
Assignee

Updated

11 years ago
Keywords: checkin-needed
Assignee

Comment 15

11 years ago
http://hg.mozilla.org/mozilla-central/rev/6e4203c77ce9
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Assignee

Updated

11 years ago
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?
Assignee

Comment 17

11 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

11 years ago
Flags: in-testsuite+

Updated

11 years ago
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1-

Updated

11 years ago
Attachment #355851 - Flags: approval1.9.1? → approval1.9.1+
Assignee

Updated

11 years ago
Keywords: checkin-needed
Reporter

Updated

11 years ago
Depends on: 473686

Comment 19

11 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
Assignee

Updated

11 years ago
No longer depends on: 473686
Depends on: 475273
Assignee

Updated

11 years ago
No longer depends on: 475273
Assignee

Updated

11 years ago
Depends on: 475273
Reporter

Comment 20

11 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

11 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
Reporter

Updated

11 years ago
Depends on: 477631
Whiteboard: [orange]
Assignee

Updated

10 years ago
No longer depends on: 477631

Updated

10 years ago
Depends on: 494373
Comment hidden (Legacy TBPL/Treeherder Robot)
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.