Bug 95178 – Support CSS3 Keyboard control

• Component: CSS
• Status: RESOLVED DUPLICATE

[Binyamin]

Add support for CSS3 Keyboard control http://dev.w3.org/csswg/css3-ui/#keyboard, the new properties: nav-index, nav-up, nav-right, nav-down, nav-left, ime-mode.

[Alexey Proskuryakov]

Do you want any of these features specifically? Having a use case would go a long way towards establishing importance of a feature.

[Binyamin]

(In reply to comment #1)

@Alexey, I think it would be grate to have support for all those features.

[Sangho Kim]

Created attachment 198982 [details]
add the feature of css3 keyboard navigation

I attached the patch which has the feature of CSS3 keyboard navigation.
Please review...

[Philippe Normand]

Comment on attachment 198982 [details]
add the feature of css3 keyboard navigation

View in context: https://bugs.webkit.org/attachment.cgi?id=198982&action=review

ChangeLog is missing. Also, can the new files be explicitely put under LGPL2 or BSD license? And don't forget to set the r flag to '?'

> Source/WebKit2/Shared/WebPreferencesStore.h:-39
> -#if PLATFORM(GTK)

Unrelated change?

[Sangho Kim]

Created attachment 199152 [details]
add change log and license

I add the change log and license.
Do I have to do another?

[WebKit Commit Bot]

Attachment 199152 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/dom/Element.h', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/StyleNavData.cpp', u'Source/WebCore/rendering/style/StyleNavData.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h', u'Source/WebKit2/Shared/WebPreferencesStore.h']" exit_code: 1
ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:17:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:18:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:19:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:20:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:21:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:22:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:23:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:24:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:25:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:26:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:27:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:28:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:29:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:30:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ChangeLog:31:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/css/CSSPrimitiveValue.h:137:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 21 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 199152 [details]
add change log and license

Attachment 199152 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/16639

[Build Bot]

Comment on attachment 199152 [details]
add change log and license

Attachment 199152 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/16640

[Alexey Proskuryakov]

What regression tests do you have for this feature? It will require a substantial number of tests.

[Ryosuke Niwa]

Has this feature been announced on webkit-dev yet? (See http://www.webkit.org/coding/adding-features.html)

[Build Bot]

Comment on attachment 199152 [details]
add change log and license

Attachment 199152 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/110757

[Ryosuke Niwa]

Comment on attachment 199152 [details]
add change log and license

View in context: https://bugs.webkit.org/attachment.cgi?id=199152&action=review

Thank you for the patch but this patch needs tests. Every new feature or bug fix needs to come up with a layout test whenever possible.
See http://www.webkit.org/coding/contributing.html

There is a lot of stylistic and naming issues as I've pointed out below.

> ChangeLog:6
> +        Reviewed by .

Please don't remove NOBODY (OOPS!). (You used webkit-patch upload or prepare-ChangeLog in Tools/Scripts, right?)

>> ChangeLog:12
>> +        * Source/WebCore/Target.pri
> 
> Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]

You're missing a colon after the file name.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2109
> +            const StyleNavIndex* nav = style->getNavIndex();

Please don't use abbreviations like nav. Spell out navigation.

> Source/WebCore/css/CSSParser.cpp:2953
> +        if ((num > 2) || (value->unit != CSSPrimitiveValue::CSS_PARSER_HEXCOLOR && value->unit != CSSPrimitiveValue::CSS_ELEMENT_ID))

No need to wrap inner conditions with parentheses. Why are you comparing the unit to CSS_PARSER_HEXCOLOR and CSS_ELEMENT_ID? That doesn't make much sense.

> Source/WebCore/rendering/style/RenderStyle.cpp:1703
> +const StyleNavData* RenderStyle::getNav(int propId)

this name is too terse. Please give more descriptive name. Also, we don't prefix a function with "get" unless it has an out argument.

> Source/WebCore/rendering/style/StyleNavData.cpp:27
> +StyleNavData::StyleNavData()

Please don't use abbreviations like Nav.
Also, StyleNavData is a very vague name. Please consider giving it a more descriptive name.

> Source/WebCore/rendering/style/StyleNavData.cpp:29
> +    : m_id("")
> +    , m_target("")

Please use emptyString instead or let the default constructor run.

[Ryosuke Niwa]

http://www.webkit.org/coding/coding-style.html is our style guideline. Please have a look at it.

[Gyuyoung Kim]

Comment on attachment 199152 [details]
add change log and license

View in context: https://bugs.webkit.org/attachment.cgi?id=199152&action=review

> ChangeLog:8
> +        Add support for CSS3 Keyboard control http://dev.w3.org/csswg/css3-ui/#keyboard.

I'm wondering if you notify webkit-dev of this feature. According to webkit policy, new feature should be notified to webkit-dev first.

