Fixes Issue 3508 Clicking on a CheckBox's label triggers two click events
CheckBoxes were sending two click events when their labels were clicked, due to
trickery going on to send ValueChangeEvents only when appropriate.
I've moved all the trickery down to RadioButton, the only place it is actually
needed, and filter out click events on the label.
Testing: CheckBoxTest has been extended for the specific problem, and I've
manually tested keyboard and mouse interaction with both CheckBox and
RadioButton on Safari, FF3, IE and Chrome. Full suite run against IE7.
git-svn-id: https://google-web-toolkit.googlecode.com/svn/trunk@5333 8db76d5a-ed1c-0410-87a9-c151d255dfc7
diff --git a/user/src/com/google/gwt/user/client/ui/CheckBox.java b/user/src/com/google/gwt/user/client/ui/CheckBox.java
index 4251521..d29f8c6 100644
--- a/user/src/com/google/gwt/user/client/ui/CheckBox.java
+++ b/user/src/com/google/gwt/user/client/ui/CheckBox.java
@@ -20,10 +20,6 @@
import com.google.gwt.dom.client.LabelElement;
import com.google.gwt.event.dom.client.ClickEvent;
import com.google.gwt.event.dom.client.ClickHandler;
-import com.google.gwt.event.dom.client.KeyUpEvent;
-import com.google.gwt.event.dom.client.KeyUpHandler;
-import com.google.gwt.event.dom.client.MouseUpEvent;
-import com.google.gwt.event.dom.client.MouseUpHandler;
import com.google.gwt.event.logical.shared.ValueChangeEvent;
import com.google.gwt.event.logical.shared.ValueChangeHandler;
import com.google.gwt.event.shared.HandlerRegistration;
@@ -56,10 +52,9 @@
* </p>
*/
public class CheckBox extends ButtonBase implements HasName, HasValue<Boolean> {
- private InputElement inputElem;
- private LabelElement labelElem;
+ InputElement inputElem;
+ LabelElement labelElem;
private boolean valueChangeHandlerInitialized;
- private boolean valueBeforeClick;
/**
* Creates a check box with no label.
@@ -118,31 +113,23 @@
ValueChangeHandler<Boolean> handler) {
// Is this the first value change handler? If so, time to add handlers
if (!valueChangeHandlerInitialized) {
-
- this.addKeyUpHandler(new KeyUpHandler() {
- public void onKeyUp(KeyUpEvent event) {
- valueBeforeClick = getValue();
- }
- });
-
- this.addMouseUpHandler(new MouseUpHandler() {
- public void onMouseUp(MouseUpEvent event) {
- valueBeforeClick = getValue();
- }
- });
-
- this.addClickHandler(new ClickHandler() {
- public void onClick(ClickEvent event) {
- ValueChangeEvent.fireIfNotEqual(CheckBox.this, valueBeforeClick,
- getValue());
- }
- });
-
+ ensureDomEventHandlers();
valueChangeHandlerInitialized = true;
}
return addHandler(handler, ValueChangeEvent.getType());
}
+ protected void ensureDomEventHandlers() {
+ addClickHandler(new ClickHandler() {
+ public void onClick(ClickEvent event) {
+ // Checkboxes always toggle their value, no need to compare
+ // with old value. Radio buttons are not so lucky, see
+ // overrides in RadioButton
+ ValueChangeEvent.fire(CheckBox.this, getValue());
+ }
+ });
+ }
+
/**
* Returns the value property of the input element that backs this widget.
* This is the value that will be associated with the CheckBox name and
@@ -329,15 +316,13 @@
}
}
- // Unlike other widgets the CheckBox sinks on its constituent elements, not
- // their wrapper element.
+ // Unlike other widgets the CheckBox sinks on its inputElement, not
+ // its wrapper
@Override
public void sinkEvents(int eventBitsToAdd) {
if (isOrWasAttached()) {
Event.sinkEvents(inputElem,
eventBitsToAdd | Event.getEventsSunk(inputElem));
- Event.sinkEvents(labelElem,
- eventBitsToAdd | Event.getEventsSunk(labelElem));
} else {
super.sinkEvents(eventBitsToAdd);
}
diff --git a/user/src/com/google/gwt/user/client/ui/RadioButton.java b/user/src/com/google/gwt/user/client/ui/RadioButton.java
index a51ccb9c..a0dedc1 100644
--- a/user/src/com/google/gwt/user/client/ui/RadioButton.java
+++ b/user/src/com/google/gwt/user/client/ui/RadioButton.java
@@ -15,19 +15,32 @@
*/
package com.google.gwt.user.client.ui;
+import com.google.gwt.dom.client.Element;
+import com.google.gwt.dom.client.EventTarget;
+import com.google.gwt.event.dom.client.BlurEvent;
+import com.google.gwt.event.dom.client.ClickEvent;
+import com.google.gwt.event.dom.client.KeyDownEvent;
+import com.google.gwt.event.dom.client.MouseUpEvent;
+import com.google.gwt.event.logical.shared.ValueChangeEvent;
import com.google.gwt.user.client.DOM;
+import com.google.gwt.user.client.Event;
/**
- * A mutually-exclusive selection radio button widget.
+ * A mutually-exclusive selection radio button widget. Fires {@link ClickEvent}s
+ * when the radio button is clicked, and {@link ValueChangeEvent}s when the
+ * button becomes checked. Note, however, that browser limitations prevent
+ * ValueChangeEvents from being sent when the radio button is cleared as a side
+ * effect of another in the group being clicked.
*
* <p>
* <img class='gallery' src='RadioButton.png'/>
* </p>
*
- * <h3>CSS Style Rules</h3>
- * <ul class='css'>
- * <li>.gwt-RadioButton { }</li>
- * </ul>
+ * <h3>CSS Style Rules</h3>
+ * <dl>
+ * <dt>.gwt-RadioButton</dt>
+ * <dd>the outer element</dd>
+ * </dl>
*
* <p>
* <h3>Example</h3> {@example com.google.gwt.examples.RadioButtonExample}
@@ -35,20 +48,84 @@
*/
public class RadioButton extends CheckBox {
+ private Boolean oldValue;
+
/**
* Creates a new radio associated with a particular group name. All radio
* buttons associated with the same group name belong to a mutually-exclusive
* set.
*
- * Radio buttons are grouped by their name attribute, so changing their
- * name using the setName() method will also change their associated
- * group.
+ * Radio buttons are grouped by their name attribute, so changing their name
+ * using the setName() method will also change their associated group.
*
* @param name the group name with which to associate the radio button
*/
public RadioButton(String name) {
super(DOM.createInputRadio(name));
setStyleName("gwt-RadioButton");
+
+ sinkEvents(Event.getTypeInt(MouseUpEvent.getType().getName()));
+ sinkEvents(Event.getTypeInt(BlurEvent.getType().getName()));
+ sinkEvents(Event.getTypeInt(KeyDownEvent.getType().getName()));
+ }
+
+ /**
+ * Overridden to send ValueChangeEvents only when appropriate.
+ */
+ @Override
+ public void onBrowserEvent(Event event) {
+ switch (DOM.eventGetType(event)) {
+ case Event.ONMOUSEUP:
+ case Event.ONBLUR:
+ case Event.ONKEYDOWN:
+ // Note the old value for onValueChange purposes (in ONCLICK case)
+ oldValue = getValue();
+ break;
+
+ case Event.ONCLICK:
+ EventTarget target = event.getEventTarget();
+ if (Element.is(target) && labelElem.isOrHasChild(Element.as(target))) {
+
+ // They clicked the label. Note our pre-click value, and
+ // short circuit event routing so that other click handlers
+ // don't hear about it
+ oldValue = getValue();
+ return;
+ }
+
+ // It's not the label. Let our handlers hear about the
+ // click...
+ super.onBrowserEvent(event);
+ // ...and now maybe tell them about the change
+ ValueChangeEvent.fireIfNotEqual(RadioButton.this, oldValue, getValue());
+ return;
+ }
+
+ super.onBrowserEvent(event);
+ }
+
+ @Override
+ public void sinkEvents(int eventBitsToAdd) {
+ // Like CheckBox, we want to hear about inputElem. We
+ // also want to know what's going on with the label, to
+ // make sure onBrowserEvent is able to record value changes
+ // initiated by label events
+ if (isOrWasAttached()) {
+ Event.sinkEvents(inputElem, eventBitsToAdd
+ | Event.getEventsSunk(inputElem));
+ Event.sinkEvents(labelElem, eventBitsToAdd
+ | Event.getEventsSunk(labelElem));
+ } else {
+ super.sinkEvents(eventBitsToAdd);
+ }
+ }
+
+ /**
+ * No-op. CheckBox's click handler is no good for radio button, so don't use
+ * it. Our event handling is all done in {@link #onBrowserEvent}
+ */
+ @Override
+ protected void ensureDomEventHandlers() {
}
/**
@@ -56,9 +133,8 @@
* with the given HTML label. All radio buttons associated with the same group
* name belong to a mutually-exclusive set.
*
- * Radio buttons are grouped by their name attribute, so changing their
- * name using the setName() method will also change their associated
- * group.
+ * Radio buttons are grouped by their name attribute, so changing their name
+ * using the setName() method will also change their associated group.
*
* @param name the group name with which to associate the radio button
* @param label this radio button's label
@@ -74,9 +150,8 @@
* buttons associated with the same group name belong to a mutually-exclusive
* set.
*
- * Radio buttons are grouped by their name attribute, so changing their
- * name using the setName() method will also change their associated
- * group.
+ * Radio buttons are grouped by their name attribute, so changing their name
+ * using the setName() method will also change their associated group.
*
* @param name name the group with which to associate the radio button
* @param label this radio button's label
@@ -94,19 +169,18 @@
/**
* Change the group name of this radio button.
*
- * Radio buttons are grouped by their name attribute, so changing their
- * name using the setName() method will also change their associated
- * group.
+ * Radio buttons are grouped by their name attribute, so changing their name
+ * using the setName() method will also change their associated group.
*
- * If changing this group name results in a new radio group with
- * multiple radio buttons selected, this radio button will remain
- * selected and the other radio buttons will be unselected.
- *
+ * If changing this group name results in a new radio group with multiple
+ * radio buttons selected, this radio button will remain selected and the
+ * other radio buttons will be unselected.
+ *
* @param name name the group with which to associate the radio button
*/
@Override
public void setName(String name) {
- // Just changing the radio button name tends to break groupiness,
+ // Just changing the radio button name tends to break groupiness,
// so we have to replace it. Note that replaceInputElement is careful
// not to propagate name when it propagates everything else
super.replaceInputElement(DOM.createInputRadio(name));
diff --git a/user/test/com/google/gwt/user/client/ui/CheckBoxTest.java b/user/test/com/google/gwt/user/client/ui/CheckBoxTest.java
index 2a4ed97..993a9e0 100644
--- a/user/test/com/google/gwt/user/client/ui/CheckBoxTest.java
+++ b/user/test/com/google/gwt/user/client/ui/CheckBoxTest.java
@@ -17,7 +17,9 @@
import com.google.gwt.dom.client.Document;
import com.google.gwt.dom.client.InputElement;
+import com.google.gwt.dom.client.NativeEvent;
import com.google.gwt.event.dom.client.ClickEvent;
+import com.google.gwt.event.dom.client.ClickHandler;
import com.google.gwt.event.logical.shared.ValueChangeEvent;
import com.google.gwt.event.logical.shared.ValueChangeHandler;
import com.google.gwt.event.shared.HandlerManager;
@@ -29,8 +31,7 @@
* Tests the CheckBox Widget.
*/
public class CheckBoxTest extends GWTTestCase {
-
- @SuppressWarnings("deprecation")
+ @SuppressWarnings("deprecation")
static class ListenerTester implements ClickListener {
static int fired = 0;
static HandlerManager manager;
@@ -44,8 +45,9 @@
public void onClick(Widget sender) {
++fired;
}
- }
+ }
+
private static class Handler implements ValueChangeHandler<Boolean> {
Boolean received = null;
@@ -166,6 +168,53 @@
assertEquals(ListenerTester.fired, 0);
}
+ public void testCheckboxClick() {
+ final int[] clickCount = {0};
+
+ CheckBox check = new CheckBox();
+ Element newInput = DOM.createInputCheck();
+ check.replaceInputElement(newInput);
+
+ check.setText("Burger");
+ check.addClickHandler(new ClickHandler() {
+ public void onClick(ClickEvent arg0) {
+ clickCount[0]++;
+ }
+ });
+ RootPanel.get().add(check);
+
+ NativeEvent e = Document.get().createClickEvent(0, 25, 25, 25, 25, false,
+ false, false, false);
+
+ newInput.dispatchEvent(e);
+ assertEquals(1, clickCount[0]);
+ }
+
+ public void testLabelClick() {
+ final int[] clickCount = {0};
+
+ CheckBox check = new CheckBox();
+ Element newInput = DOM.createInputCheck();
+ check.replaceInputElement(newInput);
+
+ check.setText("Burger");
+ check.addClickHandler(new ClickHandler() {
+ public void onClick(ClickEvent arg0) {
+ clickCount[0]++;
+ }
+ });
+ RootPanel.get().add(check);
+
+ NativeEvent e = Document.get().createClickEvent(0, 25, 25, 25, 25, false,
+ false, false, false);
+
+ // click the label, which is next to the checkbox
+ // http://code.google.com/p/google-web-toolkit/issues/detail?id=3508
+
+ newInput.getNextSiblingElement().dispatchEvent(e);
+ assertEquals(1, clickCount[0]);
+ }
+
public void testReplaceInputElement() {
cb.setValue(true);
cb.setTabIndex(1234);
@@ -192,8 +241,6 @@
elm.setChecked(true);
assertTrue(cb.getValue());
-
- // TODO: When event creation is in, test that click on the new element works
}
@SuppressWarnings("deprecation")
@@ -225,6 +272,11 @@
} catch (IllegalArgumentException e) {
/* pass */
}
+
+ // Note that we cannot test this with a simulated click, the way
+ // we do for the click handlers. IE does not change the value of
+ // the native checkbox on simulated click event, and there's
+ // naught to be done about it.
}
@Override
