Closed Bug 846380 Opened 7 years ago Closed 7 years ago

Add support for dragging of <input type=range>'s thumb using mouse/touch events

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file, 3 obsolete files)

We need support for dragging of <input type=range>'s thumb using mouse/touch events.
Attached patch patch (obsolete) — Splinter Review
nsRangeFrame::GetValueAtEventPoint will change and become a fair bit more complex depending on what we decide in bug 842021 and bug 842179, I've not put a lot of effort into polishing the logic there. For now it works as it should for the default styling of range.
Attachment #719565 - Flags: review?(bugs)
Comment on attachment 719565 [details] [diff] [review]
patch

># HG changeset patch
># Parent 3a22ec067c9e35f02e34b93ce804942e6c932637
># User Jonathan Watt <jwatt@jwatt.org>
>Bug 846380 - Add support for dragging of <input type=range>'s thumb using mouse/touch events. r=smaug.
>
>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
>--- a/content/html/content/src/nsHTMLInputElement.cpp
>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>@@ -29,16 +29,17 @@
> #include "nsIForm.h"
> #include "nsFormSubmission.h"
> #include "nsFormSubmissionConstants.h"
> #include "nsIDocument.h"
> #include "nsIPresShell.h"
> #include "nsIFormControlFrame.h"
> #include "nsITextControlFrame.h"
> #include "nsIFrame.h"
>+#include "nsRangeFrame.h"
> #include "nsEventStates.h"
> #include "nsIServiceManager.h"
> #include "nsError.h"
> #include "nsIEditor.h"
> #include "nsGUIEvent.h"
> #include "nsIIOService.h"
> #include "nsDocument.h"
> #include "nsAttrValueOrString.h"
>@@ -578,16 +579,17 @@ nsHTMLInputElement::nsHTMLInputElement(a
>   , mParserCreating(aFromParser != NOT_FROM_PARSER)
>   , mInInternalActivate(false)
>   , mCheckedIsToggled(false)
>   , mIndeterminate(false)
>   , mInhibitRestoration(aFromParser & FROM_PARSER_FRAGMENT)
>   , mCanShowValidUI(true)
>   , mCanShowInvalidUI(true)
>   , mHasRange(false)
>+  , mIsDraggingRange(false)
> {
>   // We are in a type=text so we now we currenty need a nsTextEditorState.
>   mInputData.mState = new nsTextEditorState(this);
> 
>   if (!gUploadLastDir)
>     nsHTMLInputElement::InitUploadLastDir();
> 
>   // Set up our default state.  By default we're enabled (since we're
>@@ -2509,16 +2511,20 @@ nsHTMLInputElement::PreHandleEvent(nsEve
>         }
>         break;
> 
>       default:
>         break;
>     }
>   }
> 
>+  if (mType == NS_FORM_INPUT_RANGE) {
>+    PreHandleEventForRangeThumb(aVisitor);
>+  }
>+
>   if (originalCheckedValue) {
>     aVisitor.mItemFlags |= NS_ORIGINAL_CHECKED_VALUE;
>   }
> 
>   // If mNoContentDispatch is true we will not allow content to handle
>   // this event.  But to allow middle mouse button paste to work we must allow 
>   // middle clicks to go to text fields anyway.
>   if (aVisitor.mEvent->mFlags.mNoContentDispatch) {
>@@ -2546,16 +2552,131 @@ nsHTMLInputElement::PreHandleEvent(nsEve
>       SetValueInternal(aValue, false, false);
>     }
>     FireChangeEventIfNeeded();
>   }
> 
>   return nsGenericHTMLFormElement::PreHandleEvent(aVisitor);
> }
> 
>+void
>+nsHTMLInputElement::PreHandleEventForRangeThumb(nsEventChainPreVisitor& aVisitor)
>+{
>+  // Note there are several reasons that we want to do the handling in this
>+  // method under PreHandleEvent instead of PostHandleEvent. One reason is that
>+  // we need the SetCapturingContent calls for mousedown, touchstart, mouseup
>+  // and touchend to happen before nsFrame::HandlePress/HandleRelease get a
>+  // chance to make their calls, since they don't pass the flags we need.
>+
>+  // Note: we do not use NoDefault, since that would prevent things we want
>+  // such as being given focus by nsEventStateManager::PostHandleEvent().
>+
>+  MOZ_ASSERT(mType == NS_FORM_INPUT_RANGE);
>+
>+  if (nsEventStatus_eConsumeNoDefault == aVisitor.mEventStatus ||
>+      !(aVisitor.mEvent->eventStructType == NS_MOUSE_EVENT ||
>+        aVisitor.mEvent->eventStructType == NS_TOUCH_EVENT ||
>+        aVisitor.mEvent->eventStructType == NS_KEY_EVENT)) {
>+    return;
>+  }
>+
>+  nsRangeFrame* rangeFrame = do_QueryFrame(GetPrimaryFrame());
>+  if (!rangeFrame) {
>+    CancelRangeThumbDrag();
>+    return;
>+  }
>+
>+  switch (aVisitor.mEvent->message)
>+  {
>+    case NS_MOUSE_BUTTON_DOWN:
>+    case NS_TOUCH_START: {
>+      MOZ_ASSERT(!mIsDraggingRange);
>+      nsInputEvent *inputEvent = static_cast<nsInputEvent*>(aVisitor.mEvent);
nsInputEvent*

>+        nsTouchEvent *touchEvent = static_cast<nsTouchEvent*>(aVisitor.mEvent);
nsTouchEvent*

>+    case NS_TOUCH_MOVE:
>+      if (!mIsDraggingRange)
>+        break;
Always {} with if
Also, is if (!mIsDraggingRange) enough always. Should we perhaps also check that 
currently captured element is really |this|.

>+      SetValueOfRangeForUserEvent(rangeFrame->GetValueAtEventPoint(
>+                                    static_cast<nsInputEvent*>(aVisitor.mEvent)));
>+      aVisitor.mEventStatus = nsEventStatus_eConsumeDoDefault;
>+      break;
>+
>+    case NS_MOUSE_BUTTON_UP:
>+    case NS_TOUCH_END:
>+      if (!mIsDraggingRange)
>+        break;
>+      nsIPresShell::SetCapturingContent(nullptr, 0);
>+      mIsDraggingRange = false;
>+      SetValueOfRangeForUserEvent(rangeFrame->GetValueAtEventPoint(
>+                                    static_cast<nsInputEvent*>(aVisitor.mEvent)));
>+      aVisitor.mEventStatus = nsEventStatus_eConsumeDoDefault;
>+      break;
Do we actually have to set mEventStatus in any case here?
Perhaps set mMultipleActionsPrevented

This all needs tests.
Attachment #719565 - Flags: review?(bugs) → review-
Depends on: 847453
Attached patch patch (obsolete) — Splinter Review
(In reply to Olli Pettay [:smaug] from comment #2)
> Also, is if (!mIsDraggingRange) enough always. Should we perhaps also check
> that 
> currently captured element is really |this|.

It's enough so long as nobody else grabs capture in the middle of a drag. I was kind of hoping that can never happen, but if in doubt I guess I'd have ask _you_, since you know much more about event handling. :) Anyway, I've added a check to cancel the dragging in the case that someone else grabbed capture, just to be safe.

> Do we actually have to set mEventStatus in any case here?
> Perhaps set mMultipleActionsPrevented

I forget which of the many issues I was having lead me to set mEventStatus, but in the mousedown case at least I think it was to prevent text selection from starting. Things still seem to work after changing to mMultipleActionsPrevented though, so I've made that change.
Attachment #719565 - Attachment is obsolete: true
Attachment #721010 - Flags: review?(bugs)
Attached patch patch (obsolete) — Splinter Review
Attachment #721010 - Attachment is obsolete: true
Attachment #721010 - Flags: review?(bugs)
Attachment #721011 - Flags: review?(bugs)
Attachment #721011 - Attachment is patch: true
Obviously NS_KEY_PRESS and NS_TOUCH_CANCEL need to be handled separately - I've changed that locally.
Comment on attachment 721011 [details] [diff] [review]
patch

># HG changeset patch
># Parent 1ab536b3472ef8e26e128c27d10da67d6fb69b9e
># User Jonathan Watt <jwatt@jwatt.org>
>Bug 846380 - Add support for dragging of <input type=range>'s thumb using mouse/touch events. r=smaug.
>* * *
>Bug 847453 - nsDOMWindowUtils::SendMouseEventCommon needs to set 'buttons' on the event. r=smaug.
>
>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
>--- a/content/html/content/src/nsHTMLInputElement.cpp
>+++ b/content/html/content/src/nsHTMLInputElement.cpp
>@@ -29,16 +29,17 @@
> #include "nsIForm.h"
> #include "nsFormSubmission.h"
> #include "nsFormSubmissionConstants.h"
> #include "nsIDocument.h"
> #include "nsIPresShell.h"
> #include "nsIFormControlFrame.h"
> #include "nsITextControlFrame.h"
> #include "nsIFrame.h"
>+#include "nsRangeFrame.h"
> #include "nsEventStates.h"
> #include "nsIServiceManager.h"
> #include "nsError.h"
> #include "nsIEditor.h"
> #include "nsGUIEvent.h"
> #include "nsIIOService.h"
> #include "nsDocument.h"
> #include "nsAttrValueOrString.h"
>@@ -578,16 +579,17 @@ nsHTMLInputElement::nsHTMLInputElement(a
>   , mParserCreating(aFromParser != NOT_FROM_PARSER)
>   , mInInternalActivate(false)
>   , mCheckedIsToggled(false)
>   , mIndeterminate(false)
>   , mInhibitRestoration(aFromParser & FROM_PARSER_FRAGMENT)
>   , mCanShowValidUI(true)
>   , mCanShowInvalidUI(true)
>   , mHasRange(false)
>+  , mIsDraggingRange(false)
> {
>   // We are in a type=text so we now we currenty need a nsTextEditorState.
>   mInputData.mState = new nsTextEditorState(this);
> 
>   if (!gUploadLastDir)
>     nsHTMLInputElement::InitUploadLastDir();
> 
>   // Set up our default state.  By default we're enabled (since we're
>@@ -2509,16 +2511,20 @@ nsHTMLInputElement::PreHandleEvent(nsEve
>         }
>         break;
> 
>       default:
>         break;
>     }
>   }
> 
>+  if (mType == NS_FORM_INPUT_RANGE) {
>+    PreHandleEventForRangeThumb(aVisitor);
>+  }
>+
>   if (originalCheckedValue) {
>     aVisitor.mItemFlags |= NS_ORIGINAL_CHECKED_VALUE;
>   }
> 
>   // If mNoContentDispatch is true we will not allow content to handle
>   // this event.  But to allow middle mouse button paste to work we must allow 
>   // middle clicks to go to text fields anyway.
>   if (aVisitor.mEvent->mFlags.mNoContentDispatch) {
>@@ -2546,16 +2552,150 @@ nsHTMLInputElement::PreHandleEvent(nsEve
>       SetValueInternal(aValue, false, false);
>     }
>     FireChangeEventIfNeeded();
>   }
> 
>   return nsGenericHTMLFormElement::PreHandleEvent(aVisitor);
> }
> 
>+void
>+nsHTMLInputElement::PreHandleEventForRangeThumb(nsEventChainPreVisitor& aVisitor)
>+{
>+  // Note there are several reasons that we want to do the handling in this
>+  // method under PreHandleEvent instead of PostHandleEvent. One reason is that
>+  // we need the SetCapturingContent calls for mousedown, touchstart, mouseup
>+  // and touchend to happen before nsFrame::HandlePress/HandleRelease get a
>+  // chance to make their calls, since they don't pass the flags we need.
>+
>+  // Note: we do not use NoDefault, since that would prevent things we want
>+  // such as being given focus by nsEventStateManager::PostHandleEvent().
>+
...
>+  switch (aVisitor.mEvent->message)
>+  {
>+    case NS_MOUSE_BUTTON_DOWN:
>+    case NS_TOUCH_START: {
>+      MOZ_ASSERT(!mIsDraggingRange);
>+      if (nsIPresShell::GetCapturingContent()) {
>+        break; // don't start drag if someone else is already capturing
>+      }
>+      nsInputEvent* inputEvent = static_cast<nsInputEvent*>(aVisitor.mEvent);
>+      if (inputEvent->IsShift() || inputEvent->IsControl() ||
>+          inputEvent->IsAlt() || inputEvent->IsMeta() ||
>+          inputEvent->IsAltGraph() || inputEvent->IsFn() ||
>+          inputEvent->IsOS()) {
>+        break; // ignore
>+      }
>+      if (aVisitor.mEvent->message == NS_MOUSE_BUTTON_DOWN) {
>+        nsMouseEvent* mouseEvent = static_cast<nsMouseEvent*>(aVisitor.mEvent);
>+        if (mouseEvent->buttons == nsMouseEvent::eLeftButtonFlag) {
>+          mIsDraggingRange = true;
>+          mRangeThumbDragStartValue = GetValueAsDouble();
>+          nsIPresShell::SetCapturingContent(this, CAPTURE_IGNOREALLOWED |
>+                                                  CAPTURE_RETARGETTOELEMENT);
Problem is that if this all happens during PreHandleEvent, calling someotherelement.setCapture doesn't work as expected.
Why can't down/up handling happen in PostHandleEvent if you thumb non-selectable?


>+++ b/dom/base/nsDOMWindowUtils.cpp
>@@ -814,16 +814,19 @@ nsDOMWindowUtils::SendTouchEvent(const n
>     msg = NS_TOUCH_CANCEL;
>   } else {
>     return NS_ERROR_UNEXPECTED;
>   }
>   nsTouchEvent event(true, msg, widget);
>   event.modifiers = GetWidgetModifiers(aModifiers);
>   event.widget = widget;
>   event.time = PR_Now();
>+  if (aCount > 0) {
>+    event.refPoint = ToWidgetPoint(aXs[0], aYs[0], offset, GetPresContext());
>+  }
Could you explain this?

>+++ b/layout/base/nsPresShell.cpp
>@@ -6883,16 +6883,17 @@ PresShell::DispatchTouchEvent(nsEvent *a
>         // Wrong document, don't dispatch anything.
>         continue;
>       }
>       content = capturingContent;
>     }
>     // copy the event
>     nsTouchEvent newEvent(touchEvent->mFlags.mIsTrusted, touchEvent);
>     newEvent.target = targetPtr;
>+    newEvent.refPoint = touch->mRefPoint;
And this

r- since I think at least some part of the event handling should be after content has had
chance to handle the event.
Attachment #721011 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #6)
> Problem is that if this all happens during PreHandleEvent, calling
> someotherelement.setCapture doesn't work as expected.
> Why can't down/up handling happen in PostHandleEvent if you thumb
> non-selectable?

Because by the time PostHandleEvent has been called, nsFrame::HandlePress has already been called and done things like calling nsIPresShell::SetCapturingContent passing in the nearest nsIScrollableFrame. Would it be acceptable to add a HandleEvent override to nsRangeFrame that does nothing in order to avoid this problem?

I think there were other issues too, but let me rebuild with the above and see if they still exist.
One of the other issues is that nsFrame::HandlePress will be called for the frames of the anonymous children. The only way around that is going to be to put |pointer-events:none!important| on them, which will potentially frustrate authors when they try to do something like:

::-moz-range-thumb:hover {
  background: orange;
}

That simply won't work because the :hover pseudo-class will never match when pointer-events is set to none. Instead authors will have to do:

input[type=range]:hover ::-moz-range-thumb {
  background: orange;
}

Or something, which is far from intuitive.
But why don't you make range and thumb not-selectable? nsFrame::HandlePress would then return
before doing anything.
The IsSelectable() check in nsFrame::HandlePress comes _after_ the code that calls nsIPresShell::SetCapturingContent, so setting the 'user-select' property will not help here.
(Neither will setting -moz-user-focus BTW.)
On debugging, it turns out that the reason user-select doesn't work for me is because the property name is -moz-user-select. Unfortunately I was misled by the MDC docs which redirect the -moz-user-select page to the user-select page, implying to me that the property had been unprefixed and I should now be using user-select. That is not the case, so I'm not sure what's up with the docs.
Attached patch patchSplinter Review
I've added preventDefault() tests too.
Attachment #721011 - Attachment is obsolete: true
Attachment #721243 - Flags: review?(bugs)
(I filed bug 847917 on the -moz-user-select docs FWIW.)
Comment on attachment 721243 [details] [diff] [review]
patch


>+nsHTMLInputElement::CancelRangeThumbDrag()
>+{
>+  if (mIsDraggingRange) {
>+    nsIPresShell::SetCapturingContent(nullptr, 0); // cancel capture
You should check here that range is actually capturing and not something else

>+      } else {
>+        nsTouchEvent* touchEvent = static_cast<nsTouchEvent*>(aVisitor.mEvent);
>+        if (touchEvent->touches.Length() == 1) {
>+          mIsDraggingRange = true;
>+          mRangeThumbDragStartValue = GetValueAsDouble();
>+          nsIPresShell::SetCapturingContent(this, CAPTURE_IGNOREALLOWED |
>+                                                  CAPTURE_RETARGETTOELEMENT);
>+          SetValueOfRangeForUserEvent(rangeFrame->GetValueAtEventPoint(touchEvent));
>+        } else {
>+          CancelRangeThumbDrag();
>+        }
I wonder if we'll need to tweak touch handling given that there are those legacy mouse events dispatched even when
touch events are used. This needs some testing on Android/b2g/Metrofox
In fact, what makes sure MOZ_ASSERT(!mIsDraggingRange); doesn't fire?
Maybe just return early if mIsDraggingRange is true in the mouse/touchdown case.

>+nsRangeFrame::GetValueAtEventPoint(nsGUIEvent* aEvent)

>+  if (aEvent->eventStructType == NS_TOUCH_EVENT) {
>+    MOZ_ASSERT(static_cast<nsTouchEvent*>(aEvent)->touches.Length() == 1,
>+               "Unexpected number of touches");
Why does this not fire? We can have several touches.


mounir should review this too. And if you have Android devices, could you please test this.
Attachment #721243 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #15)
> I wonder if we'll need to tweak touch handling given that there are those
> legacy mouse events dispatched even when
> touch events are used.

What legacy mouse events?

> This needs some testing on Android/b2g/Metrofox

Yes. Getting theming and whatever else needs to happen to get things working on Antroid etc. is the next stage.

> In fact, what makes sure MOZ_ASSERT(!mIsDraggingRange); doesn't fire?

The fact that if we're capturing, then we should always get any mouseup/dragend/dragcancel events and clean up?

> Maybe just return early if mIsDraggingRange is true in the mouse/touchdown
> case.

Okay.

> >+nsRangeFrame::GetValueAtEventPoint(nsGUIEvent* aEvent)
> 
> >+  if (aEvent->eventStructType == NS_TOUCH_EVENT) {
> >+    MOZ_ASSERT(static_cast<nsTouchEvent*>(aEvent)->touches.Length() == 1,
> >+               "Unexpected number of touches");
> Why does this not fire? We can have several touches.

In the case for NS_TOUCH_START in PostHandleEventForRangeThumb we call CancelRangeThumbDrag() if the touches count is not equal to 1.

> mounir should review this too.

Okay.

> And if you have Android devices, could you please test this.

I have been. It mostly works now, and the issues that there are will be follow-ups.
Attachment #721243 - Flags: review?(mounir)
per spec
"If the user agent interprets a sequence of touch events as a click, then it should dispatch mousemove, mousedown, mouseup, and click events (in that order) at the location of the touchend event for the corresponding touch input. If the contents of the document have changed during processing of the touch events, then the user agent may dispatch the mouse events to a different target than the touch events. "
Those sound like compatibility events rather than legacy events.

Given that we do that, is there any point having handling for touch events?
Well, since you wrote the support for touch events, I thought there was some reason ;)
But please test how touch events work. I assume mouse events aren't enough for dragging.
I was under the impression that mouse events don't exist on mobile devices, so I thought I needed to handle touch events in order to have the dragging work there.
Comment on attachment 721243 [details] [diff] [review]
patch

Review of attachment 721243 [details] [diff] [review]:
-----------------------------------------------------------------

I think you could merge test_input_range_mouse_events and test_input_range_touch_events in test_input_range_touch_mouse_events.html

r=me with the comments applied.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +2555,5 @@
>  
> +void
> +nsHTMLInputElement::CancelRangeThumbDrag()
> +{
> +  if (mIsDraggingRange) {

You have some code that sometimes check that variable value before calling that method. I think it would be better to have a "contract" with the method that says "mIsDraggingRange has to be true if you call me". This could be enforced by a NS_ASSERTION or a MOZ_ASSERT at the beginning of the method.

@@ +2566,5 @@
> +void
> +nsHTMLInputElement::SetValueOfRangeForUserEvent(double aValue)
> +{
> +  MOZ_ASSERT(MOZ_DOUBLE_IS_FINITE(aValue));
> +  nsAutoString val;

nit: leave a blank line between those two lines.

@@ +3070,5 @@
> +      nsInputEvent* inputEvent = static_cast<nsInputEvent*>(aVisitor.mEvent);
> +      if (inputEvent->IsShift() || inputEvent->IsControl() ||
> +          inputEvent->IsAlt() || inputEvent->IsMeta() ||
> +          inputEvent->IsAltGraph() || inputEvent->IsFn() ||
> +          inputEvent->IsOS()) {

We really need something like inputEvent->HasModifier()...

@@ +3080,5 @@
> +          mIsDraggingRange = true;
> +          mRangeThumbDragStartValue = GetValueAsDouble();
> +          nsIPresShell::SetCapturingContent(this, CAPTURE_IGNOREALLOWED |
> +                                                  CAPTURE_RETARGETTOELEMENT);
> +          SetValueOfRangeForUserEvent(rangeFrame->GetValueAtEventPoint(mouseEvent));

Shouldn't we have a StartRangeThumbDrag()?

@@ +3082,5 @@
> +          nsIPresShell::SetCapturingContent(this, CAPTURE_IGNOREALLOWED |
> +                                                  CAPTURE_RETARGETTOELEMENT);
> +          SetValueOfRangeForUserEvent(rangeFrame->GetValueAtEventPoint(mouseEvent));
> +        } else {
> +          CancelRangeThumbDrag();

Do we need that? Is it possible that the range drag is started there?

@@ +3093,5 @@
> +          nsIPresShell::SetCapturingContent(this, CAPTURE_IGNOREALLOWED |
> +                                                  CAPTURE_RETARGETTOELEMENT);
> +          SetValueOfRangeForUserEvent(rangeFrame->GetValueAtEventPoint(touchEvent));
> +        } else {
> +          CancelRangeThumbDrag();

See two previous comments.

@@ +3251,5 @@
>    }
>  
> +  if (mIsDraggingRange) {
> +    CancelRangeThumbDrag();
> +  }

Good catch! Is that tested?

Hmmm, maybe we should do that first thing in this method. With a |if (mType == ...TYPE_RANGE)|

::: content/html/content/test/forms/test_input_range_mouse_events.html
@@ +51,5 @@
> +  elem.type = "range";
> +
> +  var content = document.getElementById("content");
> +  content.appendChild(elem);
> +  var flush = document.body.clientWidth;

Why not simply adding the element to the HTML?

@@ +71,5 @@
> +  // Test mouse dragging ltr range:
> +  elem.value = QUARTER_OF_RANGE;
> +  synthesizeMouse(elem, midX, midY, { type: "mousedown" });
> +  synthesizeMouse(elem, minX, midY, { type: "mousemove" });
> +  is(elem.value, MINIMIM_OF_RANGE, "Test dragging of range to left of ltr range");

I guess you could add a test after the 'mousedown' too.

@@ +85,5 @@
> +  elem.style.direction = "rtl";
> +  var flush = document.body.clientWidth;
> +  synthesizeMouse(elem, midX, midY, { type: "mousedown" });
> +  synthesizeMouse(elem, minX, midY, { type: "mousemove" });
> +  is(elem.value, MAXIMIM_OF_RANGE, "Test dragging of range to left of rtl range");

ditto

@@ +100,5 @@
> +  // Test mouse capturing by moving pointer to a position outside the range:
> +  elem.value = QUARTER_OF_RANGE;
> +  synthesizeMouse(elem, midX, midY, { type: "mousedown" });
> +  synthesizeMouse(elem, maxX+100, midY, { type: "mousemove" });
> +  is(elem.value, MAXIMIM_OF_RANGE, "Test dragging of range to position outside range (mousemove)");

ditto

@@ +103,5 @@
> +  synthesizeMouse(elem, maxX+100, midY, { type: "mousemove" });
> +  is(elem.value, MAXIMIM_OF_RANGE, "Test dragging of range to position outside range (mousemove)");
> +
> +  synthesizeMouse(elem, maxX+100, midY, { type: "mouseup" });
> +  is(elem.value, MAXIMIM_OF_RANGE, "Test dragging of range to position outside range (mouseup)");

Can we get the same test for RTL?

@@ +124,5 @@
> +  elem.addEventListener("mousedown", preventDefault, false);
> +  synthesizeMouse(elem, midX, midY, {});
> +  is(elem.value, QUARTER_OF_RANGE, "Test that preventDefault() works");
> +  elem.removeEventListener("mousedown", preventDefault, false);
> +}

What happens if:
 - I click outside of the element and then drag inside it? (I guess nothing)
 - I do a mousedown, move the mouse and then press a key that will change the value (like HOME or END)? (I guess the HOME/END) should apply)
 - ... what will happen if after that, I continue moving the mouse? (I guess the mouse position should then be re-used)
 - ... what will happen if the mouse actually leave the frame before the key is pressed and the 'mouseup' is called after the key is pressed? (I guess the value of the element should be the value set by the key press)
Could you write tests for those?

::: content/html/content/test/forms/test_input_range_touch_events.html
@@ +50,5 @@
> +  var elem = document.createElement("input");
> +  elem.type = "range";
> +
> +  var content = document.getElementById("content");
> +  content.appendChild(elem);

Why not simply adding the element to the HTML?

::: layout/forms/nsRangeFrame.cpp
@@ +337,5 @@
> +  MOZ_ASSERT(aEvent->eventStructType == NS_MOUSE_EVENT ||
> +             aEvent->eventStructType == NS_TOUCH_EVENT,
> +             "Unexpected event type - aEvent->refPoint may be meaningless");
> +
> +  MOZ_ASSERT(mContent->IsHTML(nsGkAtoms::input), "bad cast");

I wouldn't MOZ_ASSERT() here given that we would crash just after anyway.

@@ +348,5 @@
> +  MOZ_ASSERT(MOZ_DOUBLE_IS_FINITE(minimum) &&
> +             MOZ_DOUBLE_IS_FINITE(maximum),
> +             "type=range should have a default maximum/minimum");
> +  if (maximum <= minimum || mRect.width <= 0) {
> +    return minimum;

damnit, another case that we should fix if we disallow max <= min...

@@ +367,5 @@
> +    nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, point, this);
> +  localPoint.x = mozilla::clamped(localPoint.x, 0, mRect.width);
> +  localPoint.y = mozilla::clamped(localPoint.y, 0, mRect.height);
> +  if (StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL) {
> +    return maximum - (double(localPoint.x) / double(mRect.width)) * range;

I would do static_cast<double>() instead of double() because it is more C++-y but you can keep that if you prefer.
Attachment #721243 - Flags: review?(mounir) → review+
(In reply to Mounir Lamouri (:mounir) from comment #21)
> We really need something like inputEvent->HasModifier()...

We have that, but it includes modifiers that we would want to ignore, such as the NumLock key.

> Shouldn't we have a StartRangeThumbDrag()?

Added. Also added a FinishRangeThumbDrag.

> @@ +3082,5 @@
> > +          nsIPresShell::SetCapturingContent(this, CAPTURE_IGNOREALLOWED |
> > +                                                  CAPTURE_RETARGETTOELEMENT);
> > +          SetValueOfRangeForUserEvent(rangeFrame->GetValueAtEventPoint(mouseEvent));
> > +        } else {
> > +          CancelRangeThumbDrag();
> 
> Do we need that? Is it possible that the range drag is started there?

Sure. Specifically, if someone presses one mouse button/touches one finger, then later depresses a second button/touches down a second finger.

> > +  if (mIsDraggingRange) {
> > +    CancelRangeThumbDrag();
> > +  }
> 
> Good catch! Is that tested?

Thanks. Test added.

> What happens if:
>  - I click outside of the element and then drag inside it? (I guess nothing)
>  - I do a mousedown, move the mouse and then press a key that will change
> the value (like HOME or END)? (I guess the HOME/END) should apply)
>  - ... what will happen if after that, I continue moving the mouse? (I guess
> the mouse position should then be re-used)
>  - ... what will happen if the mouse actually leave the frame before the key
> is pressed and the 'mouseup' is called after the key is pressed? (I guess
> the value of the element should be the value set by the key press)
> Could you write tests for those?

I've added these tests, although to me it doesn't seem like the best use of time or tbpl cycles to run them, to be honest. :) (This is pretty edge case stuff that is behavior that might be noticeable to users deliberately doing weird things rather than content authors, and probably isn't something a user is ever likely to notice, or care about.)

> ::: layout/forms/nsRangeFrame.cpp
> @@ +337,5 @@
> > +  MOZ_ASSERT(aEvent->eventStructType == NS_MOUSE_EVENT ||
> > +             aEvent->eventStructType == NS_TOUCH_EVENT,
> > +             "Unexpected event type - aEvent->refPoint may be meaningless");
> > +
> > +  MOZ_ASSERT(mContent->IsHTML(nsGkAtoms::input), "bad cast");
> 
> I wouldn't MOZ_ASSERT() here given that we would crash just after anyway.

Crashing on a bad cast is not guaranteed at all, which is exactly why we should assert here to avoid invoking code on bad memory! I've left this as-is.

> @@ +367,5 @@
> > +    nsLayoutUtils::GetEventCoordinatesRelativeTo(aEvent, point, this);
> > +  localPoint.x = mozilla::clamped(localPoint.x, 0, mRect.width);
> > +  localPoint.y = mozilla::clamped(localPoint.y, 0, mRect.height);
> > +  if (StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL) {
> > +    return maximum - (double(localPoint.x) / double(mRect.width)) * range;
> 
> I would do static_cast<double>() instead of double() because it is more
> C++-y but you can keep that if you prefer.

We generally use the constructor cast for double in my experience in layout land, so I'll opt to leave that as-is (it's easier to read).
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/65c8f4a18992

After working out some weird failures on Android.

(In reply to Jonathan Watt [:jwatt] from comment #22)
> > What happens if:
> >  - I click outside of the element and then drag inside it? (I guess nothing)
> >  - I do a mousedown, move the mouse and then press a key that will change
> > the value (like HOME or END)? (I guess the HOME/END) should apply)
> >  - ... what will happen if after that, I continue moving the mouse? (I guess
> > the mouse position should then be re-used)
> >  - ... what will happen if the mouse actually leave the frame before the key
> > is pressed and the 'mouseup' is called after the key is pressed? (I guess
> > the value of the element should be the value set by the key press)
> > Could you write tests for those?
> 
> I've added these tests, although to me it doesn't seem like the best use of
> time or tbpl cycles to run them, to be honest. :) (This is pretty edge case
> stuff that is behavior that might be noticeable to users deliberately doing
> weird things rather than content authors, and probably isn't something a
> user is ever likely to notice, or care about.)

The test for the HOME key fails on Android due to the keypress event being ignored for some reason. I spent way longer than I should have (given that this it's such an unimportant edge case) trying to debug why, but I can't justify spending any more time on it. I've disabled it for now.
(In reply to Jonathan Watt [:jwatt] from comment #22)
> (In reply to Mounir Lamouri (:mounir) from comment #21)
> > What happens if:
> >  - I click outside of the element and then drag inside it? (I guess nothing)
> >  - I do a mousedown, move the mouse and then press a key that will change
> > the value (like HOME or END)? (I guess the HOME/END) should apply)
> >  - ... what will happen if after that, I continue moving the mouse? (I guess
> > the mouse position should then be re-used)
> >  - ... what will happen if the mouse actually leave the frame before the key
> > is pressed and the 'mouseup' is called after the key is pressed? (I guess
> > the value of the element should be the value set by the key press)
> > Could you write tests for those?
> 
> I've added these tests, although to me it doesn't seem like the best use of
> time or tbpl cycles to run them, to be honest. :) (This is pretty edge case
> stuff that is behavior that might be noticeable to users deliberately doing
> weird things rather than content authors, and probably isn't something a
> user is ever likely to notice, or care about.)

TBH, I do not think that TBPL cycles is a good counter-argument. Spending too much time testing highly edge-casy cases is indeed something that could be avoid. I guess it's a matter of how much failing the edge case would be bad compared to how much the test is hard to write. I tend to think that we should test everything as long as we can find a reasonably easy way to test it.

(In reply to Jonathan Watt [:jwatt] from comment #23)
> The test for the HOME key fails on Android due to the keypress event being
> ignored for some reason. I spent way longer than I should have (given that
> this it's such an unimportant edge case) trying to debug why, but I can't
> justify spending any more time on it. I've disabled it for now.

Why not disabling the test on Android?
https://hg.mozilla.org/mozilla-central/rev/65c8f4a18992
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.