Bug 66027 – Implementation for "nav-up" "nav-down" "nav-left" "nav-right" properties in CSS3-UI module

• Component: CSS
• Status: RESOLVED INVALID

[kyounga]

Implementation for "nav-up" "nav-down" "nav-left" "nav-right" properties in CSS3-UI module

the latest standards
http://dev.w3.org/csswg/css3-ui/#nav-dir

This feature is necessary for a minor devices (without  mouse device). 
As the standards says, 
the author can define the directional focus navigation following the author's intension, instead of the browser's default behavior.

I think this feature can replace some JavaScript code which is made for special directional focus navigation by the author

[Shane Stephens]

Note that this specification is still an Editors Draft, which means that it may change at any time.

[Gyuyoung Kim]

As mentioned in antonio's question on webkit-dev, I wonder what is difference between spatial navigation and this feature. If operation is similar to Spatial Navigation, it seems this feature can be implemented by Spatial Navigation. 

And,  I'm not sure if this spec is sufficiently stable.

[kyounga]

(In reply to comment #2)
> As mentioned in antonio's question on webkit-dev, I wonder what is difference between spatial navigation and this feature. If operation is similar to Spatial Navigation, it seems this feature can be implemented by Spatial Navigation. 
> 
> And,  I'm not sure if this spec is sufficiently stable.

I think these property is new feature in addition to a "spatial navigation" which browsers provide by default.
If <a> element with "nav-up:auto" has focused and the user press "up arrow key", the browser use a spatial navigation, or if there is "nav-up:#foo", the browser send a focus the element with "id=foo" attribute. 

So, I think if I can reuse the fragment link function, it might NOT be a big deal.

[Simon Fraser (smfr)]

The definition of the nav-* properties is odd. No other CSS property has bare selectors as values, so we don't have any support for parsing those right now.

I'd hold off implementing this until the CSS WG has had a chance to discuss this further. If you want to kick-start discussion, please email www-style.

[kyounga]

Created attachment 106113 [details]
initial patch for review.

I know css-style are discussing on using ID selector as CSS value.
just made an initial implementation only for review.

There are several issues to be discussed in my patch
1. Changed CSSGrammar.y
    - Added IDSEL as a value, checked for #15360 issue (IDSEL was removed by this issue.)
2. Added a new class member to RenderStyle, instead of pointer or smart pointer.
    - Is it memory waste? less memory fragmentation? good performance? 
    - I think that this property won't be used frequently but if it is used once, it may be used frequently in one page.

Simple unit-tests are completed.
- no "nav-*" property.
- simple "nav-*" property from CSS3-UI example.
- scrollable page.
- inherit value
- invalid target name = no work.
- target name = between two frames.

[Antonio Gomes]

Comment on attachment 106113 [details]
initial patch for review.

please provide a patch that applies to ToT

[kyounga]

Created attachment 123237 [details]
initial patch for the latest webkit. 

Review this, please.
- Opera's supported already it. 
- These properties are used  popularly on web content in TV industry

[Antonio Gomes]

Comment on attachment 123237 [details]
initial patch for the latest webkit. 

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

patch still does not apply it seems.

anyways, some nitpicks:

> Source/WebCore/page/FocusController.cpp:795
> +    RenderObject* renderer = focusedNode ? focusedNode->renderer() : 0;
> +
> +    if (!renderer)

drop extran line here.

> Source/WebCore/page/FocusController.cpp:833
> +    if (target == "current")
> +        targetFrame = curFrame;
> +    else if (target == "root")
> +        targetFrame = curFrame->tree()->top();

DEFINE_STATIC_LOCAL(AtomicString, Current, ("current"));
DEFINE_STATIC_LOCAL(AtomicString, Root, ("root"))

> Source/WebCore/page/FocusController.cpp:844
> +    if (!anchorNode)
> +        return false;
> +    // if it's same with the current focused, it should be consumed.

add an extra line here.

if -> If

> Source/WebCore/page/FocusController.cpp:852
> +    bool successfullyFocused = setFocusedNode(anchorNode, targetFrame);
> +
> +    if (successfullyFocused)

drop extra line here.

> Source/WebCore/page/FocusController.cpp:854
> +        m_page->chrome()->focusedNodeChanged(anchorNode);
> +    return true;    

add an extra line here.

[kyounga]

Created attachment 123446 [details]
Patch_v2 is including the reviewer's comment.

I've attached a new patch following the comment from Antonio Gomes.

My patch includes two new files. 
I build and tested this patch only on my mac, with XCode project. 
I think that's a why the build bot makes fail log for cr-linux, qt, gtk, win.
("style" also has fail log even though I checked check-webkit-style script)
Should I modify project files or build scripts for other platform? 
Honestly, I don't know how to do.

[kyounga]

Is there someone to review this?
Review this patch, Please....

I think it's a useful feature and not a big deal, except css parser change. 
Any comment is welcome.

[Gyuyoung Kim]

Comment on attachment 123446 [details]
Patch_v2 is including the reviewer's comment.

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

> Source/WebCore/ChangeLog:3
> +        Need a short description and bug URL (OOPS!)

It looks you need to study how to write changelog first before submitting a patch. Please read webkit contribution guideline first.

[Gyuyoung Kim]

Comment on attachment 123446 [details]
Patch_v2 is including the reviewer's comment.

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

In my opinion, this patch should to have test cases as well. Then, I think reviewers can start to review this patch. By the way, I wonder if you notify this new feature to webkit-dev. If you didn't it, you have to do it first.

>> Source/WebCore/ChangeLog:3
>> +        Need a short description and bug URL (OOPS!)
> 
> It looks you need to study how to write changelog first before submitting a patch. Please read webkit contribution guideline first.

WebKit contribution guidance.
 - http://www.webkit.org/coding/contributing.html

> Source/WebCore/css/CSSStyleApplyProperty.cpp:1625
> +                || primitiveValue->primitiveType() == CSSPrimitiveValue::CSS_PARSER_HEXCOLOR)

