Bug 23477 – Support for WCSS extensions

• Component: CSS
• Status: RESOLVED WONTFIX

[Mahesh Kulkarni]

Support for WCSS extensions to webkit. 
http://www.wapforum.org/tech/documents/WAP-239-WCSS-20011026-a.pdf

Webkit has added support for xHTML and its tags, all CSS2 (WCSS) styles except WCSS extensions. Extension styles include wap-marquee, wap-input-format and wap-accesskey.

[Mahesh Kulkarni]

Created attachment 26928 [details]
test case

[Mahesh Kulkarni]

Created attachment 26929 [details]
initial patch for WCSS marquee styles 

first patch for only WCSS marquee extensions styles. This patch handles following WCSS extension styles,

  The -wap-marquee Value for the display Property and
  -wap-marquee-style
  -wap-marquee-loop
  -wap-marquee-dir
  -wap-marquee-speed
  
Remaining style handling will follow in next patches. 

ps: do we have to restrict these styles only to xHTML pages?

[Darin Adler]

Comment on attachment 26929 [details]
initial patch for WCSS marquee styles 

I think Hyatt should review this.

> +    RenderStyle* s = m_layer->renderer()->style();
> +    
> +    if(s->marqueeLoopCount() == 0 && s->display() == WAP_MARQUEE)

WebKit coding style would use a longer name, perhaps "style" for the local variable.

WebKit coding style would include a space after the word "if" before the "(".

WebKit coding style would normally use "!" rather than "== 0".

This patch adds lots of "wap" CSS properties, but it adds them unconditionally. That means they can be tested with regression tests. So this needs to include some regression tests demonstrating these properties are present.

This patch adds lots of "wap" CSS properties that it doesn't implement. Is that really the right way to do this?

This patch adds some properties, like -wap-marquee-speed, without adding computed style support. It's unfortunate that there are so many old properties where we don't do computed style correctly. We don't want to make things even worse by adding new properties that also don't have computed style support. So that should be included with any new CSS properties.

I'm going to say review-, but Hyatt needs to review for the more basic questions like, "Should we have these '-wap' properties unconditionally in all WebKit-based browsers?"

[Mahesh Kulkarni]

Created attachment 26997 [details]
new patch: removed unwanted changes

Thanks Darin. 

Apology for couple of coding style mistakes. I have corrected it in this patch. 

1) I have added all wap styles but handled only WAP-MARQUEE because we are working on supporting WCSS extensions styles.  
2) Will submit ACCESS-KEY and INPUT-FORMAT patch few more days as they are under work. 
3) Computed styles for wap styles not required. I have removed my changes in this patch. 

Webkit supports xHTML and WCSS except WCSS extensions. I will also ask David to review.

[Dave Hyatt]

Comment on attachment 26997 [details]
new patch: removed unwanted changes

Looks pretty good.  Here are some comments:

Your handling of the display property is not correct.  You can't mutate overflowX at that time or you won't pick up all cases.  Consider the example of "display:inherit", where someone inherits a display type of -wap-marquee.  Your code won't set overflowX in that case.  A better place for that code would be in adjustRenderStyle in CSSStyleSelector.  That code runs after the style has been fully determined, so at that time you can ask if the display type is -wap-marquee and then set overflowX to OMARQUEE.

I know you probably plan to implement them, but for this patch please remove the CSS properties from CSSPropertyNames.in that aren't being handled yet.

You need to patch CSSComputedStyleDeclaration to handle all of these new properties, so check that out.

[Mahesh Kulkarni]

Created attachment 27153 [details]
final_patch

Thanks for the comments. 

1) changes to handle "display: inherit" cases. 
2) removed the unhandled css properties

[George Staikos]

This should not go in before XHTML-MP as it doesn't make much sense without it.  All of that is held up in review, and furthermore we announced quite some time ago on webkit-dev that we had patches for these things, which are all queued up so that they go in proper order.

[Dave Hyatt]

Comment on attachment 27153 [details]
final_patch

Should this code be behind an #ifdef?  I thought George was saying that there was an #ifdef for WCSS stuff...

[Dave Hyatt]

Comment on attachment 27153 [details]
final_patch

This patch looks fine.  The only remaining thing is that WCSS code should be wrapped in an ifdef.

Please make a new ifdef for WCSS, ENABLE(WCSS), similar to ENABLE(WML), and wrap all of this code in it.  Like WML it should be turned off by default.

[Mahesh Kulkarni]

