Bug 23758 – WCSS input extension is not supported

• Component: CSS
• Status: RESOLVED WONTFIX

[yichao.yin]

WCSS input extension is one of the WAP specific extenstions to CSS2.
It is consisted of two WCSS properties: -wap-input-format and -wap-input-required

[yichao.yin]

Created attachment 27391 [details]
inital patch for WCSS input support

[yichao.yin]

Created attachment 27653 [details]
updated patch for changed code


split the initial patch into two parts: this one contains the changed code, the upcoming one is for layout test

[yichao.yin]

Created attachment 27654 [details]
updated patch for layout test

[yichao.yin]

Created attachment 30315 [details]
Updated patch for adding tests

The latest tests for WCSS input extension

[yichao.yin]

Created attachment 30316 [details]
Updated patch about code changes

Latest code changes for WCSS input extension support

[yichao.yin]

Created attachment 30443 [details]
Latest Updated patch for code change

add copyright messages for code change

[George Staikos]

Comment on attachment 30315 [details]
Updated patch for adding tests

will need to be added to skipped for Mac OS also, but patch is fine.

[yichao.yin]

(In reply to comment #7)
> (From update of attachment 30315 [details] [review])
> will need to be added to skipped for Mac OS also, but patch is fine.

Mac OS? I have changed platform/mac/Skipped. Do you mean other mac-xxx platforms?

[George Staikos]

Sorry, I meant Qt.

[Sam Weinig]

Comment on attachment 30443 [details]
Latest Updated patch for code change

> +        This change depends on https://bugs.webkit.org/show_bug.cgi?id=23452

This doesn't make sense to have in a changelog.  Either the change works or it doesn't.

> +
> +        Tests: fast/wcss/inputelement-inputformat.xhtml
> +               fast/wcss/inputelement-inputrequired.xhtml
> +               fast/wcss/textarea-inputformat.xhtml
> +               fast/wcss/textarea-inputrequired.xhtml

These tests don't seem to be included.

> +    bool ok = true;
> +    UChar mask = inputFormatMask[maskIndex];
> +    // Match the inputed character with input mask
> +    switch (mask) {
> +    case 'A':
> +        ok = !WTF::isASCIIDigit(inChar) && !WTF::isASCIILower(inChar) && WTF::isASCIIPrintable(inChar);
> +        break;
> +    case 'a':
> +        ok = !WTF::isASCIIDigit(inChar) && !WTF::isASCIIUpper(inChar) && WTF::isASCIIPrintable(inChar);
> +        break;
> +    case 'N':
> +        ok = WTF::isASCIIDigit(inChar);
> +        break;
> +    case 'n':
> +        ok = !WTF::isASCIIAlpha(inChar) && WTF::isASCIIPrintable(inChar);
> +        break;
> +    case 'X':
> +        ok = !WTF::isASCIILower(inChar) && WTF::isASCIIPrintable(inChar);
> +        break;
> +    case 'x':
> +        ok = !WTF::isASCIIUpper(inChar) && WTF::isASCIIPrintable(inChar);
> +        break;
> +    case 'M':
> +        ok = WTF::isASCIIPrintable(inChar);
> +        break;
> +    case 'm':
> +        ok = WTF::isASCIIPrintable(inChar);

The WTF should not be needed for these (or at the very least, you should add using namespace WTF at the top of file).  The cases for 'M' and 'm' can be combined.
> Index: WebCore/html/HTMLTextAreaElement.h
> ===================================================================
> --- WebCore/html/HTMLTextAreaElement.h	(revision 43681)
> +++ WebCore/html/HTMLTextAreaElement.h	(working copy)
> @@ -3,6 +3,7 @@
>   *           (C) 1999 Antti Koivisto ([e-mail])
>   *           (C) 2000 Dirk Mueller ([e-mail])
>   * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
> + * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Library General Public
> @@ -26,11 +27,19 @@
>  
>  #include "HTMLFormControlElement.h"
>  
> +#if ENABLE(WCSS)
> +#include "InputElement.h"
> +#endif
> +
>  namespace WebCore {
>  
>  class VisibleSelection;
>  
> +#if ENABLE(WCSS)
> +class HTMLTextAreaElement : public HTMLFormControlElementWithState, public InputElement {
> +#else
>  class HTMLTextAreaElement : public HTMLFormControlElementWithState {
> +#endif

This seems wrong to me.  An HTMLTextAreaElement is not an InputElement and therefore should never inherit from InputElement.  The changes made to this class seem too invasive and I think a cleaner approach is necessary.  

r-

[yichao.yin]

(In reply to comment #10)
> (From update of attachment 30443 [details] [review])
> > +        This change depends on https://bugs.webkit.org/show_bug.cgi?id=23452
> This doesn't make sense to have in a changelog.  Either the change works or it
> doesn't.
> > +
> > +        Tests: fast/wcss/inputelement-inputformat.xhtml
> > +               fast/wcss/inputelement-inputrequired.xhtml
> > +               fast/wcss/textarea-inputformat.xhtml
> > +               fast/wcss/textarea-inputrequired.xhtml
> These tests don't seem to be included.

will remove it.

> > +    bool ok = true;
> > +    UChar mask = inputFormatMask[maskIndex];
> > +    // Match the inputed character with input mask
> > +    switch (mask) {
> > +    case 'A':
> > +        ok = !WTF::isASCIIDigit(inChar) && !WTF::isASCIILower(inChar) && WTF::isASCIIPrintable(inChar);
> > +        break;
> > +    case 'a':
> > +        ok = !WTF::isASCIIDigit(inChar) && !WTF::isASCIIUpper(inChar) && WTF::isASCIIPrintable(inChar);
> > +        break;
> > +    case 'N':
> > +        ok = WTF::isASCIIDigit(inChar);
> > +        break;
> > +    case 'n':
> > +        ok = !WTF::isASCIIAlpha(inChar) && WTF::isASCIIPrintable(inChar);
> > +        break;
> > +    case 'X':
> > +        ok = !WTF::isASCIILower(inChar) && WTF::isASCIIPrintable(inChar);
> > +        break;
> > +    case 'x':
> > +        ok = !WTF::isASCIIUpper(inChar) && WTF::isASCIIPrintable(inChar);
> > +        break;
> > +    case 'M':
> > +        ok = WTF::isASCIIPrintable(inChar);
> > +        break;
> > +    case 'm':
> > +        ok = WTF::isASCIIPrintable(inChar);
> The WTF should not be needed for these (or at the very least, you should add
> using namespace WTF at the top of file).  The cases for 'M' and 'm' can be
> combined.

Thanks, will combine them.

> > Index: WebCore/html/HTMLTextAreaElement.h
> > ===================================================================
> > --- WebCore/html/HTMLTextAreaElement.h	(revision 43681)
> > +++ WebCore/html/HTMLTextAreaElement.h	(working copy)
> > @@ -3,6 +3,7 @@
> >   *           (C) 1999 Antti Koivisto ([e-mail])
> >   *           (C) 2000 Dirk Mueller ([e-mail])
> >   * Copyright (C) 2004, 2005, 2006, 2007 Apple Inc. All rights reserved.
> > + * Copyright (C) 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
> >   *
> >   * This library is free software; you can redistribute it and/or
> >   * modify it under the terms of the GNU Library General Public
> > @@ -26,11 +27,19 @@
> >  
> >  #include "HTMLFormControlElement.h"
> >  
> > +#if ENABLE(WCSS)
> > +#include "InputElement.h"
> > +#endif
> > +
> >  namespace WebCore {
> >  
> >  class VisibleSelection;
> >  
> > +#if ENABLE(WCSS)
> > +class HTMLTextAreaElement : public HTMLFormControlElementWithState, public InputElement {
> > +#else
> >  class HTMLTextAreaElement : public HTMLFormControlElementWithState {
> > +#endif
> This seems wrong to me.  An HTMLTextAreaElement is not an InputElement and
> therefore should never inherit from InputElement.  The changes made to this
> class seem too invasive and I think a cleaner approach is necessary.  
> r-

In my opinion, HTMLTextAreaElement can be treated as an InputElement since it also receives user input.
Does any specifications define InputElement explicitly and say HTMLTextAreaElement is not an InputElment? 
I am not sure for that.

[Adam Barth]

I'm confused.  Is there something to be landed here?  If not, can we remove from the commit queue?

[George Staikos]

well, the tests seem fine.  I guess they could be landed (disabled)

[Mark Rowe (bdash)]

Landing tests without the supporting functionality isn't hugely useful, and would require a different patch than the one that is attached.

[Mark Rowe (bdash)]

Comment on attachment 30315 [details]
Updated patch for adding tests

Clearing the review flag to get this out of the review queue for now.

[Charles Wei]

Created attachment 38967 [details]
re-submit the patch after porting to ToT and addressing the reviews comments 

patch that addresses the reviewer's comments. 

This patch is to add WCSS input extension to CSS2,  which includes the following CSS properties:

-wap-input-format
-wap-input-required

with -wap-input-format,  the author can define the value space of an input element.
-wap-input-required says the input element can't be empty

[Eric Seidel (no email)]

Given that we have failed to find a reviewer to support this in over 2 months, and that I am no longer in support of adding this feature to WebKit, I think we should close out this bug/patch due to lack of reviewer support.

[Eric Seidel (no email)]

Comment on attachment 38967 [details]
re-submit the patch after porting to ToT and addressing the reviews comments 

r- due to lack of reviewer support based on my above comment and that no one has commented on this bug in the 2 weeks since I made it.

[Eric Seidel (no email)]

Comment on attachment 38967 [details]
re-submit the patch after porting to ToT and addressing the reviews comments 

6 weeks later, same story.  WebKit's apathy towards this patch suggests we should close this bug as WONTFIX.  Marking r- to remove this abandoned bug/patch from the review queue.

[Alexey Proskuryakov]

Bug 23477 was resolved with a general lack of desire to implement any WCSS extensions, so closing this one, too. Please e-mail webkit-dev mailing list if you are interested in WCSS support.