> Source/WebCore/GNUmakefile.list.am:4722
> +	Source/WebCore/rendering/style/StyleNavData.cpp \

I think this file needs to be added to CMake files as well in order to support EFL, blackberry and wince ports. Besides it looks you need to add this file to xcode for Mac.

> Source/WebCore/rendering/style/StyleNavData.cpp:2
> + * Copyright (C) 2013 Sangho Kim ([e-mail])

If contributor belongs to company, contributor has added a company copyright generally.

> Source/WebKit2/Shared/WebPreferencesStore.h:39
> +#if PLATFORM(GTK) || PLATFORM(QT)

There is no WK2 ChangeLog update for this file. As Ryosuke said, please use Tools/Script/prepare-ChangeLog.

[Sangho Kim]

Created attachment 200300 [details]
css3 keyboard navigation patch

I changed some code as reference your reviews.

[WebKit Commit Bot]

Attachment 200300 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Target.pri', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSGrammar.y.in', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSPrimitiveValue.cpp', u'Source/WebCore/css/CSSPrimitiveValue.h', u'Source/WebCore/css/CSSProperty.cpp', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/dom/Element.h', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebCore/page/FocusController.cpp', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/StyleNavDirection.cpp', u'Source/WebCore/rendering/style/StyleNavDirection.h', u'Source/WebCore/rendering/style/StyleNavIndex.cpp', u'Source/WebCore/rendering/style/StyleNavIndex.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/WebPreferencesStore.h']" exit_code: 1
Source/WebCore/css/CSSPrimitiveValue.h:137:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 200300 [details]
css3 keyboard navigation patch

Attachment 200300 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/384040

[EFL EWS Bot]

Comment on attachment 200300 [details]
css3 keyboard navigation patch

Attachment 200300 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/268060

[Build Bot]

Comment on attachment 200300 [details]
css3 keyboard navigation patch

Attachment 200300 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/393032

[Build Bot]

Comment on attachment 200300 [details]
css3 keyboard navigation patch

Attachment 200300 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/268064

[Build Bot]

Comment on attachment 200300 [details]
css3 keyboard navigation patch

Attachment 200300 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/333116

[James Craig]

We should not support any of the nav properties. nav-index, nav-up, nav-right, nav-down, and nav-left are all features at risk and we should let them remain at risk until they are removed from this version of the specification.

The main thing nav-index is lacking is a context. This could potentially be solved with an additional property, potentially:

     nav-context:  ignore | relative | absolute;

Yet, as specified, nav-index has all the same serious negative implications of positive integer values of tabindex, where each node has to know about the layout order of every other node in the document. If you're only specifying nav index on first level branches, this is fine, but there is no good way to use this on leaf nodes, and it has severe consequences for breaking index order of keyboard navigation.

Another problem this does not solve is the potential for infinite navigation loops with circular references using any of the nav-* properties. The specification should define that circular references are illegal, and it should also define recovery behavior when a rendering engine encounters circular navigation loops.

I mentioned both of these problems to Tantek at last years TPAC. I can't recall whether I sent a formal note to the list, but I will send a follow-up objection now.

The ime-mode property also seems problematic but I'm not yet familiar enough with this part of the proposal to officially object. Why should a web page get to disable my Pinyin character window? The example given it would not allow extended set characters to be entered into a form, but it seems this would be better solved by either 1) simple form validation, or 2) regex accept patterns in HTML input. Allowing CSS to disable IMEs seems like overkill for this problem.

[Alexey Proskuryakov]

Bug 21279 tracks ime-mode, and I also had objections against it.

[James Craig]

Sent objections to www-style:
http://lists.w3.org/Archives/Public/www-style/2013May/0076.html

[James Craig]

David Baron says, "The group resolved a few weeks ago to drop them from level 3"
http://lists.w3.org/Archives/Public/www-style/2013Apr/0428.html

Any objections to closing this as "RESOLVED INVALID"?

[Andreas Kling]

Comment on attachment 200300 [details]
css3 keyboard navigation patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200300&action=review

It sounds like we shouldn't be implementing this at all. Regardless, r- because this should be behind an ENABLE-flag, and new features should be announced on webkit-dev.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:206
> +    DataRef<StyleNavDirection> m_navUp;
> +    DataRef<StyleNavDirection> m_navDown;
> +    DataRef<StyleNavDirection> m_navLeft;
> +    DataRef<StyleNavDirection> m_navRight;
> +    DataRef<StyleNavIndex>     m_navIndex;

I don't like the idea of growing StyleRareNonInheritedData by 5 whole pointers for a single feature that's not used on the web yet. Can we make group this stuff under a single pointer here?

[Philippe Normand]

Likely a duplicate of bug 66027.

[James Craig]

*** This bug has been marked as a duplicate of bug 66027 ***