717878 - Content in input fields can not be scrolled when value exceeds visible width

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

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.

Comment 2

[Aaron Train [:aaronmt]]

Basic, yet important functionality. This should be re-evaluated.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

The fix for bug 795045 should take care of this, if done properly.

Comment 5

[Aaron Train [:aaronmt]]

Cristian, can you check if this is an issue on tomorrow (12/20)'s Nightly?

Comment 6

[Cristian Nicolae (:xti)]

(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

Comment 7

[r.lupton]

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

[Kevin Brosnan [Ex-Mozilla]]

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

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

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;

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Return scroll target frame for single-line text controls (v1)

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

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 818733 [details] [diff] [review]
Return scroll target frame for single-line text controls (v1)

Hi Mats, :tn suggested you review this

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Enable scrolling single-line text inputs (v1)

Thanks for the pointers, Kats!

Comment 14

[Mats Palmgren (inactive)]

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.

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add test for scrolling single-line inputs (v1)

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

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Return scroll target frame for single-line text controls (v2)

> Also, you forgot to remove the IsScrollable() declaration.

Good catch! Removed.

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

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

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use div instead of input in test_offsets.html (v1)

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.

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

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?

Comment 20

[Mats Palmgren (inactive)]

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.)

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 819982 [details] [diff] [review]
Return scroll target frame for single-line text controls (v2)

Thanks.  r=mats

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

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

[Phil Ringnalda (:philor)]

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