Created attachment 27390 [details]
added all marquee changes under WCSS flag

set WCSS flag by default "off"

[Rohini Ananth]

Created attachment 27392 [details]
Patch :: -wap-accesskey support

-Changes made to CSSParser and CSSStyleSelector files to map -wap-accesskey to follow the path of "accesskey" HTML attribute. 
-Fallbacks and Multiple key Assignments not supported as of now

[Rohini Ananth]

Created attachment 27393 [details]
Test content for -wap-accesskey

[Mahesh Kulkarni]

Created attachment 27394 [details]
marquee changes put under WCSS flag.

typo in prev one

[Sreedhar Vaddi]

Created attachment 27397 [details]
wap input format support

Support WCSS style -wap-input-format and -wap-input-required for input tag.
Added the CSS style and RenderStyle for both.
Handled validation of input at client editor according to given -wap-input-format attribute value.

[Sreedhar Vaddi]

Created attachment 27398 [details]
test case for wap-inputp-format

[Sreedhar Vaddi]

Comment on attachment 27398 [details]
test case for wap-inputp-format

<?xml version="1.0"?>
<!DOCTYPE html PUBLIC "-//WAPFORUM//DTD XHTML Mobile 1.0//EN"
"http://www.wapforum.org/DTD/xhtml-mobile10.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>inputformat.xhtml</title>

<style type="text/css">

/*
Define classes of wildcard formats.
However, class names must start with chars, not digits.
*/

.star_upA {-wap-input-format: "*A";} /* upp ltr, sym, punc, NO num */
.star_lowa {-wap-input-format: "*a";} /* low ltr, sym, punc, NO num */
.star_upN {-wap-input-format: "*N";} /* num */
.star_lown {-wap-input-format: "*n";} /* num, sym, punc */
.star_upX {-wap-input-format: "*X";} /* up ltr, num, sym, punc */
.star_lowx {-wap-input-format: "*x";} /* low ltr, num, sym, punc */
.star_upM {-wap-input-format: "*M";} /* any, up default */
.star_lowm {-wap-input-format: "*m";} /* any, low default */

/* Constrained, extra backslash to escape literal backslash */
.one {-wap-input-format: "3N";}
.two {-wap-input-format: "3a";}
.three {-wap-input-format: "NNN\\-NNNN";}
.four {-wap-input-format: "NN\\/2N";}
.five {-wap-input-format: "A\\-NN\\-*N";}
.six {-wap-input-format: "Aaa\\ Aaa";}
.seven {-wap-input-format: "NNAANN2a";}

</style>

</head>

<body>

<form method="get" action="formsubget.asp">

  <p>
  Basic formats:<br />
  <br />
  *A: up ltr, sym, punc<br />
  <input class="star_upA" type="text" name="star_upa"/><br />
  <br />
  *a: low ltr, sym, punc<br />

  <input class="star_lowa" type="text" name="star_lowa"/><br />
  <br />
  *N: num<br />
  <input class="star_upN" type="text" name="star_upn"/><br />
  <br />
  *n: num, sym, punc<br />
  <input class="star_lown" type="text" name="star_lown"/><br />
  <br />

  *X: up ltr, num, sym, punc<br />
  <input class="star_upX" type="text" name="star_upx"/><br />
  <br />
  *x: low ltr, num, sym, punc<br />
  <input class="star_lowx" type="text" name="star_lowx"/><br />
  <br />
  *M: anything, up ltr default<br />

  <input class="star_upM" type="text" name="star_upm"/><br />
  <br />
  *m: anything, low ltr default<br />
  <input class="star_lowm" type="text" name="star_lowm"/><br />
  <br />
  Constrained formats:<br />
  <br />
  3N: 3 num<br />

  <textarea class="one" rows="1" cols="10" name="text1"></textarea><br />
  <br />
  3a: 3 low ltr<br />
  <textarea class="two" rows="1" cols="10" name="text2"></textarea><br />
  <br />
  NNN-NNNN: 123-4567<br />
  <textarea class="three" rows="1" cols="10" name="text3"></textarea><br />
  <br />

  NN/2N: 12/25<br />
  <textarea class="four" rows="1" cols="10" name="text4"></textarea><br />
  <br />
  A-NN-*N: R-12-89...<br />
  <textarea class="five" rows="1" cols="10" name="text5"></textarea><br />
  <br />
  Aaa Aaa: Mad Pad<br />

  <textarea class="six" rows="1" cols="10" name="text6"></textarea><br />
  <br />
  NNAANN2a: 23AD23ad<br />
  <textarea class="seven" rows="1" cols="10" name="text7"></textarea><br />
  <br />
  Competing formats:<br />
  <br />
  3N: 3 num<br />

  <input class="one" format="3A" name="compete"/><br />
  <br />
  <input type="submit" />
  </p>

