Bug 39417 – Autoscroll for selection caret should be available on fullscreen view

• Status: UNCONFIRMED
• Component: HTML Editing

[Hajime Morrita]

Created attachment 56580 [details]
a repro

Derived from http://crbug.com/44435 but the problem is applicable all browsers that support fullscreen view.

How to reproduce:
- switch to fullscreen view
- open attached html
- select text with mouse, dragging it to right side of the view

Expected result:
- The view scrolls to right
What happens instead:
- The view doesn't scroll

[Hajime Morrita]

Created attachment 56659 [details]
patch v0

[Kent Tamura]

Comment on attachment 56659 [details]
patch v0

LayoutTests/fast/events/autoscroll-border-margin-vertical-body.html:67
 +  This is vertically large box...
I don't like tests and results have many empty lines.
How about using CSS height like <pre id="line" style="height:1000px;"> ?

[Hajime Morrita]

Created attachment 56964 [details]
patch v1

[Hajime Morrita]

Hi Kent-san, thank you for the feedback and sorry for a slow reply.
I updated the patch.

> LayoutTests/fast/events/autoscroll-border-margin-vertical-body.html:67
>  +  This is vertically large box...
> I don't like tests and results have many empty lines.
> How about using CSS height like <pre id="line" style="height:1000px;"> ?
Agreed. fixed to use style instead of blank lines.

[Hajime Morrita]

Created attachment 56973 [details]
rebased to make bots work

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/IntRect.h:156:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 44 files


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

[WebKit Review Bot]

Attachment 56973 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2268641

[Hajime Morrita]

Created attachment 56976 [details]
fixed style violation, chromium build error

[Hajime Morrita]

Created attachment 57295 [details]
patch v0

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/IntRect.cpp:61:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/IntRect.cpp:63:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/IntRect.cpp:65:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/graphics/IntRect.cpp:67:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/chromium/src/WebViewImpl.cpp:1670:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/chromium/src/WebViewImpl.cpp:1681:  Missing spaces around /  [whitespace/operators] [3]
WebKit/chromium/src/WebViewImpl.cpp:1690:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 7 in 7 files


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

[Hajime Morrita]

Comment on attachment 57295 [details]
patch v0

Cancelling due to wrong bug id. I'sorry for disturbing you...

[Hajime Morrita]

Comment on attachment 56976 [details]
fixed style violation, chromium build error

This patch is for review. (I had cancelled this accidentally.)

[Peter Kasting]

Comment on attachment 56976 [details]
fixed style violation, chromium build error

> --- a/WebCore/platform/graphics/IntRect.h
> +++ b/WebCore/platform/graphics/IntRect.h
> @@ -136,6 +136,29 @@ public:
>          m_size.setHeight(m_size.height() + dy + dy);
>      }
>      void inflate(int d) { inflateX(d); inflateY(d); }
> +
> +    void deflateX(int dx)
> +    {
> +        m_location.setX(m_location.x() + dx);
> +        if (dx + dx < m_size.width())
> +            m_size.setWidth(m_size.width() - (dx + dx));
> +        else
> +            m_size.setWidth(0);
> +    }
> +    void deflateY(int dy)
> +    {
> +        m_location.setY(m_location.y() + dy);
> +        if (dy + dy < m_size.height())
> +            m_size.setHeight(m_size.height() - (dy + dy));
> +        else
> +            m_size.setHeight(0);
> +    }
> +    void deflate(int d)
> +    {
> +        deflateX(d);
> +        deflateY(d);
> +    }

It seems like instead of adding these, it might be better to use the existing inflate() functions, and have IntSize::set{Width,Height}() clamp the minimum size to zero.

However, you'd have to make sure we don't actually rely on negative sizes anywhere.  (It seems like a bug if we do.)  Might want to run this idea past Darin Adler.

[Tony Chang]

Comment on attachment 56976 [details]
fixed style violation, chromium build error

> +++ b/LayoutTests/fast/events/autoscroll-border-margin-horizontal-body.html
> +                    var endY = startY;
> +                    eventSender.mouseMoveTo(endX, endY);
> +                }
> +
> +                setTimeout(afterDragging, 1000);

Is it possible to avoid this timeout (maybe by using eventSender.leapFoward)?  Is there an event we can get instead?  Or maybe we can poll every 50ms to check for changes?

Also, is it possible to merge some or all of these tests into the same .html file?

