536421 - Reframing a text control inside onfocus loses the caret

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Testcase attached.  See also bug 536172.

Comment 2

[Mounir Lamouri (:mounir)]

Requesting blocking-2.0 flag because it blocks bug 553097 which is a 2.0 blocker.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mats, you can probably take this

Comment 6

[(no longer active)]

Once we have a patch for this bug, we should test to make sure it's solving the duplicate bugs as well.

Comment 7

[Mats Palmgren (inactive)]

Attachment: mochitest.diff

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This fixes it for me locally.  I've pushed it to Try - I'll ask for review
if it passes unit tests.

Comment 9

[Mats Palmgren (inactive)]

Attachment: reftest.diff

The test doesn't need privileges so a regular reftest will do.

Comment 10

[(no longer active)]

Comment on attachment 480830 [details] [diff] [review]
Patch rev. 1

Makes sense.  Did you also test this patch with the duplicate bug reports in this bug?

Comment 11

[(no longer active)]

Comment on attachment 480981 [details] [diff] [review]
reftest.diff

>+      timer = setTimeout(finishTest, 5000); // if onfocus is never called

Why is this necessary?  If onfocus is never called, the reftest will just time out, right?

>+input { border:1px solid blue; }

Nit: why is this necessary?

>diff --git a/layout/reftests/forms/reftest.list b/layout/reftests/forms/reftest.list

Could you please add this reftest to layout/reftests/editor?  Thanks!

Comment 12

[Mats Palmgren (inactive)]

> >+      timer = setTimeout(finishTest, 5000); // if onfocus is never called
> 
> Why is this necessary?

Unit test tinderboxes are known to have intermittent focus issues.

> If onfocus is never called, the reftest will just time out, right?

I honestly don't know what happens for a class=reftest-wait test if the
class isn't removed.  Even if the harness has a timeout, it will
still cause an orange?  The timeout doesn't hurt so I'll leave it in
unless you see a problem with that.

> >+input { border:1px solid blue; }
> 
> Nit: why is this necessary?

To disable native themes affecting the test in any way.

> Could you please add this reftest to layout/reftests/editor?  Thanks!

Will do.

Comment 13

[(no longer active)]

(In reply to comment #12)
> > >+      timer = setTimeout(finishTest, 5000); // if onfocus is never called
> > 
> > Why is this necessary?
> 
> Unit test tinderboxes are known to have intermittent focus issues.

Sure (although I've never seen any such problems personally, but it's worth protecting against it).  See below.

> > If onfocus is never called, the reftest will just time out, right?
> 
> I honestly don't know what happens for a class=reftest-wait test if the
> class isn't removed.  Even if the harness has a timeout, it will
> still cause an orange?  The timeout doesn't hurt so I'll leave it in
> unless you see a problem with that.

What happens if the test doesn't remove reftest-wait in a timely fashion is that it will time out and cause an orange.  With this timeout in place, it won't time out, and if we're really unlucky, the test will pass even if the rendering is not the same.  Therefore, I'd really rather we don't use a timeout this way, to make sure that the test will fail if the input is never focused.

> > >+input { border:1px solid blue; }
> > 
> > Nit: why is this necessary?
> 
> To disable native themes affecting the test in any way.

But it shouldn't.  This is the sort of thing which might mask a regression in the future, so I think you should remove it as well.

> > Could you please add this reftest to layout/reftests/editor?  Thanks!
> 
> Will do.

Awesome, thanks!

Comment 14

[Mats Palmgren (inactive)]

> Therefore, I'd really rather we don't use a timeout
> this way, to make sure that the test will fail if the input is never focused.

Fortunately the Tbox focus issues are rare so the test not running
would be rare.  Personally, I think those intermittent oranges are
just annoying and if there's a real regression the test would flag
that soon enough.

Also, people don't like reftests to hang when the window doesn't
have focus.  I had to add a timeout to fix a test in bug 564413.
Are you sure you want me to remove the timeout?

> > > >+input { border:1px solid blue; }
> But it shouldn't.  This is the sort of thing which might mask a regression in
> the future...

But the opposite is equally true - the theme might mask a regression.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Mats, does that patch fix bug 602130?

Comment 16

[(no longer active)]

(In reply to comment #14)
> > Therefore, I'd really rather we don't use a timeout
> > this way, to make sure that the test will fail if the input is never focused.
> 
> Fortunately the Tbox focus issues are rare so the test not running
> would be rare.  Personally, I think those intermittent oranges are
> just annoying and if there's a real regression the test would flag
> that soon enough.

But if there's a real failure, this will be perma-orange.  And something causes intermittent focus problems, many of the tests in layout/reftests/editor will go orange.  In fact, that's half of the reason I asked you to move the test there!  :-)

> Also, people don't like reftests to hang when the window doesn't
> have focus.  I had to add a timeout to fix a test in bug 564413.
> Are you sure you want me to remove the timeout?

Yes!  That's the other half of the reason that I asked you to move the test there, as most of the tests in that directory need the browser to be in foreground and focused.  I don't know what the ideal situation with regards to bug 564413 is, but we know for a fact that the reftests in that directory are going to fail if the browser is not in the foreground.

<rant>
I was one of the people that hated the reftest framework to break down if not in foreground (the crashtest framework doesn't to the best of my knowledge).  But there were some placeholder tests which simply couldn't operate correctly without focus, so I finally gave up!  And given the fact that every other test framework that we have except for crashtest and xpcshell-tests also requires focus, I think I'm in a rather strong position to defend this, even though I'm acting as devil's advocate here!
</rant>

> > > > >+input { border:1px solid blue; }
> > But it shouldn't.  This is the sort of thing which might mask a regression in
> > the future...
> 
> But the opposite is equally true - the theme might mask a regression.

Sure, but we *really* want to catch the regressions which are _not_ theme-related.  Now that you mentioned it though, how about adding both series of tests?  :-)

Comment 17

[Mats Palmgren (inactive)]

Attachment: reftest2.diff

Removed the timer.  Tests with and without theme.

Comment 18

[Mounir Lamouri (:mounir)]

> Removed the timer.  Tests with and without theme.

You should probably use "-moz-appearance: none;" instead of assuming a border rule is removing the native appearance.

Comment 19

[(no longer active)]

Comment on attachment 481139 [details] [diff] [review]
reftest2.diff

r=me with Mounir's comment addressed.

Comment 20

[(no longer active)]

(In reply to comment #15)
> Mats, does that patch fix bug 602130?

FWIW, the fix to this bug should also fix bug 602130.  Mats said that it doesn't, however, which means that this fix is not complete.

Comment 21

[Mats Palmgren (inactive)]

Let's deal with the caret position in bug 602130.

http://hg.mozilla.org/mozilla-central/rev/e841ade1b0c5

Comment 22

[(no longer active)]

OK.