Bug 136650

Summary: Rubber-banding in the DOM view of the WebInspector is really jumpy
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, buildbot, commit-queue, esprehn+autocc, glenn, kondapallykalyan, rniwa, sam, simon.fraser, thorton
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Reduction
none
Patch
simon.fraser: review+, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch darin: review+

Description Beth Dakin 2014-09-08 17:17:23 PDT
* SUMMARY
If you try to rubber-band in the DOM view of the WebInspector, things get a bit jumpy.

* STEPS TO REPRODUCE
1. Open a web page, I used http://www.webkit.org.
2. Open the WebInspector and make sure the DOM tree view is visible.
3. Make sure the DOM tree view is scrollable by expanding as many of the nodes as you can (I option-clicked the body).
4. Scroll and rubber-band the view with your mouse cursor over the nodes (you will notice the blue hightlight moving from node to node staying under your cursor).

* RESULTS
As you rubber-band, the view will jump around a bit.

* REGRESSION
This is new with the changes to add rubber-banding in overflow areas.

* NOTES
If you rubber-band over the scrollbar track while it is visible or when the WebInspector is not the frontmost window, you will not see the jumping, but you will notice the blue hightlight also does not change.

rdar://problem/18166043
Comment 1 Beth Dakin 2014-09-08 17:18:07 PDT
Created attachment 237821 [details]
Reduction
Comment 2 Beth Dakin 2014-09-08 17:21:27 PDT
Created attachment 237822 [details]
Patch
Comment 3 Simon Fraser (smfr) 2014-09-08 17:30:29 PDT
Comment on attachment 237822 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:1390
> +    if (!renderer().frame().settings().rubberBandingForOverflowScrollEnabled())
> +        return false;

Do you need to check the setting here, since presumably overhangAmount() will just return 0,0 otherwise?
Comment 4 Beth Dakin 2014-09-08 17:35:45 PDT
(In reply to comment #3)
> (From update of attachment 237822 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237822&action=review
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1390
> > +    if (!renderer().frame().settings().rubberBandingForOverflowScrollEnabled())
> > +        return false;
> 
> Do you need to check the setting here, since presumably overhangAmount() will just return 0,0 otherwise?

True. Probably no need to belt-and-suspenders here.
Comment 5 Build Bot 2014-09-08 18:14:20 PDT
Comment on attachment 237822 [details]
Patch

Attachment 237822 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6264184811552768

New failing tests:
css3/flexbox/flexbox-overflow-auto.html
css3/flexbox/flexbox-baseline.html
css3/flexbox/child-overflow.html
Comment 6 Build Bot 2014-09-08 18:14:23 PDT
Created attachment 237826 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 7 Beth Dakin 2014-09-09 15:50:00 PDT
Created attachment 237865 [details]
Patch

Here's a new patch that shouldn't break any tests or other ports. RenderLayer should call into ScrollAnimatorMac's implementation of isRubberBandInProgress() that will check to ensure there is a non-zero overhangAmount (like my first patch did) but it will also make sure you are in the middle of a momentum gesture.
Comment 8 Darin Adler 2014-09-10 08:32:04 PDT
Comment on attachment 237865 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:1399
> +    return false;
> +#else
> +    return false;
> +#endif

Could omit the else and share the "return false". Not sure if it would be better or not.
Comment 9 Beth Dakin 2014-09-10 13:41:15 PDT
http://trac.webkit.org/changeset/173484
Comment 10 Beth Dakin 2014-09-10 13:42:11 PDT
Thanks Darin!
Comment 11 Simon Fraser (smfr) 2014-11-10 20:36:55 PST
This caused bug 138525.