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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: sgautherie, Assigned: dao)

Tracking

({fixed1.9.1, intermittent-failure})

1.9.1 Branch
mozilla1.9.2a1
fixed1.9.1, intermittent-failure
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

10 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

10 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.
Created attachment 355755 [details] [diff] [review]
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?.
(Assignee)

Comment 4

10 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

10 years ago
Created attachment 355771 [details] [diff] [review]
avoid the timeouts

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

10 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

10 years ago
Created attachment 355776 [details] [diff] [review]
avoid the timeouts (with updated tests)
Attachment #355771 - Attachment is obsolete: true
Attachment #355776 - Flags: review?(enndeakin)
Attachment #355771 - Flags: review?(gavin.sharp)
(Assignee)

Comment 8

10 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

10 years ago
Created attachment 355783 [details] [diff] [review]
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.
Attachment #355755 - Attachment is obsolete: true
Attachment #355776 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Blocks: 472515
(Assignee)

Comment 10

10 years ago
Created attachment 355820 [details] [diff] [review]
 avoid the timeouts, with fix for bug 472515
Attachment #355783 - Attachment is obsolete: true
Attachment #355820 - Flags: review?(enndeakin)
(Assignee)

Updated

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

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

10 years ago
Created attachment 355851 [details] [diff] [review]
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.
Attachment #355820 - Attachment is obsolete: true
Attachment #355851 - Flags: review?(enndeakin)
Attachment #355820 - Flags: review?(enndeakin)
Attachment #355851 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Comment 15

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

Updated

10 years ago
Attachment #355851 - Flags: approval1.9.1?

Comment 16

10 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

10 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

10 years ago
Flags: in-testsuite+

Updated

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

Updated

10 years ago
Attachment #355851 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Comment 18

10 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e37ddc9c85d3
Keywords: checkin-needed → fixed1.9.1
(Reporter)

Updated

10 years ago
Depends on: 473686

Comment 19

10 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

10 years ago
No longer depends on: 473686
(Assignee)

Updated

10 years ago
No longer depends on: 475273
(Assignee)

Updated

10 years ago
Depends on: 475273
(Reporter)

Comment 20

10 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

10 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

10 years ago
Depends on: 477631
Whiteboard: [orange]
(Assignee)

Updated

10 years ago
No longer depends on: 477631

Updated

9 years ago
Depends on: 494373
Depends on: 507040
Comment hidden (Treeherder Robot)
Keywords: intermittent-failure
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.