Bug 119760 – Text dragging can scroll overflow:hidden boxes

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Antonio Gomes]

From https://code.google.com/p/chromium/issues/detail?id=116655

URLs (if applicable) : http://jsfiddle.net/auk9S/8/

Windows
 Chrome 17.0.963.56 (Official Build 121963) m: FAIL
      Safari 5: FAIL
 Firefox 9.0.1: OK
          IE 8: OK
Linux
 Chrome 16.0.912.63 (Official Build 113337): FAIL

What steps will reproduce the problem?
1. Click inside the text box, hold the mouse button and drag

What is the expected result?
 Nothing

What happens instead?
 The div scrolls around

[Antonio Gomes]

Created attachment 208655 [details]
patch for EWS

[Build Bot]

Comment on attachment 208655 [details]
patch for EWS

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

New failing tests:
editing/input/caret-at-the-edge-of-input.html
editing/input/caret-at-the-edge-of-contenteditable.html
fast/forms/input-readonly-autoscroll.html
fast/forms/input-text-scroll-left-on-blur.html
fast/transforms/scrollIntoView-transformed.html

[Build Bot]

Created attachment 208694 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Simon Fraser (smfr)]

This is a behavior change that I feel like we've discussed in the past, but can't find relevant discussions.

[Antonio Gomes]

I can not find it either, but still believe it should be fixed.

The patch though is not 100% correct, and the EWS failures are real .. it is being reworked, and tests being added.

[Antonio Gomes]

Created attachment 208767 [details]
patch 2 for EWS

[Antonio Gomes]

Created attachment 208809 [details]
patch 3 for review

[Antonio Gomes]

Created attachment 208832 [details]
patch 3.1 for review

[Antonio Gomes]

Simon/Beth: would you guys mind to have a look?

[Build Bot]

Comment on attachment 208832 [details]
patch 3.1 for review

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

New failing tests:
fast/overflow/scrollRevealButton.html
http/tests/navigation/anchor-frames-same-origin.html
fast/events/autoscroll-nonscrollable-iframe-in-scrollable-div.html
fast/dom/HTMLAnchorElement/anchor-in-noscroll-iframe.html
fast/transforms/scrollIntoView-transformed.html

[Build Bot]

Created attachment 208878 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Build Bot]

Comment on attachment 208832 [details]
patch 3.1 for review

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

New failing tests:
fast/overflow/scrollRevealButton.html
fast/events/autoscroll-nonscrollable-iframe-in-scrollable-div.html
fast/dom/HTMLAnchorElement/anchor-in-noscroll-iframe.html
fast/transforms/scrollIntoView-transformed.html

[Build Bot]

Created attachment 208966 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Antonio Gomes]

Created attachment 209158 [details]
patch 4 - for EWS

[Antonio Gomes]

Created attachment 209159 [details]
patch 4.1 - for EWS

[WebKit Commit Bot]

Attachment 209159 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/events/autoscroll-upwards-propagation-expected.txt', u'LayoutTests/fast/events/autoscroll-upwards-propagation.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderLayer.cpp']" exit_code: 1
Source/WebCore/rendering/RenderLayer.cpp:1407:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

Created attachment 209160 [details]
patch 4.2 - for review

[Antonio Gomes]

Comment on attachment 209160 [details]
patch 4.2 - for review

Patch for review. It fixes the way autoscroll gets propagated upwards by taking the layer scrollability into account.

[Antonio Gomes]

(In reply to comment #18)
> (From update of attachment 209160 [details])
> Patch for review. It fixes the way autoscroll gets propagated upwards by taking the layer scrollability into account.

Will add to the ChangeLog that the fix makes WebKit's autoscroll to behavior like Firefox' and Opera12's (pre-blink) with regards to upwards scroll propagation.

[Darin Adler]

Comment on attachment 209160 [details]
patch 4.2 - for review

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

Looks OK to me.

> Source/WebCore/rendering/RenderLayer.cpp:1397
> +    RenderObject* renderer = layer->renderer();
> +    Document* document = renderer->document();
> +    if (document) {
> +        HTMLFrameOwnerElement* ownerElement = document->ownerElement();
> +        if (ownerElement) {
> +            RenderObject* ownerRenderer = ownerElement->renderer();
> +            if (ownerRenderer)
> +                return ownerRenderer->enclosingLayer();
> +        }
> +    }

I prefer early return, with various calls to return 0.

Or if you are going to nest like this, then I suggest putting the definitions inside the if:

    if (Document* document = layer->renderer()->document())

Which would make the code much more brief and thus easier for read.

I don’t think we need the local variable, renderer.

[Antonio Gomes]

(In reply to comment #20)
> (From update of attachment 209160 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209160&action=review
> 
> Looks OK to me.
> 
> > Source/WebCore/rendering/RenderLayer.cpp:1397
> > +    RenderObject* renderer = layer->renderer();
> > +    Document* document = renderer->document();
> > +    if (document) {
> > +        HTMLFrameOwnerElement* ownerElement = document->ownerElement();
> > +        if (ownerElement) {
> > +            RenderObject* ownerRenderer = ownerElement->renderer();
> > +            if (ownerRenderer)
> > +                return ownerRenderer->enclosingLayer();
> > +        }
> > +    }
> 
> I prefer early return, with various calls to return 0.
> 
> Or if you are going to nest like this, then I suggest putting the definitions inside the if:
> 
>     if (Document* document = layer->renderer()->document())
> 
> Which would make the code much more brief and thus easier for read.
> 
> I don’t think we need the local variable, renderer.

Fixed and landed: https://trac.webkit.org/changeset/154382