Closed Bug 536421 Opened 10 years ago Closed 9 years ago

Reframing a text control inside onfocus loses the caret

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: bzbarsky, Assigned: mats)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Attached file Testcase
Testcase attached.  See also bug 536172.
Blocks: 536172
Duplicate of this bug: 585463
Blocks: 553097
OS: Mac OS X → All
Hardware: x86 → All
Requesting blocking-2.0 flag because it blocks bug 553097 which is a 2.0 blocker.
blocking2.0: --- → ?
Mats, you can probably take this
Assignee: nobody → matspal
blocking2.0: ? → final+
Blocks: 597331
Duplicate of this bug: 598842
Duplicate of this bug: 601159
Once we have a patch for this bug, we should test to make sure it's solving the duplicate bugs as well.
Attached patch mochitest.diff (obsolete) — Splinter Review
Attached patch Patch rev. 1Splinter Review
This fixes it for me locally.  I've pushed it to Try - I'll ask for review
if it passes unit tests.
Attached patch reftest.diff (obsolete) — Splinter Review
The test doesn't need privileges so a regular reftest will do.
Attachment #480826 - Attachment is obsolete: true
Attachment #480830 - Flags: review?(ehsan)
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?
Attachment #480830 - Flags: review?(ehsan) → review+
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!
> >+      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.
(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!
> 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.
Blocks: 602130
Mats, does that patch fix bug 602130?
(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?  :-)
No longer blocks: 602130
Attached patch reftest2.diffSplinter Review
Removed the timer.  Tests with and without theme.
Attachment #480981 - Attachment is obsolete: true
> 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.
Blocks: 602130
Comment on attachment 481139 [details] [diff] [review]
reftest2.diff

r=me with Mounir's comment addressed.
Attachment #481139 - Flags: review+
(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.
Whiteboard: [needs new patch]
Let's deal with the caret position in bug 602130.

http://hg.mozilla.org/mozilla-central/rev/e841ade1b0c5
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
OK.
Whiteboard: [needs new patch]
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.