I just noticed that <input type=range> is not currently firing "change"/"input" events. That needs to be fixed for release.

Actually we can't just assert that nsContentUtils::IsCallerChrome(), since there is one rare case where that will fail (see the comment I added).

Comment on attachment 724929 [details] [diff] [review] patch Review of attachment 724929 [details] [diff] [review]: ----------------------------------------------------------------- You do not seem to update the code in ::HandleTypeChange(), ::SetValue(), ::PostHandleEvent(), ::PreHandleEvent(). Basically, look at all FireChangeIfNeeded() calls and mFocusedValue calls. ::: content/html/content/src/nsHTMLInputElement.cpp @@ +2608,5 @@ > + if (!nsContentUtils::IsCallerChrome()) { > + // This can happen when script does input.type=non-range during a thumb > + // drag, causing CancelRangeThumbDrag() to call us. > + return; > + } I am not really sure I do understand what is happening here. I shall have a deeper look.

Why do I need to look at the places that reference mFocusedValue? (In reply to Mounir Lamouri (:mounir) from comment #3) > You do not seem to update the code in ::HandleTypeChange() There isn't any FireChangeEventIfNeeded() call there. > ::SetValue(), or here > ::PostHandleEvent(), This calls SetValueOfRangeForUserEvent, and I've added a FireChangeEventIfNeeded() call to that method. > ::PreHandleEvent(). The call there should work fine for range as-is, no? Or do you want me to add a check for range with the IsExperimentalMobileType() check? > I am not really sure I do understand what is happening here. I shall have a > deeper look. If you have it assert instead of return, test_input_range_mouse_and_touch_events.html will trigger the assert.

(In reply to Jonathan Watt [:jwatt] from comment #4) > Why do I need to look at the places that reference mFocusedValue? Ah, okay, I understand; we sometimes set mFocusedValue to the current value to deliberately prevent the change event from being dispatched. The following comments still stand: > > ::PostHandleEvent(), > > This calls SetValueOfRangeForUserEvent, and I've added a > FireChangeEventIfNeeded() call to that method. > > > ::PreHandleEvent(). > > The call there should work fine for range as-is, no? Or do you want me to > add a check for range with the IsExperimentalMobileType() check? > > > I am not really sure I do understand what is happening here. I shall have a > > deeper look. > > If you have it assert instead of return, > test_input_range_mouse_and_touch_events.html will trigger the assert. Or to give you the stack when script does type=range -> type=something-else: nsHTMLInputElement::SetValueOfRangeForUserEvent nsHTMLInputElement::CancelRangeThumbDrag nsHTMLInputElement::HandleTypeChange nsHTMLInputElement::ParseAttribute mozilla::dom::Element::SetAttr nsGenericHTMLElement::SetAttr nsGenericHTMLElement::SetAttr nsGenericHTMLElement::SetAttrHelper nsHTMLInputElement::SetType nsIDOMHTMLInputElement_SetType

Comment on attachment 725213 [details] [diff] [review] patch Review of attachment 725213 [details] [diff] [review]: ----------------------------------------------------------------- So, I think we should change an input event whenever the value is changed but we shouldn't follow the behaviour of Webkit and IE10 regarding the change event. I think firing a change event when we blur is the way to go. We could also fire a change event when a drag is finished given that it can be considered somehow as a 'commit' [1]. Regarding the tests, I guess you could add some specific tests in test_change_event.html for the <input type='range'>. You would have to handle the blur and drag scenarios. Do we have tests for the input events? I think you could do a helper method like: bool DoesFireChangeOnBlur() const { return IsSingleLineTextControl(false) || mType == NS_FORM_INPUT_RANGE; } [1] http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#event-input-change ::: content/html/content/src/nsHTMLInputElement.cpp @@ +2605,5 @@ > frame->UpdateThumbPositionForValueChange(); > } > + if (!nsContentUtils::IsCallerChrome()) { > + // This can happen when script does input.type=non-range during a thumb > + // drag, causing CancelRangeThumbDrag() to call us. So, I think we shouldn't do that here. IsCallerChrome() works in most situations but wouldn't work if the type change is made by a chrome privileged caller like Firefox UI. I would prefer to not handle this edge case here and open a follow-up (blocking the feature shipping) to fix it properly. @@ +2612,5 @@ > + nsContentUtils::DispatchTrustedEvent(OwnerDoc(), > + static_cast<nsIDOMHTMLInputElement*>(this), > + NS_LITERAL_STRING("input"), true, > + true); > + FireChangeEventIfNeeded(); I guess that would have to be done elsewhere (like FinishRangeThumbDrag()). @@ +3316,5 @@ > // Updating mFocusedValue in consequence: > // If the new type is a single line text control but the previous wasn't, we > // should set mFocusedValue to the current value. > // Otherwise, if the new type isn't a text control but the previous was, we > // should clear out mFocusedValue. Can you update the comment? ::: content/html/content/test/forms/test_change_event.html @@ +155,5 @@ > else { > input = document.getElementById("input_" + NonTextInputTypes[i]); > input.focus(); > + if (input.type == "range") { > + synthesizeKey("VK_HOME", {}); I think we should not fire change event in that case but only when we blur the element after changing its value with the keyboard.

(In reply to Mounir Lamouri (:mounir) from comment #6) > So, I think we should change an input event whenever the value is changed > but we shouldn't follow the behaviour of Webkit and IE10 regarding the > change event. Okay, but, as noted on IRC, we'll not work with the demos that I've seen. We can try "fixing the web though". :) > Regarding the tests, I guess you could add some specific tests in > test_change_event.html for the <input type='range'>. You would have to > handle the blur and drag scenarios. Do we have tests for the input events? I added "input" events too, although I don't see any such tests for other input types.

Comment on attachment 725531 [details] [diff] [review] patch Review of attachment 725531 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments applied an the follow-up filed. Also, I would have preferred to not test the input event behaviour in test_change_event.html. It would be great if you could create a test_input_event.html test. Otherwise, at least file a follow-up for that to be done. ::: content/html/content/src/nsHTMLInputElement.cpp @@ +2606,5 @@ > } > + nsContentUtils::DispatchTrustedEvent(OwnerDoc(), > + static_cast<nsIDOMHTMLInputElement*>(this), > + NS_LITERAL_STRING("input"), true, > + true); The event should not be cancellable per HTML spec. Make the second boolean 'false'. @@ +3310,5 @@ > // Updating mFocusedValue in consequence: > + // If the new type is a single line text control or range, but the previous > + // wasn't, we should set mFocusedValue to the current value. > + // Otherwise, if the new type isn't a text control or range, but the previous > + // was, we should clear out mFocusedValue. I would not say "single line text control or range" but "firing change event on blur" just so we hide the implementation of the method. ::: content/html/content/src/nsHTMLInputElement.h @@ +819,5 @@ > bool mIsDraggingRange : 1; > > private: > + > + bool DoesFireChangeOnBlur(uint8_t aType) const { Make it static. @@ +826,5 @@ > + } > + > + bool DoesFireChangeOnBlur() const { > + return IsSingleLineTextControl(false, mType) || > + mType == NS_FORM_INPUT_RANGE; return DoesFireChangeOnBlur(mType);

https://hg.mozilla.org/integration/mozilla-inbound/rev/46585b03426a (In reply to Mounir Lamouri (:mounir) from comment #8) > Also, I would have preferred to not test the input event behaviour in > test_change_event.html. It would be great if you could create a > test_input_event.html test. Otherwise, at least file a follow-up for that to > be done. I'll file a follow-up, since I'd like to land this but don't for now have time to create a separate test that will cover all the things you can dream up for a standalone test and meet your exacting standards. ;) I also think it probably makes sense to rename the test to test_change_and_input_events.html and integrate "input" events for all types there, but don't want to hold things up as we discuss that. > @@ +826,5 @@ > > + } > > + > > + bool DoesFireChangeOnBlur() const { > > + return IsSingleLineTextControl(false, mType) || > > + mType == NS_FORM_INPUT_RANGE; > > return DoesFireChangeOnBlur(mType); Huh. I meant to do that. Thanks. :) I also renamed DoesFireChangeOnBlur to MayFireChangeOnBlur as discussed on IRC.

Thanks for fixing this and working on <input type="range">, Jonathan! :) (In reply to Jonathan Watt [:jwatt] from comment #7) > (In reply to Mounir Lamouri (:mounir) from comment #6) > > So, I think we should change an input event whenever the value is changed > > but we shouldn't follow the behaviour of Webkit and IE10 regarding the > > change event. > > Okay, but, as noted on IRC, we'll not work with the demos that I've seen. We > can try "fixing the web though". :) Yeah, this is rather unfortunate. The spec is unfortunately ambiguous enough to allow for both interpretations here. It is difficult to dictate demo assumptions when we're playing catchup. On the bright side, we will make at least a few developers happy: "Currently the onChange event on my range inputs is firing at each step. Is there a way to stop this event from firing until the user has let go of the slider?" - Randy Marsh from http://stackoverflow.com/questions/5165579/onchange-event-for-html5-range

That's good to hear. :)

(In reply to Mounir Lamouri (:mounir) from comment #6) > So, I think we shouldn't do that here. IsCallerChrome() works in most > situations but wouldn't work if the type change is made by a chrome > privileged caller like Firefox UI. > I would prefer to not handle this edge case here and open a follow-up > (blocking the feature shipping) to fix it properly. Filed bug 851782. (In reply to Mounir Lamouri (:mounir) from comment #8) > Also, I would have preferred to not test the input event behaviour in > test_change_event.html. It would be great if you could create a > test_input_event.html test. Otherwise, at least file a follow-up for that to > be done. Filed bug 851780.

https://hg.mozilla.org/integration/mozilla-inbound/rev/46585b03426a https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca17fc803e4

Backed out for hitting a different assert than the one I mentioned, but at the same two points running test_input_range_mouse_and_touch_events.html as mentioned in comment 4 and comment 5. Stack is: ###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file ../../../../content/events/src/nsEventDispatcher.cpp, line 494 nsEventDispatcher::Dispatch nsEventDispatcher::DispatchDOMEvent nsINode::DispatchEvent nsContentUtils::DispatchEvent nsContentUtils::DispatchTrustedEvent nsHTMLInputElement::SetValueOfRangeForUserEvent nsHTMLInputElement::CancelRangeThumbDrag nsHTMLInputElement::HandleTypeChange nsHTMLInputElement::ParseAttribute mozilla::dom::Element::SetAttr nsGenericHTMLElement::SetAttr nsGenericHTMLElement::SetAttr nsGenericHTMLElement::SetAttrHelper nsHTMLInputElement::SetType nsIDOMHTMLInputElement_SetType So basically the issue I filed bug 851782 for, but for some reason this other assertion doesn't trigger on Mac so I didn't notice it.

Repushed with a temporary fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/3648f5fa5787 What we actually do long term will still be determined in bug 851782.

dev-doc-needed was already set on bug 344618, but I'd like to set it here to ensure that we cover the point about onchange events being fired at different points than in some other browsers. We'll likely keep getting bugs like bug 853670 though.