> diff --git a/WebCore/page/Settings.h b/WebCore/page/Settings.h
> index dc3b50fb7ad8e423733754deb6ecf03974ef29e6..cc9f35780219435e9d3bc2a2e8252c84b8f35097 100644
> --- a/WebCore/page/Settings.h
> +++ b/WebCore/page/Settings.h
> @@ -311,6 +311,9 @@ namespace WebCore {
>          void setHTML5ParserEnabled(bool flag) { m_html5ParserEnabled = flag; }
>          bool html5ParserEnabled() const { return m_html5ParserEnabled; }
>  
> +        void setAutoscrollBorderMargin(unsigned value) { m_autoscrollBorderMargin = value; }
> +        unsigned autoscrollBorderMargin() const { return m_autoscrollBorderMargin; }
> +

I don't know if this should be a setting or not.  For example, the minimum drag distances (hysteresis) are hard coded in EventHandler.cpp.  I bet this is a platform specific value.

> --- a/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h
> +++ b/WebKitTools/DumpRenderTree/qt/LayoutTestControllerQt.h
> @@ -187,6 +187,8 @@ public slots:
>      */
>      void setScrollbarPolicy(const QString& orientation, const QString& policy);
>  
> +    void setautoscrollbordermargin(unsigned);
> +

This being in lowercase is probably causing the QT bot to be red.

[Hajime Morrita]

Created attachment 58322 [details]
patch v6

[Hajime Morrita]

Kasting, Tony, thank you for your feedback, and I'm sorry for my slow response.

> > +    void deflate(int d)
> > +    {
> > +        deflateX(d);
> > +        deflateY(d);
> > +    }
> 
> It seems like instead of adding these, it might be better to use the existing inflate() functions, and 
have IntSize::set{Width,Height}() clamp the minimum size to zero.
Fixed to implement deflate(x) with inflate(-x).

> However, you'd have to make sure we don't actually rely on negative sizes anywhere.  (It seems like a bug if we do.)  Might want to run this idea past Darin Adler.
Agreed. In this case, "inflate" might be just no longer a good name.

(In reply to comment #14)
> > +                setTimeout(afterDragging, 1000);
> 
> Is it possible to avoid this timeout (maybe by using eventSender.leapFoward)?  Is there an event we can get instead?  Or maybe we can poll every 50ms to check for changes?
Done.

> 
> Also, is it possible to merge some or all of these tests into the same .html file?
Done.

> > +        void setAutoscrollBorderMargin(unsigned value) { m_autoscrollBorderMargin = value; }
> > +        unsigned autoscrollBorderMargin() const { return m_autoscrollBorderMargin; }
> > +
> 
> I don't know if this should be a setting or not.  For example, the minimum drag distances (hysteresis) are hard coded in EventHandler.cpp.  I bet this is a platform specific value.
Agreed. I moved it to ScreenView::m_autoscrollBorderMargin and gave it to
platform specific value. (It's currently only for chromium.) I left its setter for testing purpose.

> >  
> > +    void setautoscrollbordermargin(unsigned);
> > +
> 
> This being in lowercase is probably causing the QT bot to be red.
Oops. fixed.

According to the feedback, the behavior goes stateless, instead of fullscreen-specific.
Although no other browser cares this issue, I think it's worth to care it because
Fullscreen-like usage will grow on more lightweight devices such as netbooks.

[Tony Chang]

I tested this on IE and Firefox.

Firefox 3.6 only autoscrolls if you are over a scrollbar.  This means it's not possible to autoscroll a selection left when maximized or fullscreen because there's no autoscroll area.  Another way to describe this is that hovering over a scrollbar is treated as being outside the content area, therefore it triggers autoscrolling.

IE 8 also autoscrolls when you are over a scrollbar, but it also autoscrolls if you are within about 3 pixels of an edge.  This happens even when not in fullscreen mode.  If there's a scrollbar, this happens within about 3 pixels of the scrollbar.  This also applies to iframes, but doesn't seem to apply to div's with scrollbars (see URL for test case, the div case is probably a bug).

So I think there are 2 bugs:
1) We should not count scrollbars as part of the content area when computing the drag speed.  It looks like WebKit currently determines the drag speed based on the mouse pointer distance outside the page.  Fixing this would fix the original bug report at http://crbug.com/44435

2) If within 3 pixels of an edge, autoscroll like IE.  I'm not 100% sure we should do this on all platforms because IE is the only application that seems to do this.  Wordpad (win) and TextEdit (mac) don't do this.  Maybe windows only.  Anyway, it seems like this should be a separate bug.

Your patch seems closer to (2), but it doesn't take into account scrollbars and seems to be too big (30 instead of only 3).  Maybe we should fix (1) first?

[Hajime Morrita]

Hi Tony, thank your for your investigation!

> Your patch seems closer to (2), but it doesn't take into account scrollbars and seems to be too big (30 instead of only 3).  Maybe we should fix (1) first?
That's totally makes sense!
I filed (1) as Bug 40403.
It looks good idea to tackle 40403 first because it is apparently a bug .
 
Anyway, I'll cancel this review due to pixel width issue you mentioned.