Bug 26229 - Can't click outside the slider thumb and start dragging
Summary: Can't click outside the slider thumb and start dragging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Pierre d'Herbemont
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-05 15:50 PDT by Pierre d'Herbemont
Modified: 2009-07-09 14:24 PDT (History)
3 users (show)

See Also:


Attachments
patch v1. (5.71 KB, patch)
2009-06-05 16:10 PDT, Pierre d'Herbemont
no flags Details | Formatted Diff | Diff
patch v2. (5.65 KB, text/plain)
2009-06-05 16:16 PDT, Pierre d'Herbemont
no flags Details
patch v3. (12.28 KB, patch)
2009-07-08 18:02 PDT, Pierre d'Herbemont
simon.fraser: review-
Details | Formatted Diff | Diff
patch v4. (12.18 KB, patch)
2009-07-08 20:56 PDT, Pierre d'Herbemont
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre d'Herbemont 2009-06-05 15:50:32 PDT
STEPS:
1- Open LayoutTests/media/video-played.html
2- Wait for the page to be loaded
3- Click on the video slider where the slider thumb is not
4- attempt to slide it

OCCURS:
4- Doesn't move the slider thumb.
Comment 1 Pierre d'Herbemont 2009-06-05 16:10:41 PDT
Created attachment 31017 [details]
patch v1.

Patch attached.
Comment 2 Pierre d'Herbemont 2009-06-05 16:16:11 PDT
Created attachment 31018 [details]
patch v2.

This time with a changelog diff that will apply a bit better.
Comment 3 Pierre d'Herbemont 2009-06-05 16:57:20 PDT
Comment on attachment 31018 [details]
patch v2.

In the case of the mouse click on the slider we shouldn't account the mouse position.
Comment 4 Pierre d'Herbemont 2009-07-08 18:02:03 PDT
Created attachment 32494 [details]
patch v3.
Comment 5 Simon Fraser (smfr) 2009-07-08 18:15:53 PDT
Comment on attachment 32494 [details]
patch v3.

> diff --git a/WebCore/rendering/RenderSlider.cpp b/WebCore/rendering/RenderSlider.cpp
> index 610a060..9e00d68 100644
> --- a/WebCore/rendering/RenderSlider.cpp
> +++ b/WebCore/rendering/RenderSlider.cpp
> @@ -127,41 +127,52 @@ private:
>      virtual Node* shadowParentNode() { return m_shadowParent; }
>  
>      Node* m_shadowParent;
> -    FloatPoint m_initialClickPoint;       // initial click point in RenderSlider-local coordinates
> -    int m_initialPosition;
>      bool m_inDragMode;
> +    FloatPoint m_mouseDragVectorToThumb;

Please move this above the bool to optimize packing. I'm not sure 'mouseDragVector' is really tbe best way to describe this. Is it "offsetFromThumb"? Maybe it should be a FloatSize? 

> +    , m_mouseDragVectorToThumb(0, 0)

No need to init a FloatPoint.

> +                }
> +                else {

Should be } else { on one line.

> +                    // We are outside the thumb, move the thumb to the point were
> +                    // we clicked. We'll be exactly at the center of the thumb.
> +                    m_mouseDragVectorToThumb.setX(0);
> +                    m_mouseDragVectorToThumb.setY(0);

Is it possible for the click to be off the end of the slider, so we'd end up with a negative thumb position? i.e. does this code need to handle end conditions?

> +FloatPoint RenderSlider::mouseEventVectorToThumb(MouseEvent* evt)
> +{
> +    ASSERT (m_thumb && m_thumb->renderer());
> +    FloatPoint localPoint = m_thumb->renderBox()->absoluteToLocal(evt->absoluteLocation(), false, true);
> +    IntRect thumbBounds = m_thumb->renderBox()->borderBoxRect();
> +    FloatPoint distance;
> +    distance.setX(thumbBounds.x() + thumbBounds.height() / 2 - localPoint.x());
> +    distance.setY(thumbBounds.y() + thumbBounds.width() / 2 - localPoint.y());

I think you have height and width switched.

> +    return distance;

You call this distance, but the method says vector.

r- because I think this needs another revision, but it's certainly in the right direction.
Comment 6 Pierre d'Herbemont 2009-07-08 20:55:41 PDT
(In reply to comment #5)
> (From update of attachment 32494 [details])
> > diff --git a/WebCore/rendering/RenderSlider.cpp b/WebCore/rendering/RenderSlider.cpp
> > index 610a060..9e00d68 100644
> > --- a/WebCore/rendering/RenderSlider.cpp
> > +++ b/WebCore/rendering/RenderSlider.cpp
> > @@ -127,41 +127,52 @@ private:
> >      virtual Node* shadowParentNode() { return m_shadowParent; }
> >  
> >      Node* m_shadowParent;
> > -    FloatPoint m_initialClickPoint;       // initial click point in RenderSlider-local coordinates
> > -    int m_initialPosition;
> >      bool m_inDragMode;
> > +    FloatPoint m_mouseDragVectorToThumb;
> 
> Please move this above the bool to optimize packing. I'm not sure
> 'mouseDragVector' is really tbe best way to describe this. Is it
> "offsetFromThumb"? Maybe it should be a FloatSize? 

As this is an offset/vector and can contain negative value a Size is not appropriate.

> > +                    // We are outside the thumb, move the thumb to the point were
> > +                    // we clicked. We'll be exactly at the center of the thumb.
> > +                    m_mouseDragVectorToThumb.setX(0);
> > +                    m_mouseDragVectorToThumb.setY(0);
> 
> Is it possible for the click to be off the end of the slider, so we'd end up
> with a negative thumb position? i.e. does this code need to handle end
> conditions?

It's being handled properly already: We use the same code path than before, and we'll place the thumb in a constrained interval through RenderSlider::positionForOffset.

Rest should be addressed, thanks.
Comment 7 Pierre d'Herbemont 2009-07-08 20:56:05 PDT
Created attachment 32500 [details]
patch v4.
Comment 8 Pierre d'Herbemont 2009-07-09 14:24:14 PDT
http://trac.webkit.org/changeset/45658