Bug 66027 - Implementation for "nav-up" "nav-down" "nav-left" "nav-right" properties in CSS3-UI module
: Implementation for "nav-up" "nav-down" "nav-left" "nav-right" properties in C...
Status: RESOLVED INVALID
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Enhancement
Assigned To:
: http://dev.w3.org/csswg/css3-ui/#nav-dir
:
:
:
  Show dependency treegraph
 
Reported: 2011-08-10 18:33 PST by
Modified: 2013-10-08 05:41 PST (History)


Attachments
initial patch for review. (32.46 KB, patch)
2011-09-02 04:00 PST, kyounga
tonikitoo: review-
Review Patch | Details | Formatted Diff | Diff
initial patch for the latest webkit. (30.39 KB, patch)
2012-01-19 18:40 PST, kyounga
no flags Review Patch | Details | Formatted Diff | Diff
Patch_v2 is including the reviewer's comment. (30.67 KB, patch)
2012-01-21 06:13 PST, kyounga
no flags Review Patch | Details | Formatted Diff | Diff
patch for latest webkit (27.48 KB, patch)
2013-07-25 02:24 PST, kyounga
tonikitoo: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-08-10 18:33:27 PST
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
------- Comment #1 From 2011-08-10 20:21:19 PST -------
Note that this specification is still an Editors Draft, which means that it may change at any time.
------- Comment #2 From 2011-08-10 21:51:49 PST -------
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.
------- Comment #3 From 2011-08-11 07:15:27 PST -------
(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.
------- Comment #4 From 2011-08-11 08:33:36 PST -------
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.
------- Comment #5 From 2011-09-02 04:00:14 PST -------
Created an attachment (id=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.
------- Comment #6 From 2012-01-06 13:38:29 PST -------
(From update of attachment 106113 [details])
please provide a patch that applies to ToT
------- Comment #7 From 2012-01-19 18:40:58 PST -------
Created an attachment (id=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
------- Comment #8 From 2012-01-19 21:58:24 PST -------
(From update of attachment 123237 [details])
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.
------- Comment #9 From 2012-01-21 06:13:31 PST -------
Created an attachment (id=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.
------- Comment #10 From 2012-03-18 18:09:20 PST -------
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.
------- Comment #11 From 2012-07-17 03:50:19 PST -------
(From update of attachment 123446 [details])
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.
------- Comment #12 From 2012-07-17 04:03:54 PST -------
(From update of attachment 123446 [details])
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 (kyounga.ra@gmail.com)

s/2011/2012/g

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

Use *explicit* keyword.
------- Comment #13 From 2013-06-24 09:30:27 PST -------
The CSS WG agreed to remove this feature recently. See discussion in bug 95178. Marking as Resolved/Invalid.
------- Comment #14 From 2013-06-24 09:31:47 PST -------
*** Bug 95178 has been marked as a duplicate of this bug. ***
------- Comment #15 From 2013-06-24 10:38:42 PST -------
(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.
------- Comment #16 From 2013-07-25 02:24:24 PST -------
Created an attachment (id=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.
------- Comment #17 From 2013-07-25 07:17:10 PST -------
(From update of attachment 207442 [details])
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*/)
------- Comment #18 From 2013-07-25 09:35:59 PST -------
(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.
------- Comment #19 From 2013-07-26 20:09:16 PST -------
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.
------- Comment #20 From 2013-07-26 20:10:32 PST -------
(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?
------- Comment #21 From 2013-08-06 19:28:38 PST -------
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