Bug 47036 - Wheel scrolls and autoscrolls should not scroll overflow:hidden
Summary: Wheel scrolls and autoscrolls should not scroll overflow:hidden
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-01 18:00 PDT by Peter Kasting
Modified: 2013-12-20 01:36 PST (History)
10 users (show)

See Also:


Attachments
WIP patch (5.89 KB, patch)
2010-10-01 18:23 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
WIP patch w/test (7.16 KB, patch)
2010-10-01 18:29 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v1 (15.61 KB, patch)
2010-10-05 18:15 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
patch v1.1 (15.61 KB, patch)
2010-10-05 18:22 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Reduced patch for 517 branch (11.37 KB, patch)
2010-10-06 14:16 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
include WebCore.exp.in changes so this builds on mac (65.57 KB, patch)
2010-10-06 18:05 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Followon patch v1 (8.77 KB, patch)
2010-10-29 14:01 PDT, Peter Kasting
levin: review-
Details | Formatted Diff | Diff
Followon patch v2 (11.00 KB, patch)
2011-01-28 16:25 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Followon patch v3 (23.42 KB, patch)
2011-05-23 16:56 PDT, Peter Kasting
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from cr-jail-7 (161.94 KB, application/zip)
2011-05-23 23:45 PDT, WebKit Commit Bot
no flags Details
Followon patch v4 (24.89 KB, patch)
2011-05-25 14:36 PDT, Peter Kasting
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.22 MB, application/zip)
2011-05-25 22:27 PDT, WebKit Review Bot
no flags Details
Followon patch v5 (28.17 KB, patch)
2011-05-26 14:21 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Followon patch v6 (28.17 KB, patch)
2011-05-26 14:26 PDT, Peter Kasting
tonikitoo: review-
tonikitoo: commit-queue-
Details | Formatted Diff | Diff
Followon patch v7 (15.77 KB, patch)
2011-06-02 14:59 PDT, Peter Kasting
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Followon patch v8 (15.24 KB, patch)
2011-06-02 17:26 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff
Followon patch v9 (15.24 KB, patch)
2011-06-03 11:06 PDT, Peter Kasting
eric: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2010-10-01 18:00:18 PDT
If a ScrollView has more content than is visible in some dimension, but there is no scrollbar for that dimension, then user events such as down arrow/scroll wheel/middle-mouse-autoscroll ("pan-scroll") should not scroll it into view.  The arrow keys already don't.  This is about fixing the wheel and autoscroll code.

