787624 - input text has changed position on open native help-autocomplete

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[sergserg_86]

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]

Attachment: Reduced reporter's testcase.

Comment 2

[Loic]

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 [:bzbarsky]]

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]

Attachment: 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) (email my personal email if necessary)]

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]

Attachment: Prepare patch for checkin

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=f4a411b02572

Comment 7

[Adam]

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]

Attachment: 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]]

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]

Attachment: 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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

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]

Attachment: Patch without test

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d7ec60a56f7a

Comment 14

[Adam]

Attachment: 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) (email my personal email if necessary)]

Thanks!

Comment 16

[Adam]

Attachment: Ready for checkin

Green try is: 
https://tbpl.mozilla.org/?tree=Try&rev=d7ec60a56f7a

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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]

Attachment: 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) (email my personal email if necessary)]

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) (email my personal email if necessary)]

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]

Attachment: [test] Ready for checkin

Green try for test: 
https://tbpl.mozilla.org/?tree=Try&rev=f576ac5d551a

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d33dc9c61144

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 661067 [details] [diff] [review]
[test] Ready for checkin

https://hg.mozilla.org/integration/mozilla-inbound/rev/486dcdbdfa23

Comment 24

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f8f408f877 for Android failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=15208102&tree=Mozilla-Inbound

Comment 25

[Adam]

Attachment: [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]

Attachment: [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]]

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

https://hg.mozilla.org/mozilla-central/rev/6e9c93b6dbc2

Comment 29

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/a06f7b317f78
https://hg.mozilla.org/releases/mozilla-aurora/rev/54983f4ff707

https://hg.mozilla.org/releases/mozilla-beta/rev/346fe62c5b65
https://hg.mozilla.org/releases/mozilla-beta/rev/444efd90a31a

Comment 30

[Manuela Muntean [Away]]

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