Wrong indentation for *||* operator.

> Source/WebCore/css/CSSStyleSelector.cpp:4465
> +        for (size_t i = 0; i < 2; i++) {

WebKit prefers to use ++i instead of i++.

> Source/WebCore/css/CSSStyleSelector.h:355
> +    StyleNavigationValue getStyleNavigationValue(CSSValue*);

Nit : Add an empty line.

> Source/WebCore/page/FocusController.cpp:840
> +    // TODO:// which one is right?

WebKit prefer to use FIXME: instead of TODO:

> Source/WebCore/rendering/style/StyleNavigationData.h:2
> + * Copyright (C) 2011 Kyounga Ra ([e-mail])

s/2011/2012/g

> Source/WebCore/rendering/style/StyleNavigationData.h:33
> +    StyleNavigationData(const StyleNavigationData& o)

Use *explicit* keyword.

[James Craig]

The CSS WG agreed to remove this feature recently. See discussion in bug 95178. Marking as Resolved/Invalid.

[James Craig]

*** Bug 95178 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

(In reply to comment #13)
> The CSS WG agreed to remove this feature recently. See discussion in bug 95178. Marking as Resolved/Invalid.

Not "remove", but just "don't include in level 3", which means they will likely get postponed to level 4.

[kyounga]

Created attachment 207442 [details]
patch for latest webkit

patch for latest webkit. 
webkit-style is checked.

Even though nav-dir properties was dropped from CSS3, it is still demanded from next CSS4 or TV industry.

[Antonio Gomes]

Comment on attachment 207442 [details]
patch for latest webkit

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

It needs review on the style change. I only reviewed it overly and specially the FocusController change.

1) it needs a proper changelog entry
2) it needs tests. maybe lots...

r- for now. It looks to be in the right track though.

> Source/WebCore/ChangeLog:9
> +        Support directional focus navigation properties(nav-[dir]) in CSS UI module.
> +        https://bugs.webkit.org/show_bug.cgi?id=66027
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests (OOPS!).
> +

Please explain your solution here, preferably with per function change entries (see below)

> Source/WebCore/page/FocusController.cpp:945
> +    // If we were in the autoscroll/panScroll mode we want to stop it.
> +    curFrame->eventHandler()->stopAutoscrollTimer();

this needs a test, I would say.

> Source/WebCore/page/FocusController.cpp:962
> +    if (focused == anchor)
> +        return true;

this needs a test.

> Source/WebCore/page/FocusController.cpp:964
> +    anchor->scrollIntoViewIfNeeded(false);

add a comment saying what "false" is. e.g. (false /*bleh*/)

[James Craig]

(In reply to comment #16)
> Even though nav-dir properties was dropped from CSS3, it is still demanded from next CSS4 or TV industry.

While I agree with you that directional navigation is necessary,implementing this portion of the current specification is short-sighted. The draft has serious problems that will result in formal objections to the W3C if they remained unaddressed.

[Benjamin Poulain]

This needs a feature flag (since it does not have platform integration, you can probably enable it by default), an experimental prefix, and tests; a lot of tests.

[Benjamin Poulain]

(In reply to comment #18)
> (In reply to comment #16)
> > Even though nav-dir properties was dropped from CSS3, it is still demanded from next CSS4 or TV industry.
> 
> While I agree with you that directional navigation is necessary,implementing this portion of the current specification is short-sighted. The draft has serious problems that will result in formal objections to the W3C if they remained unaddressed.

Let's be constructive. Can you share which problems should block this?

[James Craig]

Objections sent to the CSS working group on www-style list.
http://lists.w3.org/Archives/Public/www-style/2013May/0076.html

Another FYI, nav-up/down/left/right were pushed to CSS4 but according to Leif: "[CSS3] retains nav-up,
nav-down, nav-left, nav-right [and marked as at-risk, but], nav-index is still
postponed." http://lists.w3.org/Archives/Public/www-style/2013Jul/0711.html