[gwt-contrib] Re: Introduces BrowserEvents string constants (issue1550803)
LGTM http://gwt-code-reviews.appspot.com/1550803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduces BrowserEvents string constants (issue1550803)
PTAL
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/Document.java
File user/src/com/google/gwt/dom/client/Document.java (right):
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/Document.java#newcode18
user/src/com/google/gwt/dom/client/Document.java:18: import static
com.google.gwt.dom.client.BrowserEvents.*;
We have an auto-importer setting against it. Fixed here and in
ClickableTextCell.
On 2011/09/19 14:26:41, jlabanca wrote:
Do we have a rule against importing .*?
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/Document.java#newcode571
user/src/com/google/gwt/dom/client/Document.java:571:
@SuppressWarnings("deprecation")
Eclipse seems to think so. But it also seems not to notice the
@SuppressWarnings and keep nagging. Reverted.
On 2011/09/19 14:26:41, jlabanca wrote:
Do you need to suppress deprecation on a deprecated method?
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java
File
user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java
(right):
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java#newcode162
user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java:162:
if (BrowserEvents.FOCUSIN.equals(type)) {
I think Thomas wins here.
On 2011/09/19 14:26:41, jlabanca wrote:
FOCUSIN is IE specific. I would leave the string in this case, or
create a
static private String in this impl class.
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java#newcode175
user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java:175:
} else if ("focusout".equals(type)) {
And adding this to BrowserEvents
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java
File user/src/com/google/gwt/user/client/impl/DOMImplStandard.java
(right):
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java#newcode57
user/src/com/google/gwt/user/client/impl/DOMImplStandard.java:57: if
(evt.getType().equals(BrowserEvents.MOUSEOUT)) {
Thanks!
On 2011/09/19 14:26:41, jlabanca wrote:
MOUSEOUT/MOUSEOVER
http://gwt-code-reviews.appspot.com/1550803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduces BrowserEvents string constants (issue1550803)
http://gwt-code-reviews.appspot.com/1550803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduces BrowserEvents string constants (issue1550803)
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/BrowserEvents.java File user/src/com/google/gwt/dom/client/BrowserEvents.java (right): http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/BrowserEvents.java#newcode39 user/src/com/google/gwt/dom/client/BrowserEvents.java:39: public static final String FOCUSIN = "focusin"; On 2011/09/19 14:26:41, jlabanca wrote: focusin is not part of any standard Actually, they're being standardized in DOM3-Events: http://www.w3.org/TR/DOM-Level-3-Events/#event-type-focusIn and is only used in IE. According to PPK, all browsers but Firefox fires them (but WebKit doesn't support the onfocusin and onfocusout event handlers): http://www.quirksmode.org/dom/events/blurfocus.html#t06 The problem is: they weren't supported outside IE not so long ago, but there's a gap in PPK's tests, so we won't know at which point WebKit and Opera added support for them, unless we conduct tests ourselves: http://web.archive.org/web/20110624124152/http://quirksmode.org/dom/events/blurfocus.html I used it in CellWidgets because it is the equivalent of focus, but it bubbles so we can catch it at a top level element. I suggest removing it from this list. Given that they'll be standardized, and that browsers are adding support for them, I'd rather vote to keep them in. It's not much different from DnD, touch events or gestures not being supported everywhere either. http://gwt-code-reviews.appspot.com/1550803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduces BrowserEvents string constants (issue1550803)
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/BrowserEvents.java
File user/src/com/google/gwt/dom/client/BrowserEvents.java (right):
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/BrowserEvents.java#newcode39
user/src/com/google/gwt/dom/client/BrowserEvents.java:39: public static
final String FOCUSIN = "focusin";
focusin is not part of any standard and is only used in IE. I used it
in CellWidgets because it is the equivalent of focus, but it bubbles so
we can catch it at a top level element. I suggest removing it from this
list.
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/Document.java
File user/src/com/google/gwt/dom/client/Document.java (right):
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/Document.java#newcode18
user/src/com/google/gwt/dom/client/Document.java:18: import static
com.google.gwt.dom.client.BrowserEvents.*;
Do we have a rule against importing .*?
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/dom/client/Document.java#newcode571
user/src/com/google/gwt/dom/client/Document.java:571:
@SuppressWarnings("deprecation")
Do you need to suppress deprecation on a deprecated method?
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java
File
user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java
(right):
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java#newcode162
user/src/com/google/gwt/user/cellview/client/CellBasedWidgetImplTrident.java:162:
if (BrowserEvents.FOCUSIN.equals(type)) {
FOCUSIN is IE specific. I would leave the string in this case, or
create a static private String in this impl class.
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java
File user/src/com/google/gwt/user/client/impl/DOMImplStandard.java
(right):
http://gwt-code-reviews.appspot.com/1550803/diff/1/user/src/com/google/gwt/user/client/impl/DOMImplStandard.java#newcode57
user/src/com/google/gwt/user/client/impl/DOMImplStandard.java:57: if
(evt.getType().equals(BrowserEvents.MOUSEOUT)) {
MOUSEOUT/MOUSEOVER
http://gwt-code-reviews.appspot.com/1550803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Introduces BrowserEvents string constants (issue1550803)
RayC, John is on vacation. Can you review? Please keep a good eye out for typos. http://gwt-code-reviews.appspot.com/1550803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
