Last Comment Bug 787624 - input text has changed position on open native help-autocomplete
: input text has changed position on open native help-autocomplete
Status: VERIFIED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 14 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla18
Assigned To: Adam [:hobophobe]
: Manuela Muntean [Away]
Mentors:
http://jsfiddle.net/JohnJ/LC4kt/
Depends on:
Blocks: 720126
  Show dependency treegraph
 
Reported: 2012-09-01 00:29 PDT by sergserg_86
Modified: 2012-12-12 05:56 PST (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed
+
fixed
+
verified


Attachments
Reduced reporter's testcase. (780 bytes, text/html)
2012-09-01 14:20 PDT, Loic
no flags Details
Make forms scroll for popup only if not visible at all (8.12 KB, patch)
2012-09-04 18:50 PDT, Adam [:hobophobe]
roc: review+
Details | Diff | Splinter Review
Prepare patch for checkin (8.13 KB, patch)
2012-09-04 20:26 PDT, Adam [:hobophobe]
no flags Details | Diff | Splinter Review
Make test HTML (7.79 KB, patch)
2012-09-05 17:05 PDT, Adam [:hobophobe]
no flags Details | Diff | Splinter Review
Test works! (8.57 KB, patch)
2012-09-12 18:46 PDT, Adam [:hobophobe]
roc: review-
Details | Diff | Splinter Review
Patch without test (3.33 KB, patch)
2012-09-12 19:48 PDT, Adam [:hobophobe]
roc: review+
Details | Diff | Splinter Review
Updated test using holding-pattern/do-or-timeout (5.79 KB, patch)
2012-09-12 19:51 PDT, Adam [:hobophobe]
no flags Details | Diff | Splinter Review
Ready for checkin (3.34 KB, patch)
2012-09-12 23:18 PDT, Adam [:hobophobe]
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
ryanvm: checkin+
Details | Diff | Splinter Review
Fixed a few typos in previous test. (5.80 KB, patch)
2012-09-13 17:25 PDT, Adam [:hobophobe]
roc: review+
Details | Diff | Splinter Review
[test] Ready for checkin (5.80 KB, patch)
2012-09-13 17:40 PDT, Adam [:hobophobe]
ryanvm: checkin+
Details | Diff | Splinter Review
[test] Include the test in the android.json excludes (7.25 KB, patch)
2012-09-13 21:30 PDT, Adam [:hobophobe]
roc: review+
Details | Diff | Splinter Review
[test] Ready for checkin (7.26 KB, patch)
2012-09-14 13:03 PDT, Adam [:hobophobe]
ryanvm: checkin+
Details | Diff | Splinter Review

Description sergserg_86 2012-09-01 00:29:41 PDT
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120824154833

Steps to reproduce:

1. put wide input in short div with css {position: absolute}
2. focus on text-element
3. open a native autocomplete-help variants

Example: http://jsfiddle.net/JohnJ/LC4kt/


Actual results:

margin-left of input is 0 now? position of input was changed


Expected results:

margin must don't changed and the input position too..
Comment 1 Loic 2012-09-01 14:20:28 PDT
Created attachment 657602 [details]
Reduced reporter's testcase.
Comment 2 Loic 2012-09-01 14:26:11 PDT
I found a regression range, but hard to find the underlying bug. Someone needs to bisect with local builds.

For the testcase, the user needs to have some entries in his history for the input field (here it's "email").

m-c
good=2012-03-20
bad=2012-03-21
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b972b89518c3&tochange=4bdae514b9be

m-i
good=2012-03-20
bad=2012-03-21
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=46c5015550af&tochange=4c0b0a3c272f

I'd say the suspected bug is:
Adam Dane [:hobophobe] — Bug 720126 - Update scroll API to use ScrollAxis parameters (where and when). r=roc
Comment 3 Boris Zbarsky [:bz] 2012-09-04 08:11:24 PDT
Loic, thanks for narrowing that down!  Would you be willing to set tracking flags as needed on regressions?  That would be very helpful!
Comment 4 Adam [:hobophobe] 2012-09-04 18:50:46 PDT
Created attachment 658333 [details] [diff] [review]
Make forms scroll for popup only if not visible at all

|ScrollAxis| defaults to |IF_NOT_FULLY_VISIBLE|, but form elements need only scroll if not visible at all (as they did before the regression).

The same regression exists for |PresShell::PrepareToUseCaretPosition|, where it scrolls content to ensure the caret is visible for a popup menu.  Fixing that here too.

The latter can be reproduced on the same test case when opening the context menu for the |input| using the keyboard (either a dedicated keyboard key or |shift + f10| (may be different on other platforms)).  Note that it won't happen for context-click with the mouse, as that uses mouse coordinates for positioning the menu.

Includes a test (based on the bug's test case) for the autocomplete, but I haven't found a good way to simulate opening the input's context menu so it would look like it was from keyboard.  If there's a good way to do that, it'll be easy to add another test for that to the test here.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-04 19:59:37 PDT
Comment on attachment 658333 [details] [diff] [review]
Make forms scroll for popup only if not visible at all

Review of attachment 658333 [details] [diff] [review]:
-----------------------------------------------------------------

great, thanks
Comment 6 Adam [:hobophobe] 2012-09-04 20:26:31 PDT
Created attachment 658352 [details] [diff] [review]
Prepare patch for checkin

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=f4a411b02572
Comment 7 Adam [:hobophobe] 2012-09-05 14:06:43 PDT
Cleaned up the test a bit, but mainly trying again because I can't reproduce locally.  Hoping a second batch of results will be useful.

https://tbpl.mozilla.org/?tree=Try&rev=1341ce1dad60
Comment 8 Adam [:hobophobe] 2012-09-05 17:05:29 PDT
Created attachment 658692 [details] [diff] [review]
Make test HTML

No help there; reworked it as a HTML-based test:

https://tbpl.mozilla.org/?tree=Try&rev=a2974bbebb98
Comment 9 Alex Keybl [:akeybl] 2012-09-12 14:26:47 PDT
If we're going to fix this in FF16, we need to land a fix on mozilla-central this week, and on Aurora/Beta early next week. Otherwise this may be wontfix'd for FF16.
Comment 10 Adam [:hobophobe] 2012-09-12 18:46:49 PDT
Created attachment 660652 [details] [diff] [review]
Test works!

Sorry for the delay; been beating my head against try failures of the foolish test for too long.  Finally got one that works (https://tbpl.mozilla.org/?tree=Try&rev=56ea0615e70a).

Full try push (in progress):
https://tbpl.mozilla.org/?tree=Try&rev=441e42507c8d
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 19:00:50 PDT
Comment on attachment 660652 [details] [diff] [review]
Test works!

Review of attachment 660652 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/satchel/test/test_bug_787624.html
@@ +147,5 @@
> +        SimpleTest.finish();
> +        return;
> +  }
> +
> +  setTimeout(runTest, 50, testNum + 1);

This is going to suck. No fixed timeout will ever be enough when a machine is going really slowly.

The easiest thing to do here might be use a setTimeout(0) but at each step, check whether the conditions are correct and if not, stay at the same step and try again. That means that any test failure will turn into a test timeout, but that's just fine.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 19:40:54 PDT
In the interests of time I suggest you land the patch without this test ASAP, and then land the test in a separate patch.
Comment 13 Adam [:hobophobe] 2012-09-12 19:48:24 PDT
Created attachment 660668 [details] [diff] [review]
Patch without test

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d7ec60a56f7a
Comment 14 Adam [:hobophobe] 2012-09-12 19:51:44 PDT
Created attachment 660671 [details] [diff] [review]
Updated test using holding-pattern/do-or-timeout

Test now tries again until it can pass or times out.

> > +  setTimeout(runTest, 50, testNum + 1);
> 
> This is going to suck. No fixed timeout will ever be enough when a machine
> is going really slowly.

I would have thought so too, but the test was almost a straight copy of the test from bug 511615, which didn't appear to have any intermittent failures.  Given the problems I've had with the test here, I was reluctant to deviate far from the known-working test.

I'll throw this on try after the main patch has finished.
Comment 15 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-12 19:59:14 PDT
Thanks!
Comment 16 Adam [:hobophobe] 2012-09-12 23:18:51 PDT
Created attachment 660719 [details] [diff] [review]
Ready for checkin

Green try is: 
https://tbpl.mozilla.org/?tree=Try&rev=d7ec60a56f7a
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-09-13 15:52:13 PDT
Comment on attachment 660719 [details] [diff] [review]
Ready for checkin

https://hg.mozilla.org/integration/mozilla-inbound/rev/d33dc9c61144

Leaving open for tests.
Comment 18 Adam [:hobophobe] 2012-09-13 17:25:02 PDT
Created attachment 661058 [details] [diff] [review]
Fixed a few typos in previous test.

Green try for test:
https://tbpl.mozilla.org/?tree=Try&rev=f576ac5d551a
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-13 17:30:22 PDT
Comment on attachment 660719 [details] [diff] [review]
Ready for checkin

Review of attachment 660719 [details] [diff] [review]:
-----------------------------------------------------------------

Very safe, just reverting to some older behavior by flipping some parameters.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-13 17:30:57 PDT
Comment on attachment 661058 [details] [diff] [review]
Fixed a few typos in previous test.

Review of attachment 661058 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a ton!
Comment 21 Adam [:hobophobe] 2012-09-13 17:40:19 PDT
Created attachment 661067 [details] [diff] [review]
[test] Ready for checkin

Green try for test: 
https://tbpl.mozilla.org/?tree=Try&rev=f576ac5d551a
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-09-13 18:55:55 PDT
https://hg.mozilla.org/mozilla-central/rev/d33dc9c61144
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-09-13 19:00:10 PDT
Comment on attachment 661067 [details] [diff] [review]
[test] Ready for checkin

https://hg.mozilla.org/integration/mozilla-inbound/rev/486dcdbdfa23
Comment 25 Adam [:hobophobe] 2012-09-13 21:30:00 PDT
Created attachment 661126 [details] [diff] [review]
[test] Include the test in the android.json excludes

Whoops!  Thanks philor!

The test wasn't meant to run on Android (as none of the other autocomplete tests do); so now it won't.

(/me adds android.json to the list of things he learned today.)
Comment 26 Adam [:hobophobe] 2012-09-14 13:03:53 PDT
Created attachment 661333 [details] [diff] [review]
[test] Ready for checkin

Shouldn't need a new try push. 

Last green try for non-Android test was:
https://tbpl.mozilla.org/?tree=Try&rev=f576ac5d551a

Backout was failure on Android, and only change is disabling the test on Android.
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-09-14 13:11:17 PDT
Comment on attachment 661333 [details] [diff] [review]
[test] Ready for checkin

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9c93b6dbc2
Comment 28 Ryan VanderMeulen [:RyanVM] 2012-09-14 18:17:10 PDT
https://hg.mozilla.org/mozilla-central/rev/6e9c93b6dbc2
Comment 30 Manuela Muntean [Away] 2012-12-12 05:56:01 PST
Verified fixed on Firefox 18 beta 3.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20121205060959

Note You need to log in before you can comment on or make changes to this bug.