Closed Bug 787624 Opened 9 years ago Closed 9 years ago

input text has changed position on open native help-autocomplete

Categories

(Core :: Layout, defect)

14 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox15 --- affected
firefox16 + fixed
firefox17 + fixed
firefox18 + verified

People

(Reporter: sergserg_86, Assigned: hobophobe)

References

()

Details

(Keywords: regression, testcase)

Attachments

(3 files, 9 obsolete files)

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..
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
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Keywords: regression, testcase
Product: Firefox → Core
Loic, thanks for narrowing that down!  Would you be willing to set tracking flags as needed on regressions?  That would be very helpful!
|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.
Assignee: nobody → unusualtears
Status: NEW → ASSIGNED
Attachment #658333 - Flags: review?(roc)
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
Attachment #658333 - Flags: review?(roc) → review+
Attached patch Prepare patch for checkin (obsolete) — Splinter Review
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=f4a411b02572
Attachment #658333 - Attachment is obsolete: true
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
Attached patch Make test HTML (obsolete) — Splinter Review
No help there; reworked it as a HTML-based test:

https://tbpl.mozilla.org/?tree=Try&rev=a2974bbebb98
Attachment #658352 - Attachment is obsolete: true
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.
Attached patch Test works! (obsolete) — Splinter Review
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
Attachment #658692 - Attachment is obsolete: true
Attachment #660652 - Flags: review?(roc)
Version: 15 Branch → 14 Branch
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.
Attachment #660652 - Flags: review?(roc) → review-
In the interests of time I suggest you land the patch without this test ASAP, and then land the test in a separate patch.
Attached patch Patch without test (obsolete) — Splinter Review
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d7ec60a56f7a
Attachment #660652 - Attachment is obsolete: true
Attachment #660668 - Flags: review?(roc)
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.
Green try is: 
https://tbpl.mozilla.org/?tree=Try&rev=d7ec60a56f7a
Attachment #660668 - Attachment is obsolete: true
Attachment #660719 - Flags: checkin?
Keywords: checkin-needed
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [leave open]
Green try for test:
https://tbpl.mozilla.org/?tree=Try&rev=f576ac5d551a
Attachment #660671 - Attachment is obsolete: true
Attachment #661058 - Flags: review?(roc)
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.
Attachment #660719 - Flags: approval-mozilla-beta?
Attachment #660719 - Flags: approval-mozilla-aurora?
Comment on attachment 661058 [details] [diff] [review]
Fixed a few typos in previous test.

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

Thanks a ton!
Attachment #661058 - Flags: review?(roc) → review+
Attached patch [test] Ready for checkin (obsolete) — Splinter Review
Green try for test: 
https://tbpl.mozilla.org/?tree=Try&rev=f576ac5d551a
Attachment #661058 - Attachment is obsolete: true
Attachment #661067 - Flags: checkin?
Keywords: checkin-needed
Flags: in-testsuite? → in-testsuite+
Whiteboard: [leave open]
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.)
Attachment #661067 - Attachment is obsolete: true
Attachment #661126 - Flags: review?(roc)
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.
Attachment #661126 - Attachment is obsolete: true
Attachment #661333 - Flags: checkin?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6e9c93b6dbc2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Attachment #660719 - Flags: approval-mozilla-beta?
Attachment #660719 - Flags: approval-mozilla-beta+
Attachment #660719 - Flags: approval-mozilla-aurora?
Attachment #660719 - Flags: approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
QA Contact: manuela.muntean
You need to log in before you can comment on or make changes to this bug.