Bug 1209035. Fix incorrect "is this control focused?" checks in form code. r=smaug
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 27 Jun 2018 12:04:26 -0400
changeset 481750 9451d5fcfaa48131cac9e1039484aef9d303c06f
parent 481688 5c07dcfb636869b36b911473c6611a7ad71f3134
child 481751 0ee5b54dab6c03ce716214462543b8479c41dfaf
push id1815
push userffxbld-merge
push dateMon, 15 Oct 2018 10:40:45 +0000
treeherdermozilla-release@18d4c09e9378 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1209035
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1209035. Fix incorrect "is this control focused?" checks in form code. r=smaug For number controls, nsContentUtils::IsFocusedContent doesn't really do the right thing, because the thing it thinks is focused is the anonymous text element inside the number control. As a result, we weren't properly updating the state of the currently-focused number control when hitting enter in it to submit the form. The HTMLFormElement change is enough on its own to fix the bug. The constraint validation change is a just-in-case. I haven't figured out a sane way to write a reftest for this, unfortunately: the enter key press needs to look like a real user event to trigger the submission behavior.
dom/html/HTMLFormElement.cpp
dom/html/HTMLInputElement.cpp
dom/html/nsIConstraintValidation.cpp
--- a/dom/html/HTMLFormElement.cpp
+++ b/dom/html/HTMLFormElement.cpp
@@ -1948,17 +1948,22 @@ HTMLFormElement::CheckValidFormSubmissio
 
         nsAutoScriptBlocker scriptBlocker;
 
         for (uint32_t i = 0, length = mControls->mElements.Length();
              i < length; ++i) {
           // Input elements can trigger a form submission and we want to
           // update the style in that case.
           if (mControls->mElements[i]->IsHTMLElement(nsGkAtoms::input) &&
-              nsContentUtils::IsFocusedContent(mControls->mElements[i])) {
+              // We don't use nsContentUtils::IsFocusedContent here, because it
+              // doesn't really do what we want for number controls: it's true
+              // for the anonymous textnode inside, but not the number control
+              // itself.  We can use the focus state, though, because that gets
+              // synced to the number control by the anonymous text control.
+              mControls->mElements[i]->State().HasState(NS_EVENT_STATE_FOCUS)) {
             static_cast<HTMLInputElement*>(mControls->mElements[i])
               ->UpdateValidityUIBits(true);
           }
 
           mControls->mElements[i]->UpdateState(true);
         }
 
         // Because of backward compatibility, <input type='image'> is not in
--- a/dom/html/HTMLInputElement.cpp
+++ b/dom/html/HTMLInputElement.cpp
@@ -6430,32 +6430,34 @@ void
 HTMLInputElement::AddStates(EventStates aStates)
 {
   if (mType == NS_FORM_INPUT_TEXT) {
     EventStates focusStates(aStates & (NS_EVENT_STATE_FOCUS |
                                        NS_EVENT_STATE_FOCUSRING));
     if (!focusStates.IsEmpty()) {
       HTMLInputElement* ownerNumberControl = GetOwnerNumberControl();
       if (ownerNumberControl) {
+        // If this code changes, audit existing places that check for NS_EVENT_STATE_FOCUS.
         ownerNumberControl->AddStates(focusStates);
       }
     }
   }
   nsGenericHTMLFormElementWithState::AddStates(aStates);
 }
 
 void
 HTMLInputElement::RemoveStates(EventStates aStates)
 {
   if (mType == NS_FORM_INPUT_TEXT) {
     EventStates focusStates(aStates & (NS_EVENT_STATE_FOCUS |
                                        NS_EVENT_STATE_FOCUSRING));
     if (!focusStates.IsEmpty()) {
       HTMLInputElement* ownerNumberControl = GetOwnerNumberControl();
       if (ownerNumberControl) {
+        // If this code changes, audit existing places that check for NS_EVENT_STATE_FOCUS.
         ownerNumberControl->RemoveStates(focusStates);
       }
     }
   }
   nsGenericHTMLFormElementWithState::RemoveStates(aStates);
 }
 
 static nsTArray<OwningFileOrDirectory>
--- a/dom/html/nsIConstraintValidation.cpp
+++ b/dom/html/nsIConstraintValidation.cpp
@@ -127,21 +127,21 @@ nsIConstraintValidation::CheckValidity(b
 
 bool
 nsIConstraintValidation::ReportValidity()
 {
   if (!IsCandidateForConstraintValidation() || IsValid()) {
     return true;
   }
 
-  nsCOMPtr<nsIContent> content = do_QueryInterface(this);
-  MOZ_ASSERT(content, "This class should be inherited by HTML elements only!");
+  nsCOMPtr<Element> element = do_QueryInterface(this);
+  MOZ_ASSERT(element, "This class should be inherited by HTML elements only!");
 
   bool defaultAction = true;
-  nsContentUtils::DispatchTrustedEvent(content->OwnerDoc(), content,
+  nsContentUtils::DispatchTrustedEvent(element->OwnerDoc(), element,
                                        NS_LITERAL_STRING("invalid"),
                                        CanBubble::eNo,
                                        Cancelable::eYes,
                                        &defaultAction);
   if (!defaultAction) {
     return false;
   }
 
@@ -159,38 +159,42 @@ nsIConstraintValidation::ReportValidity(
   // Return true on error here because that's what we always did
   NS_ENSURE_SUCCESS(rv, true);
 
   bool hasObserver = false;
   rv = theEnum->HasMoreElements(&hasObserver);
 
   nsCOMPtr<nsIMutableArray> invalidElements =
     do_CreateInstance(NS_ARRAY_CONTRACTID, &rv);
-  invalidElements->AppendElement(content);
+  invalidElements->AppendElement(element);
 
   NS_ENSURE_SUCCESS(rv, true);
   nsCOMPtr<nsISupports> inst;
   nsCOMPtr<nsIFormSubmitObserver> observer;
   bool more = true;
   while (NS_SUCCEEDED(theEnum->HasMoreElements(&more)) && more) {
     theEnum->GetNext(getter_AddRefs(inst));
     observer = do_QueryInterface(inst);
 
     if (observer) {
       observer->NotifyInvalidSubmit(nullptr, invalidElements);
     }
   }
 
-  if (content->IsHTMLElement(nsGkAtoms::input) &&
-      nsContentUtils::IsFocusedContent(content)) {
-    HTMLInputElement* inputElement = HTMLInputElement::FromNode(content);
+  if (element->IsHTMLElement(nsGkAtoms::input) &&
+      // We don't use nsContentUtils::IsFocusedContent here, because it doesn't
+      // really do what we want for number controls: it's true for the
+      // anonymous textnode inside, but not the number control itself.  We can
+      // use the focus state, though, because that gets synced to the number
+      // control by the anonymous text control.
+      element->State().HasState(NS_EVENT_STATE_FOCUS)) {
+    HTMLInputElement* inputElement = HTMLInputElement::FromNode(element);
     inputElement->UpdateValidityUIBits(true);
   }
 
-  dom::Element* element = content->AsElement();
   element->UpdateState(true);
   return false;
 }
 
 void
 nsIConstraintValidation::SetValidityState(ValidityStateType aState,
                                           bool aValue)
 {