Closed
Bug 787624
Opened 12 years ago
Closed 12 years ago
input text has changed position on open native help-autocomplete
Categories
(Core :: Layout, defect)
Tracking
()
VERIFIED
FIXED
mozilla18
People
(Reporter: sergserg_86, Assigned: unusualtears)
References
()
Details
(Keywords: regression, testcase)
Attachments
(3 files, 9 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
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..
Reporter | ||
Updated•12 years ago
|
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
Comment 3•12 years ago
|
||
Loic, thanks for narrowing that down! Would you be willing to set tracking flags as needed on regressions? That would be very helpful!
Blocks: 720126
status-firefox15:
--- → affected
tracking-firefox16:
--- → ?
tracking-firefox17:
--- → ?
tracking-firefox18:
--- → ?
Updated•12 years ago
|
|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 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+
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
No help there; reworked it as a HTML-based test:
https://tbpl.mozilla.org/?tree=Try&rev=a2974bbebb98
Attachment #658352 -
Attachment is obsolete: true
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
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)
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.
Assignee | ||
Comment 13•12 years ago
|
||
Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d7ec60a56f7a
Attachment #660652 -
Attachment is obsolete: true
Attachment #660668 -
Flags: review?(roc)
Assignee | ||
Comment 14•12 years ago
|
||
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.
Attachment #660668 -
Flags: review?(roc) → review+
Thanks!
Assignee | ||
Comment 16•12 years ago
|
||
Green try is:
https://tbpl.mozilla.org/?tree=Try&rev=d7ec60a56f7a
Attachment #660668 -
Attachment is obsolete: true
Attachment #660719 -
Flags: checkin?
Keywords: checkin-needed
Updated•12 years ago
|
Comment 17•12 years ago
|
||
Comment on attachment 660719 [details] [diff] [review]
Ready for checkin
https://hg.mozilla.org/integration/mozilla-inbound/rev/d33dc9c61144
Leaving open for tests.
Attachment #660719 -
Flags: checkin? → checkin+
Assignee | ||
Comment 18•12 years ago
|
||
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+
Assignee | ||
Comment 21•12 years ago
|
||
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
Comment 22•12 years ago
|
||
Keywords: checkin-needed
Comment 23•12 years ago
|
||
Comment on attachment 661067 [details] [diff] [review]
[test] Ready for checkin
https://hg.mozilla.org/integration/mozilla-inbound/rev/486dcdbdfa23
Attachment #661067 -
Flags: checkin? → checkin+
Updated•12 years ago
|
Flags: in-testsuite? → in-testsuite+
Updated•12 years ago
|
Whiteboard: [leave open]
Comment 24•12 years ago
|
||
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
Assignee | ||
Comment 25•12 years ago
|
||
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)
Attachment #661126 -
Flags: review?(roc) → review+
Assignee | ||
Comment 26•12 years ago
|
||
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
Comment 27•12 years ago
|
||
Comment on attachment 661333 [details] [diff] [review]
[test] Ready for checkin
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9c93b6dbc2
Attachment #661333 -
Flags: checkin? → checkin+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 28•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•12 years ago
|
Attachment #660719 -
Flags: approval-mozilla-beta?
Attachment #660719 -
Flags: approval-mozilla-beta+
Attachment #660719 -
Flags: approval-mozilla-aurora?
Attachment #660719 -
Flags: approval-mozilla-aurora+
Comment 30•12 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•