</form>
<p>
Enter Name : (in alphabits)
<input type="text" style="-wap-input-format: '*a'" />
<br />
Enter DateOfBirth : 
<input type="text" style="-wap-input-format: 'NN\\-NN\\-NNNN'" /> (enter in DD-MM-YYYY format)

<br />
Enter Phoneno : 
<input type="text" style="-wap-input-format: '9N'" /> (enter only 9 Digites only)
<br />
<br />
Comment please: 
<textarea style="-wap-input-format: '*x'" rows="3" cols="20"></textarea>
</p>
</body>
</html>

[Mark Rowe (bdash)]

Comment on attachment 27397 [details]
wap input format support

The majority of this code seems misplaced.  There doesn't seem to be anything Qt-specific about it, yet it's in Qt-specific files. In the WebKit directory no less.  Code that deals with parsing of the values of CSS properties should live in WebKit rather than WebCore, and should not be reimplemented for each platform.

> +#if ENABLE(WCSS)
> +    bool createFormatMask();
> +    bool cancleEditing();
> +    bool validateFormatText();
> +    void updateEditingMode();
> +#endif

There's a typo in here.


> +// END OF FILE

This isn't a particularly useful comment.


> +typedef unsigned short int UChar;
> +

This header shouldn't be redeclaring UChar.

> +typedef enum {
> +    ELeLoSymPuc,
> +    ELeUpSymPuc,   
> +    ENumSymPuc,    
> +    ENumChar,      
> +    ELeLoNumSymPuc,
> +    ELeUpNumSymPuc,
> +    EAnyLow,
> +    EAnyUpper,
> +    EStatic,
> +    ENoFormat 
> +}InputFormatMaskType;

We don't typically prefix our enum values with E.
  
> Index: WebCore/page/FocusController.cpp
> ===================================================================
> --- WebCore/page/FocusController.cpp	(revision 9761)
> +++ WebCore/page/FocusController.cpp	(working copy)
> @@ -31,6 +31,9 @@
>  #include "Document.h"
>  #include "Editor.h"
>  #include "EditorClient.h"
> +#if ENABLE(WCSS)
> +#include "EditorClientQt.h"
> +#endif
>  #include "Element.h"
>  #include "Event.h"
>  #include "EventHandler.h"
> @@ -257,7 +260,11 @@
>          
>      if (oldFocusedNode && oldFocusedNode->rootEditableElement() == oldFocusedNode && !relinquishesEditingFocus(oldFocusedNode))
>          return false;
> -        
> +#if ENABLE(WCSS)        
> +    if(!(static_cast<EditorClientQt*>(m_page->editorClient())->validateFormatText()))
> +        return false;
> +#endif

This is totally incorrect.  It's obviously broken on non-Qt platforms, but on a deeper level it's not correct for WebCore to access types declared in WebKit.  It's a layering violation.

> @@ -40,6 +44,9 @@
>  class CSSStyleSelector;
>  class StyleFlexibleBoxData;
>  class StyleMarqueeData;
> +#if ENABLE(WCSS)
> +class StyleWapInput;
> +#endif

Please don't put an #if'd block in the middle of a sorted list like this.  Add it at the end.

>  class StyleMultiColData;
>  class StyleReflection;
>  class StyleTransformData;

