Closed
Bug 717878
Opened 13 years ago
Closed 11 years ago
Content in input fields can not be scrolled when value exceeds visible width
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox11 affected, firefox12 affected, firefox13 affected, firefox16 affected, firefox17 affected, firefox18 affected, firefox19 affected, firefox20 affected, fennec-)
People
(Reporter: xti, Assigned: jchen)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
enndeakin
:
review+
enndeakin
:
feedback+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Android;Linux armv7l;rv:12.0a1)Gecko/20120112
Firefox/12.0a1 Fennec/12.0a1
Devices: Samsung Galaxy S
OS: Android 2.2
Steps to reproduce:
1. Go to www.google.com
2. Tap on search input field
3. Type a bunch of characters (more than input field length size)
4. Pan the search field content to left/right
Expected result:
The input field content pans left/right.
Actual result:
The input field content cannot be panned.
Note:
This works as expected on Stock Browser.
Updated•13 years ago
|
Priority: -- → P4
Reporter | ||
Updated•13 years ago
|
Comment 2•12 years ago
|
||
Basic, yet important functionality. This should be re-evaluated.
tracking-fennec: --- → ?
status-firefox16:
--- → affected
status-firefox17:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
Priority: P4 → --
Updated•12 years ago
|
Summary: Input fields content cannot be panned → Content in input fields can not be scrolled when value exceeds visible width
Comment 4•12 years ago
|
||
The fix for bug 795045 should take care of this, if done properly.
Depends on: 795045
Updated•12 years ago
|
tracking-fennec: ? → -
Comment 5•12 years ago
|
||
Cristian, can you check if this is an issue on tomorrow (12/20)'s Nightly?
Flags: needinfo?(nicolae.cristian)
Reporter | ||
Comment 6•12 years ago
|
||
(In reply to Aaron Train [:aaronmt] from comment #5)
> Cristian, can you check if this is an issue on tomorrow (12/20)'s Nightly?
Sure. It seems that this issue still occurs on the latest Nightly.
--
Firefox for Android 20.0a1 (2012-12-20)
Device: Galaxy S2
OS: Android 4.0.3
Flags: needinfo?(nicolae.cristian)
Reporter | ||
Updated•12 years ago
|
status-firefox20:
--- → affected
This is partially fixed in the latest Firefox Beta: on the Google page, you can now scroll left, but the scrolling won't move to the right (perhaps due to the "x" clear and button)
Comment 9•11 years ago
|
||
From 927931
> As mentioned on IRC, to implement this the code at [1] needs to be updated to also detect text fields as scrollable. > The code at [2] is an example of how to detect text fields.
>
> [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=0f25523048c3#4625
> [2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=0f25523048c3#1271
Assignee | ||
Comment 10•11 years ago
|
||
I knew there had to be one filed already :)
I added this code per Kats' suggestion, but it didn't work because it appears scrollLeftMax is always 0 for an input element. I'm going to dig deeper into the input element code.
> diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
> index 9cd65d5..5204b05 100644
> --- a/mobile/android/chrome/content/browser.js
> +++ b/mobile/android/chrome/content/browser.js
> @@ -4623,13 +4623,14 @@ var BrowserEventHandler = {
> while (elem) {
> /* Element is scrollable if its scroll-size exceeds its client size, and:
> * - It has overflow 'auto' or 'scroll', or
> - * - It's a textarea or HTML node, or
> + * - It's a text input, textarea, or HTML node, or
> * - It's a select element showing multiple rows
> */
> if (checkElem) {
> if ((elem.scrollTopMax > 0 || elem.scrollLeftMax > 0) &&
> (this._hasScrollableOverflow(elem) ||
> elem.mozMatchesSelector("html, textarea")) ||
> + (elem instanceof HTMLInputElement && elem.mozIsTextField(false)) ||
> (elem instanceof HTMLSelectElement && (elem.size > 1 || elem.multiple))) {
> scrollable = true;
> break;
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Assignee | ||
Comment 11•11 years ago
|
||
And the reason why scrollLeftMax is 0 is because nsTextControlFrame::IsScrollable returns false for single-line text controls in nsTextControlFrame::GetScrollTargetFrame [1], which I don't think makes sense. When I take out that check, everything works perfectly.
nsTextControlFrame::IsScrollable is only used by nsTextControlFrame::GetScrollTargetFrame, and that code goes back to the good ol' CVS days, so I think we can just remove it.
[1] http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsTextControlFrame.h?rev=1406ff031c19#41
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 818733 [details] [diff] [review]
Return scroll target frame for single-line text controls (v1)
Hi Mats, :tn suggested you review this
Attachment #818733 -
Flags: review?(matspal)
Assignee | ||
Comment 13•11 years ago
|
||
Thanks for the pointers, Kats!
Attachment #818743 -
Flags: review?(bugmail.mozilla)
Updated•11 years ago
|
Attachment #818743 -
Flags: review?(bugmail.mozilla) → review+
Comment 14•11 years ago
|
||
Comment on attachment 818733 [details] [diff] [review]
Return scroll target frame for single-line text controls (v1)
Did this pass all tests on Try?
Why are there no additional tests in the attached patches?
This patch also fixes bug 293186 (which is nice) but I would like
to see reftests that assigns/reads .scrollLeft / .scrollTop on one
<input> that does not overflow, and one that does overflow.
(all combinations thereof)
Please also include a test that "over-scrolls", i.e. assign
.scrollLeft a value that is larger than the width of the overflowing
text (the expected result is that it only scrolls to the end).
Also, you forgot to remove the IsScrollable() declaration.
Attachment #818733 -
Flags: review?(matspal) → review-
Assignee | ||
Comment 15•11 years ago
|
||
> This patch also fixes bug 293186 (which is nice) but I would like
> to see reftests that assigns/reads .scrollLeft / .scrollTop on one
> <input> that does not overflow, and one that does overflow.
> (all combinations thereof)
> Please also include a test that "over-scrolls", i.e. assign
> .scrollLeft a value that is larger than the width of the overflowing
> text (the expected result is that it only scrolls to the end).
Any reason to use a reftest? Here's a mochitest that checks for proper scrollLeft/scrollTop support for no-overflow/overflow cases.
Attachment #819978 -
Flags: review?(matspal)
Assignee | ||
Comment 16•11 years ago
|
||
> Also, you forgot to remove the IsScrollable() declaration.
Good catch! Removed.
Attachment #818733 -
Attachment is obsolete: true
Attachment #819982 -
Flags: review?(matspal)
Assignee | ||
Comment 17•11 years ago
|
||
Hmm the patch breaks dom/tests/mochitest/general/test_offsets.html (https://tbpl.mozilla.org/php/getParsedLog.php?id=29459758&tree=Try&full=1)
More specifically, the test assumes an <input>'s scrollWidth and clientWidth include its padding. However, in our implementation, <input>'s padding is not part of its scroll frame (so scrollWidth does not include padding). And because clientWidth is derived from the scroll frame, the clientWidth does not include padding either.
The test worked before because we did not return a scroll frame for single-line inputs, so I think a fallback path was used which happened to work. The same test would fail right now if it used textareas instead of inputs.
The spec defines the scrolling view as including the padding, so I think we might not be following the spec here...
Assignee | ||
Comment 18•11 years ago
|
||
Ehsan pointed out that my problem is likely caused by bug 157846, and it appears that bug won't be fixed soon (I can probably look at it if I have some cycles).
In the meantime, the only thing blocking this bug is test_offsets.html, which uses inputs but AFAICT the test is not really testing the behavior of inputs, so I wonder if we can use divs instead of inputs in that test. It only works around bug 157846, but I think this is reasonable considering input panning in Fennec depends on this bug.
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 821209 [details] [diff] [review]
Use div instead of input in test_offsets.html (v1)
Neil, do you remember why test_offsets.html used inputs?
Attachment #821209 -
Flags: feedback?(enndeakin)
Comment 20•11 years ago
|
||
Comment on attachment 819978 [details] [diff] [review]
Add test for scrolling single-line inputs (v1)
(In reply to Jim Chen [:jchen :nchen] from comment #15)
> Any reason to use a reftest? Here's a mochitest that checks for proper
> scrollLeft/scrollTop support for no-overflow/overflow cases.
Ideally we would have both, but since we're now using a generic code
path a mochitest seems sufficient. (A reftest has a higher degree of
confidence that the scrolling actually took place and is rendered
correctly though. We likely have that covered for other frame types
though.)
Attachment #819978 -
Flags: review?(matspal) → review+
Comment 21•11 years ago
|
||
Comment on attachment 819982 [details] [diff] [review]
Return scroll target frame for single-line text controls (v2)
Thanks. r=mats
Attachment #819982 -
Flags: review?(matspal) → review+
Updated•11 years ago
|
Attachment #821209 -
Flags: feedback?(enndeakin) → feedback+
Updated•11 years ago
|
Attachment #821209 -
Flags: review+
Assignee | ||
Comment 22•11 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=99b83d0abebe
https://hg.mozilla.org/integration/mozilla-inbound/rev/f34f8e6cd607
https://hg.mozilla.org/integration/mozilla-inbound/rev/381f84dc910e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5291077dfd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e753ebb35e64
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f34f8e6cd607
https://hg.mozilla.org/mozilla-central/rev/381f84dc910e
https://hg.mozilla.org/mozilla-central/rev/c5291077dfd2
https://hg.mozilla.org/mozilla-central/rev/e753ebb35e64
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•