There is one bit of weirdness here.  Consider a scrollable page containing an iframe with overflow:hidden content.  EventHandler::handleWheelEvent() will first try to dispatch a wheel event to the inner frame.  (My patch will fix the ScrollView for this frame to correctly report that it's not scrollable.)  When that doesn't accept the event, it will then dispatch to the containing frame (the outer document).  This is how wheel scrolls scroll the whole page once a hovered element reaches the end of its scroll area.

Now consider the same case, but with middle-mouse-autoscroll.  We dispatch a mousedown event to the inner frame, which gets a chance to prevent it.  If it doesn't, the code in Node::defaultEventHandler() checks if the inner frame is scrollable.  When it's not, we terminate without doing anything.  It seems to me that it would be nice to scroll the outer page in this case.  But this raises a lot of questions.  First, should I dispatch a mousedown event to the containing frame too, like wheel events do?  Second, should I duplicate the wheel event's behavior of scrolling the outer page for scrollable inner frames that are at the end of their scroll area, or only do this for cases where the inner frames aren't scrollable at all?  Both of these seem like they could be weird.

hyatt, if you have any thoughts, I'd appreciate them...
Comment 1 Peter Kasting 2010-10-01 18:23:12 PDT
Created attachment 69555 [details]
WIP patch

Here's a patch which basically fixes things.  The LayoutTest isn't done yet -- it works, but it's a manual test, I haven't yet scripted it or added expected results.  I'm not sure what the right behavior for middle-mouse autoscroll is and I haven't added a test for that.

It'd be nice to get feedback on whether my implementation here, especially w.r.t "hasAScrollbar()", is good or crazy.
Comment 2 Peter Kasting 2010-10-01 18:29:48 PDT
Created attachment 69557 [details]
WIP patch w/test

Oops, forgot to svn add the LayoutTest.
Comment 3 Peter Kasting 2010-10-05 18:15:38 PDT
Created attachment 69874 [details]
patch v1
Comment 4 WebKit Review Bot 2010-10-05 18:18:37 PDT
Attachment 69874 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/ScrollView.cpp:696:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/ScrollView.cpp:697:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/ScrollView.cpp:698:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/ScrollView.cpp:702:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Peter Kasting 2010-10-05 18:22:14 PDT
Created attachment 69876 [details]
patch v1.1

Man, none of those was even my fault.
Comment 6 Peter Kasting 2010-10-06 14:16:09 PDT
Created attachment 69988 [details]
Reduced patch for 517 branch

This is a trimmed-down version of the patch for the 517 branch, that doesn't attempt to address pan-scrolling issues.
Comment 7 James Robinson 2010-10-06 16:38:38 PDT
On the reduced patch the test is not right, it shouldn't be a pixel test.  Also there are some spurious svn properties set.  That said, the bug fix is pretty critical so I'm gonna land just that part and leave the bug open to land a proper test and for the rest of the logic changes.
Comment 8 James Robinson 2010-10-06 16:38:48 PDT
Comment on attachment 69988 [details]
Reduced patch for 517 branch

Landing by hand
Comment 9 James Robinson 2010-10-06 16:57:17 PDT
Committed r69257: <http://trac.webkit.org/changeset/69257>
Comment 10 James Robinson 2010-10-06 16:57:43 PDT
Not fixed yet
Comment 11 Peter Kasting 2010-10-06 17:28:24 PDT
Comment on attachment 69988 [details]
Reduced patch for 517 branch

Marking obsolete as this was (partly) landed
Comment 12 Peter Kasting 2010-10-06 17:28:56 PDT
Comment on attachment 69876 [details]
patch v1.1

Obsoleting this patch.  I'll produce another with a better test and without the bits that have already landed.
Comment 13 James Robinson 2010-10-06 18:05:50 PDT
Created attachment 70021 [details]
include WebCore.exp.in changes so this builds on mac
Comment 14 James Robinson 2010-10-06 18:11:19 PDT
Comment on attachment 70021 [details]
include WebCore.exp.in changes so this builds on mac

Sigh, this patch does not belong to this bug
Comment 15 Eric Seidel (no email) 2010-10-14 07:19:54 PDT
Comment on attachment 69988 [details]
Reduced patch for 517 branch

Cleared James Robinson's review+ from obsolete attachment 69988 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 16 Peter Kasting 2010-10-29 14:01:37 PDT
Created attachment 72387 [details]
Followon patch v1

This patches the pan scroll part (the part that didn't already land) and converts the wheel event layout test from a pixel test to a text test.  It also fixes the svn:executable bit being set.
Comment 17 Peter Beverloo 2010-11-03 12:29:20 PDT
You might want to take note of bug 22769, which also is related to scrolling content which has overflow:hidden set. While the gesture is slightly different than the ones you're describing, the effect is similar.
Comment 18 David Levin 2010-12-26 11:35:17 PST
Comment on attachment 72387 [details]
Followon patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=72387&action=review

> WebCore/dom/Node.cpp:2929
> +            // scrolling the entire page.  We could fix this by trying to

Single space after periods in comments (in WebKit).

> WebCore/rendering/RenderLayer.cpp:1275
> +    if (renderer()->hasOverflowClip() && (!renderer()->parent() || renderer()->parent()->style()->lineClamp().isNone())) {

This long clause is repeated in two places. Should it be a method?

Also, this appears like it is unrelated to the fix. It simply changes the form of the code to something that is less clear (to me).

> WebCore/rendering/RenderLayer.cpp:1306
> +bool RenderLayer::hasAScrollbar(bool vertical, bool horizontal)

This would be be better as two functions:


hasVerticalScrollbar
hasHorizontalScrollbar

> WebCore/rendering/RenderLayer.h:234
> +    // directions.  For renderers without an overflow clip (documents), this

Single space after .

> LayoutTests/fast/events/wheelevent-bubble.html:57
>  \ No newline at end of file

Please fix.
Comment 19 Peter Kasting 2011-01-28 16:25:17 PST
Comment on attachment 72387 [details]
Followon patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=72387&action=review

New patch to be uploaded momentarily.

>> WebCore/dom/Node.cpp:2929
>> +            // scrolling the entire page.  We could fix this by trying to
> 
> Single space after periods in comments (in WebKit).

Fixed.

>> WebCore/rendering/RenderLayer.cpp:1275
>> +    if (renderer()->hasOverflowClip() && (!renderer()->parent() || renderer()->parent()->style()->lineClamp().isNone())) {
> 
> This long clause is repeated in two places. Should it be a method?
> 
> Also, this appears like it is unrelated to the fix. It simply changes the form of the code to something that is less clear (to me).

Factored to a separate function, which all other code now calls, and exploded that function a little for readability.

>> WebCore/rendering/RenderLayer.cpp:1306
>> +bool RenderLayer::hasAScrollbar(bool vertical, bool horizontal)
> 
> This would be be better as two functions:
> 
> 
> hasVerticalScrollbar
> hasHorizontalScrollbar

Fixed.

>> WebCore/rendering/RenderLayer.h:234
>> +    // directions.  For renderers without an overflow clip (documents), this
> 
> Single space after .

This comment is now changed.

>> LayoutTests/fast/events/wheelevent-bubble.html:57
>>  \ No newline at end of file
> 
> Please fix.

Fixed.
Comment 20 Peter Kasting 2011-01-28 16:25:57 PST
Created attachment 80522 [details]
Followon patch v2
Comment 21 Adele Peterson 2011-04-26 17:00:14 PDT
Will this break scrolling in text fields, which use overflow:hidden?
Comment 22 Peter Kasting 2011-04-26 17:28:27 PDT
(In reply to comment #21)
> Will this break scrolling in text fields, which use overflow:hidden?

They do?  Testing on this comment box gives me a scrollbar when my text gets too big.  Am I testing the wrong thing?
Comment 23 Adele Peterson 2011-04-26 17:44:08 PDT
That's a textarea.  I was talking about <input>s.

(In reply to comment #22)
> (In reply to comment #21)
> > Will this break scrolling in text fields, which use overflow:hidden?
> 
> They do?  Testing on this comment box gives me a scrollbar when my text gets too big.  Am I testing the wrong thing?
Comment 24 Peter Kasting 2011-04-26 17:51:59 PDT
(In reply to comment #23)
> That's a textarea.  I was talking about <input>s.

So, single-line entries?  Hmm, I didn't check.  My Windows box doesn't have anything with native horizontal scrolling, I'd need to try a trackpad or something.  I didn't even know you could wheel-scroll such things.
Comment 25 James Robinson 2011-04-26 17:58:01 PDT
On my mac I can't currently two-finger scroll horizontally within an <input> that contains a large amount of text, so I don't think this would be a behavior change.
Comment 26 Adele Peterson 2011-04-26 18:04:10 PDT
What about autoscrolling?
Comment 27 Peter Kasting 2011-04-26 18:06:43 PDT
(In reply to comment #26)
> What about autoscrolling?

I kinda feel like if we currently allow autoscrolling in James' case, it's a bug.  Shouldn't we ought to allow wheel and autoscrolls in the same cases?
Comment 28 Adele Peterson 2011-04-26 18:10:07 PDT
hmm maybe I'm not talking about the same thing here - when you say autoscroll, do you mean, pan-scroll?  Because I don't :)
Comment 29 Peter Kasting 2011-04-26 18:13:03 PDT
(In reply to comment #28)
> hmm maybe I'm not talking about the same thing here - when you say autoscroll, do you mean, pan-scroll?  Because I don't :)

Yes.  From comment 0: "...user events such as down arrow/scroll wheel/middle-mouse-autoscroll ("pan-scroll")..."

This will have no effect at all on whether the field scrolls e.g. as you type characters.
Comment 30 Adele Peterson 2011-04-26 19:36:45 PDT
Ok sounds like this isn't an issue then.  Just for future reference though - autoscroll != pan-scroll in WebKit.
Comment 31 Eric Seidel (no email) 2011-05-23 13:30:36 PDT
Comment on attachment 80522 [details]
Followon patch v2

I suspect this patch no longer applies since it's 4 months old now.
Comment 32 Eric Seidel (no email) 2011-05-23 14:18:30 PDT
Comment on attachment 80522 [details]
Followon patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=80522&action=review

This seems reasonable to me now that I actually read it.  I'm sad it was ignored so long.

> Source/WebCore/rendering/RenderLayer.h:242
> +    bool hasVerticalScrollbar();
> +    bool hasHorizontalScrollbar();

Should these be const?
Comment 33 Peter Kasting 2011-05-23 14:20:33 PDT
Comment on attachment 80522 [details]
Followon patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=80522&action=review

>> Source/WebCore/rendering/RenderLayer.h:242
>> +    bool hasHorizontalScrollbar();
> 
> Should these be const?

Yes, they should.  cq- so I can fix that.
Comment 34 Antonio Gomes 2011-05-23 14:25:53 PDT
Comment on attachment 80522 [details]
Followon patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=80522&action=review

> Source/WebCore/rendering/RenderBox.cpp:638
> +    if (!canBeProgramaticallyScrolled(false))

Since you are here, could you also add a comment styled "false /*XXX*/" to make clear for readers of the code what the parameter is?
Comment 35 Peter Kasting 2011-05-23 14:43:40 PDT
Comment on attachment 80522 [details]
Followon patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=80522&action=review

>> Source/WebCore/rendering/RenderBox.cpp:638
>> +    if (!canBeProgramaticallyScrolled(false))
> 
> Since you are here, could you also add a comment styled "false /*XXX*/" to make clear for readers of the code what the parameter is?

Actually I think that arg is dead.
Comment 36 Peter Kasting 2011-05-23 16:56:53 PDT
Created attachment 94518 [details]
Followon patch v3

This version constifies the two functions Eric asked about, and removes unused args from a few other functions.
Comment 37 WebKit Commit Bot 2011-05-23 23:45:01 PDT
Comment on attachment 94518 [details]
Followon patch v3

Rejecting attachment 94518 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'build-..." exit_code: 2

Last 500 characters of output:
electors .......................................................................................................
fast/spatial-navigation .....
fast/spatial-navigation/snav-div-overflow-scrol-hidden.html -> failed
............
fast/spatial-navigation/snav-iframe-with-offscreen-focusable-element.html -> failed

Exiting early after 20 failures. 11150 tests run.
293.10s total testing time

11130 test cases (99%) succeeded
20 test cases (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8729415
Comment 38 WebKit Commit Bot 2011-05-23 23:45:05 PDT
Created attachment 94575 [details]
Archive of layout-test-results from cr-jail-7

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-7  Port: Mac  Platform: Mac OS X 10.6.7
Comment 39 Kenneth Rohde Christiansen 2011-05-24 02:11:47 PDT
Some platforms, like iOS uses two finger panning as a way to emulate wheel events and thus be able to scroll divs with overflow. They might be using a separate code path, but I thought that I should let you aware of this.
Comment 40 Kenneth Rohde Christiansen 2011-05-24 02:13:14 PDT
(In reply to comment #39)
> Some platforms, like iOS uses two finger panning as a way to emulate wheel events and thus be able to scroll divs with overflow. They might be using a separate code path, but I thought that I should let you aware of this.

Sorry too early here :-) I didn't notice the :hidden
Comment 41 Peter Kasting 2011-05-25 14:36:34 PDT
Created attachment 94862 [details]
Followon patch v4

This version reverts an unintentional change that broke calculation of |parentLayer| in RenderLayer::scrollRectToVisible().
Comment 42 WebKit Review Bot 2011-05-25 14:39:03 PDT
Attachment 94862 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebKit/mac/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 WebKit Review Bot 2011-05-25 22:27:05 PDT
Comment on attachment 94862 [details]
Followon patch v4

Attachment 94862 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8737076

New failing tests:
fast/events/autoscroll-with-non-scrollable-parent.html
fast/forms/input-readonly-autoscroll.html
fast/events/autoscroll-in-textfield.html
Comment 44 WebKit Review Bot 2011-05-25 22:27:11 PDT
Created attachment 94917 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 45 Peter Kasting 2011-05-26 14:21:38 PDT
Created attachment 95039 [details]
Followon patch v5

This reduces the scope of the change to only affect pan scrolls specifically, and avoid affecting anyone else who's currently checking for whether an area can be programmatically scrolled.

By now the patch has changed enough from the reviewed version that I'm more comfortable asking for re-review.
Comment 46 WebKit Review Bot 2011-05-26 14:24:45 PDT
Attachment 95039 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/page/EventHandler.cpp:533:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 47 Peter Kasting 2011-05-26 14:26:33 PDT
Created attachment 95041 [details]
Followon patch v6

Fix style nit.
Comment 48 Antonio Gomes 2011-05-26 21:37:44 PDT
Comment on attachment 95041 [details]
Followon patch v6

View in context: https://bugs.webkit.org/attachment.cgi?id=95041&action=review

Peter, I know it might sound overkill to you, but I really think the removal of parameters should go in a different patch/bug /// (It could even block this one). It would make this and the supposed other patch much cleaner.

Can we go for it?

> Source/WebCore/ChangeLog:7
> +        wheel scrolls.  This also removes some unused arguments from a couple of

>>> "    " This. 

To much space.
Comment 49 Peter Kasting 2011-05-27 10:52:11 PDT
(In reply to comment #48)
> (From update of attachment 95041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95041&action=review
> 
> Peter, I know it might sound overkill to you, but I really think the removal of parameters should go in a different patch/bug /// (It could even block this one). It would make this and the supposed other patch much cleaner.

Sure, that's easy.  Filed bug 61648.  I'll break the patch into two pieces and post one each on that bug and this one.
Comment 50 Antonio Gomes 2011-05-27 11:27:29 PDT
(In reply to comment #49)
> (In reply to comment #48)
> > (From update of attachment 95041 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95041&action=review
> > 
> > Peter, I know it might sound overkill to you, but I really think the removal of parameters should go in a different patch/bug /// (It could even block this one). It would make this and the supposed other patch much cleaner.
> 
> Sure, that's easy.  Filed bug 61648.  I'll break the patch into two pieces and post one each on that bug and this one.

Thanks. Shame that I got this:

Access Denied
You are not authorized to access PR #61648.
Comment 51 Peter Kasting 2011-05-27 13:36:45 PDT
(In reply to comment #50)
> Access Denied
> You are not authorized to access PR #61648.

That's weird.  I promise I didn't mark it as a security bug...
Comment 52 Peter Kasting 2011-06-02 14:59:12 PDT
Created attachment 95818 [details]
Followon patch v7

Now without unrelated cleanup.
Comment 53 WebKit Review Bot 2011-06-02 15:15:13 PDT
Comment on attachment 95818 [details]
Followon patch v7

Attachment 95818 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8754871
Comment 54 Peter Kasting 2011-06-02 17:26:49 PDT
Created attachment 95840 [details]
Followon patch v8
Comment 55 Peter Kasting 2011-06-03 11:06:06 PDT
Created attachment 95929 [details]
Followon patch v9
Comment 56 Kenneth Rohde Christiansen 2011-06-05 09:03:54 PDT
Comment on attachment 95929 [details]
Followon patch v9

View in context: https://bugs.webkit.org/attachment.cgi?id=95929&action=review

> Source/WebCore/dom/Node.cpp:2808
> +            // Allow pan-scrolling only on areas with scrollbars, to match the
> +            // behavior of the mouse wheel and arrow keys.

I dont like this description because there are platforms not showing scrollbars (and maybe not even scroll indicators). Would it be possible to put it differently?

> Source/WebCore/dom/Node.cpp:2819
> +            while (renderer && (!renderer->isBox() || !toRenderBox(renderer)->canBeScrolledAndHasScrollableArea(true)))

I wonder what that true means, coudl you add /* description */ true ?

> Source/WebCore/page/EventHandler.cpp:536
> +    if (toRenderBox(renderer)->canBeScrolledAndHasScrollableArea(false))

same here

> Source/WebCore/rendering/RenderBox.cpp:642
> +bool RenderBox::canBeScrolledAndHasScrollableArea(bool mustHaveScrollbar) const

What is platforms disable scrollbars? I dont like this depending on actually having scrollbars, but well.

> Source/WebCore/rendering/RenderLayer.cpp:1284
> +    return frameView && frameView->verticalScrollbar();

I expect some platforms to not even create the scrollbars, maybe iOS, Android, others... how will this work?
Comment 57 Peter Kasting 2011-06-06 11:09:37 PDT
This patch is merely supposed to make pan-scrolling act like wheel and arrow scrolling already do.  Platforms with no scrollbars are presumably not using pan scrolling (or wheel or arrow scrolling) at all, but rather gestural scrolling.  I don't have the ability to compile iOS or Android, so I can't tell you for certain.  But I can't see why this would have any effect.
Comment 58 Eric Seidel (no email) 2011-06-18 13:34:31 PDT
Comment on attachment 80522 [details]
Followon patch v2

Cleared Eric Seidel's review+ from obsolete attachment 80522 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 59 Eric Seidel (no email) 2012-01-10 18:49:17 PST
Comment on attachment 95929 [details]
Followon patch v9

View in context: https://bugs.webkit.org/attachment.cgi?id=95929&action=review

Seems like this needs a refresh to be landed.

>> Source/WebCore/dom/Node.cpp:2819
>> +            while (renderer && (!renderer->isBox() || !toRenderBox(renderer)->canBeScrolledAndHasScrollableArea(true)))
> 
> I wonder what that true means, coudl you add /* description */ true ?

In general when that's the case, we should be usign an enum instead of a bool.
Comment 60 cuentanumerouno 2013-12-20 01:36:35 PST
Is this the same bug?

http://jsfiddle.net/dNk9v/ 

(Navigating with the TAB key should NOT break the transition properly working when clicking the tabs - legend tags)

In this example, the form inside a container with overflow:hidden is MOVED somehow to display the focused field, but such motion is NOT reflected in ANY CSS value, so it's impossible to control/prevent the transition to break.

More info here  http://stackoverflow.com/questions/20579803/how-do-i-stop-the-browser-from-jumping-to-the-focused-field

Regardless of the solution, whatever scrolls/shifts/moves/pulls the form field into the visible area of the container, should be reflected in the css, so we can control it.

As it is right now, a simple css transition that could be made with one line of JS, requires tens of lines to prevent defaults and manage EVERYTHING (focus, keydown, keyup, submit events… just because the focused field pulled into the visible area of the container BREAKS the css transition (the transition moves the form as I requested, not aware of whatever property the browser changed)

Or should I open another ticket?