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
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Enhancement
Assigned To: Nobody
http://dev.w3.org/csswg/css3-ui/#nav-dir
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-10 18:33 PDT by kyounga
Modified: 2013-10-08 05:41 PDT (History)
22 users (show)

See Also:


Attachments
initial patch for review. (32.46 KB, patch)
2011-09-02 04:00 PDT, kyounga
tonikitoo: review-
Details | Formatted Diff | Diff
initial patch for the latest webkit. (30.39 KB, patch)
2012-01-19 18:40 PST, kyounga
no flags 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 Details | Formatted Diff | Diff
patch for latest webkit (27.48 KB, patch)
2013-07-25 02:24 PDT, kyounga
tonikitoo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description kyounga 2011-08-10 18:33:27 PDT
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 Shane Stephens 2011-08-10 20:21:19 PDT
Note that this specification is still an Editors Draft, which means that it may change at any time.
Comment 2 Gyuyoung Kim 2011-08-10 21:51:49 PDT
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 kyounga 2011-08-11 07:15:27 PDT
(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 Simon Fraser (smfr) 2011-08-11 08:33:36 PDT
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 kyounga 2011-09-02 04:00:14 PDT
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.
Comment 6 Antonio Gomes 2012-01-06 13:38:29 PST
Comment on attachment 106113 [details]
initial patch for review.

please provide a patch that applies to ToT
Comment 7 kyounga 2012-01-19 18:40:58 PST
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
Comment 8 Antonio Gomes 2012-01-19 21:58:24 PST
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.
Comment 9 kyounga 2012-01-21 06:13:31 PST
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.
Comment 10 kyounga 2012-03-18 18:09:20 PDT
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 Gyuyoung Kim 2012-07-17 03:50:19 PDT
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.
Comment 12 Gyuyoung Kim 2012-07-17 04:03:54 PDT
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 (kyounga.ra@gmail.com)

s/2011/2012/g

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

Use *explicit* keyword.
Comment 13 James Craig 2013-06-24 09:30:27 PDT
The CSS WG agreed to remove this feature recently. See discussion in bug 95178. Marking as Resolved/Invalid.
Comment 14 James Craig 2013-06-24 09:31:47 PDT
*** Bug 95178 has been marked as a duplicate of this bug. ***
Comment 15 Simon Fraser (smfr) 2013-06-24 10:38:42 PDT
(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 kyounga 2013-07-25 02:24:24 PDT
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.
Comment 17 Antonio Gomes 2013-07-25 07:17:10 PDT
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*/)
Comment 18 James Craig 2013-07-25 09:35:59 PDT
(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 Benjamin Poulain 2013-07-26 20:09:16 PDT
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 Benjamin Poulain 2013-07-26 20:10:32 PDT
(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 James Craig 2013-08-06 19:28:38 PDT
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