Summary: | Wheel scrolls and autoscrolls should not scroll overflow:hidden | ||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Kasting <pkasting> | ||||||||||||||||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Peter Kasting <pkasting> | ||||||||||||||||||||||||||||||||||||
Status: | REOPENED --- | ||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | adele, commit-queue, cuentanumerouno, dglazkov, hyatt, jamesr, kenneth, peter, tonikitoo, webkit.review.bot | ||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Peter Kasting
2010-10-01 18:00:18 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.
Created attachment 69557 [details]
WIP patch w/test
Oops, forgot to svn add the LayoutTest.
Created attachment 69874 [details]
patch v1
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.
Created attachment 69876 [details]
patch v1.1
Man, none of those was even my fault.
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.
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 on attachment 69988 [details]
Reduced patch for 517 branch
Landing by hand
Committed r69257: <http://trac.webkit.org/changeset/69257> Not fixed yet Comment on attachment 69988 [details]
Reduced patch for 517 branch
Marking obsolete as this was (partly) landed
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.
Created attachment 70021 [details]
include WebCore.exp.in changes so this builds on mac
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 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. 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.
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 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 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. Created attachment 80522 [details]
Followon patch v2
Will this break scrolling in text fields, which use overflow:hidden? (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? 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? (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. 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. What about autoscrolling? (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? hmm maybe I'm not talking about the same thing here - when you say autoscroll, do you mean, pan-scroll? Because I don't :) (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. Ok sounds like this isn't an issue then. Just for future reference though - autoscroll != pan-scroll in WebKit. Comment on attachment 80522 [details]
Followon patch v2
I suspect this patch no longer applies since it's 4 months old now.
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 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 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 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. 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 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 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
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. (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 Created attachment 94862 [details]
Followon patch v4
This version reverts an unintentional change that broke calculation of |parentLayer| in RenderLayer::scrollRectToVisible().
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 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 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
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.
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.
Created attachment 95041 [details]
Followon patch v6
Fix style nit.
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. (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. (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. (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... Created attachment 95818 [details]
Followon patch v7
Now without unrelated cleanup.
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 Created attachment 95840 [details]
Followon patch v8
Created attachment 95929 [details]
Followon patch v9
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? 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 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 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. 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? |