> Index: WebCore/rendering/style/StyleWapInput.h
> ===================================================================
> --- WebCore/rendering/style/StyleWapInput.h	(revision 0)
> +++ WebCore/rendering/style/StyleWapInput.h	(revision 0)
> @@ -0,0 +1,32 @@
> +#ifndef  StyleWapInput_h 
> +#define  StyleWapInput_h 
> +
> +#include "AtomicString.h"
> +#include <wtf/RefCounted.h>
> +#include <wtf/PassRefPtr.h>
> +
> +namespace WebCore {
> +
> +class StyleWapInput : public RefCounted<StyleWapInput> {
> +public:
> +    static PassRefPtr<StyleWapInput> create() { return adoptRef(new StyleWapInput); }
> +    PassRefPtr<StyleWapInput> copy() const { return adoptRef(new StyleWapInput(*this)); }

Is this copy method ever used?  It seems inconsistent with the code below.

> +private:
> +    StyleWapInput();
> +    StyleWapInput(const StyleWapInput&);
> +};

If this is intended to prevent copying then it is not needed.  RefCounted already inherits from WTF::Noncopyable.

> Index: WebCore/rendering/style/StyleWapInput.cpp
> ===================================================================
> --- WebCore/rendering/style/StyleWapInput.cpp	(revision 0)
> +++ WebCore/rendering/style/StyleWapInput.cpp	(revision 0)
> @@ -0,0 +1,19 @@
> +#include "config.h"
> +#include "StyleWapInput.h" 
> +
> +namespace WebCore {
> +
> +StyleWapInput::StyleWapInput() : 
> +               required(false)
> +{
> +
> +}
> +
> +StyleWapInput::StyleWapInput(const StyleWapInput& o) :
> +               RefCounted<StyleWapInput>(),
> +               format(o.format),
> +               required(o.required)

Please see our other files (eg, StyleRareNonInheritedData.cpp above) for how we format our initialisation lists.

[Sreedhar Vaddi]

Created attachment 28750 [details]
WCSS InputFormat Patch

Thanks for the comments. 

I have updated the patch for WAP_INPUT_FORMAT as per the review comments. 
Few other changes made are,
 - handling of automatically inserting escape characters   
 - invalid input/format call back is platform specific only if they implement. Otherwise behavior is like normal
   
Please review this patch.

[George Staikos]

Comment on attachment 27394 [details]
marquee changes put under WCSS flag.

marquee is checked in.

[Maciej Stachowiak]

Comment on attachment 27392 [details]
Patch :: -wap-accesskey support

For the WAP marquee properties, I suggested using a separate .in file for the property names like SVG does. I think that would be good here. At the very least the names should be conditional with ifdefs.

The change in accessKeyModifiers is not explained and seems unrelated. If it is related, please explain it in the ChangeLog.

r- to address these two issues.

[Dave Hyatt]

Comment on attachment 28750 [details]
WCSS InputFormat Patch

Minusing from Maciej's comments since I think he just forgot to?

[Dave Hyatt]

Comment on attachment 28750 [details]
WCSS InputFormat Patch

Seems like you are missing checking for m_wapInput in the == operator of StyleRareNonInheritedData.

I see code that is asking if isTextField() || isPasswordField().  Isn't it impossible to be a password field and not be a text field?  Seems like you just need to check isTextField?

I don't much like the name WebTextFormatMask. What's the reasoning behind prefacing the name with "Web"?  Seems like you could just drop that.

You should use an OwnPtr for m_textFormatMask.

[Rohini Ananth]

Created attachment 30710 [details]
Wap Access Key Patch with review comments incorporated

[Rohini Ananth]

Comment on attachment 30710 [details]
Wap Access Key Patch with review comments incorporated

Resubmitting patch with explanation in changelog about the changes related to wap-accesskey WCSS property

[Sreedhar Vaddi]

Created attachment 30901 [details]
Updated Patch for WCSS Inputformat 

WapInputFormat patch with review comments incorporated. I did not create a separate in file for the WCSS styles as they are few and could be just part of CSS?

[Eric Seidel (no email)]

Comment on attachment 30901 [details]
Updated Patch for WCSS Inputformat 

Lots of style errors.  Please run check-webkit-style.

+    MaskBase* m = NULL;

No need for ()
+    return (validate(text, eb));

Style:
+bool TextFormatMask::createStaticMask(const WebCore::String &p, int &pos)


eb is not a good variable name:
+bool TextFormatMask::validate(const String& text, ErrorBlock& eb)

No == 0:
+    if (text.length() == 0) 

Style:
+        ErrorBlock() : m_start(-1), m_extent(-1) {}
+    struct ErrorBlock
+    {

Style:
+    InputFormatMaskType nextInputMaskType(WebCore::Frame *frame, int aOffset);

overused WebCore:: namespace.

Please run check-webkit-style and have someone like George clear this for style before re-posting.  Silly to waste reviewers time fixing style issues.

I'm not sure we want this patch from a technical perspective (being discussed on webkit-dev), but we certainly can't accept it as-is style-wise.

You'll also have more luck getting reviews by posting smaller patches.  svn-create-patch will correctly put your layout tests and ChangeLogs first in the patch file which can make for easier reviews, but is not required.

[Adam Barth]

WCSS isn't even the top search result for itself.  I don't think we want extensions for it.