Implementation of CSS3 nav-up/down/left/right properties from CSS3 UI.

Implementation of CSS3 nav-up/down/left/right properties from CSS3 UI.
(http://dev.w3.org/csswg/css-ui/#nav-dir)
The CSS parser and RenderStyle is extended to to keep the information about CSS nav-* properties for HTML elements. The ID of element in current and other frames can be set as well as a special value "auto".

This patch reuses fragments of similar WebKit patch submitted by Kyounga Ra:
https://bugs.webkit.org/show_bug.cgi?id=66027
 
Note that spatial navigation feature (see https://codereview.chromium.org/13811041 and https://codereview.chromium.org/14110006) needs to be enabled for this patch to be effective.

Info on tests:
Opera has agreed to submit tests to the CSS-WG and the WG has agreed to keep these properties in if tests are submitted.
The layout test to validate the added properties is also a part of this patch.

BUG=

[esprehn, 2013-06-20 19:45:09]

This needs an intent to implement email and also tests.

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSCompute...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSCompute...
Source/core/css/CSSComputedStyleDeclaration.cpp:1415: static
PassRefPtr<CSSValue> navDirToCSSValue(const StyleNavigationValue& navValue)
navDirectionToCSSValue

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSGrammar...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSGrammar...
Source/core/css/CSSGrammar.y.in:1637: | IDSEL maybe_space { $$.id =
CSSValueInvalid; $$.string = $1; $$.unit = CSSPrimitiveValue::CSS_STRING; }
Hmm, why does the grammar change?

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSParser.cpp
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSParser....
Source/core/css/CSSParser.cpp:2307: return false;
It'd be nicer if you split out these cases more, It's code to have a little code
duplication (ex. for the Auto case and the num == 1 case in separate parts of
the if() else if () so you have three cases.

Also I think you're missing a return true?

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:2720:
(m_state.style()->*setter)(StyleNavigationValue(vals[0], vals[1]));
You should do this the other way around instead of using function pointers with
a separate switch, this switch statement should be:

case CSSPropertyNavDown:
  style->setNavDown(spatialNavigationValue(...));
  break;
case CSSPropertyNavUp:
  ...

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.h:406: static NavDirSetter
getNavDirSetterForProperty(CSSPropertyID);
I'm not a fan of this pattern. Why do you need to return a function pointer like
this?

The naming of this method is also wrong. setterForNavigationDirection() matches
the convention.

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/EventHand...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/EventHand...
Source/core/page/EventHandler.cpp:3461: return;
Hmm, why doesn't this need to do setDefaultHandled? Should this be inside
advanceFocus?

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/FocusCont...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/FocusCont...
Source/core/page/FocusController.cpp:879: Frame* curFrame =
focusedOrMainFrame();
frame, or currentFrame. No abbreviations.

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/FocusCont...
Source/core/page/FocusController.cpp:930: // TODO:// which one is right?
FIXME:, also you have extra //

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/RenderStyle.h:1638: static StyleNavigationValue
initialStyleNavigation() { DEFINE_STATIC_LOCAL(StyleNavigationValue, navi, ());
return navi; }
don't abbreviate, what is navi?

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
File Source/core/rendering/style/StyleNavigationValue.h (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleNavigationValue.h:30:
:m_id(AtomicString("auto"))
Missing space

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleNavigationValue.h:31: ,
m_target(AtomicString("current")) { }
You don't need the AtomicString here, just call the constructor with
m_target("current")

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleNavigationValue.h:33:
StyleNavigationValue(const char* id, const char* target)
String? We don't really use const char* like this.

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleNavigationValue.h:34: :m_id(AtomicString(id))
Same

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleRareNonInheritedData.h:127: StyleNavigationData
m_nav;
m_navigation ?

[Mostyn Bramley-Moore, 2013-06-21 21:16:58]

Adam Barth gave a thumbs-up on implementing (but not shipping) this:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/u2lKAP3EiU4/qRn4YTJr...

[Krzysztof Olczyk, 2013-07-22 14:14:15]

Thank you for the review.
I have rebased the diff to latest version and applied your suggestions.

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSCompute...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSCompute...
Source/core/css/CSSComputedStyleDeclaration.cpp:1415: static
PassRefPtr<CSSValue> navDirToCSSValue(const StyleNavigationValue& navValue)
On 2013/06/20 19:45:10, esprehn wrote:
> navDirectionToCSSValue

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSGrammar...
File Source/core/css/CSSGrammar.y.in (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSGrammar...
Source/core/css/CSSGrammar.y.in:1637: | IDSEL maybe_space { $$.id =
CSSValueInvalid; $$.string = $1; $$.unit = CSSPrimitiveValue::CSS_STRING; }
On 2013/06/20 19:45:10, esprehn wrote:
> Hmm, why does the grammar change?

In order to be able specify the target element, e.g.

nav-left: #someId;
nav-right: someframe #anotherId;

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSParser.cpp
File Source/core/css/CSSParser.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/CSSParser....
Source/core/css/CSSParser.cpp:2307: return false;
On 2013/06/20 19:45:10, esprehn wrote:
> It'd be nicer if you split out these cases more, It's code to have a little
code
> duplication (ex. for the Auto case and the num == 1 case in separate parts of
> the if() else if () so you have three cases.

Done
 
> Also I think you're missing a return true?

No, we need additional common handling at the end of this function.

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:2720:
(m_state.style()->*setter)(StyleNavigationValue(vals[0], vals[1]));
On 2013/06/20 19:45:10, esprehn wrote:
> You should do this the other way around instead of using function pointers
with
> a separate switch, this switch statement should be:
> 
> case CSSPropertyNavDown:
>   style->setNavDown(spatialNavigationValue(...));
>   break;
> case CSSPropertyNavUp:
>   ...

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.h (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.h:406: static NavDirSetter
getNavDirSetterForProperty(CSSPropertyID);
On 2013/06/20 19:45:10, esprehn wrote:
> I'm not a fan of this pattern. Why do you need to return a function pointer
like
> this?
> 
> The naming of this method is also wrong. setterForNavigationDirection()
matches
> the convention.

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/EventHand...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/EventHand...
Source/core/page/EventHandler.cpp:3461: return;
On 2013/06/20 19:45:10, esprehn wrote:
> Hmm, why doesn't this need to do setDefaultHandled? Should this be inside
> advanceFocus?

Just an omission. Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/FocusCont...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/FocusCont...
Source/core/page/FocusController.cpp:879: Frame* curFrame =
focusedOrMainFrame();
On 2013/06/20 19:45:10, esprehn wrote:
> frame, or currentFrame. No abbreviations.

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/page/FocusCont...
Source/core/page/FocusController.cpp:930: // TODO:// which one is right?
On 2013/06/20 19:45:10, esprehn wrote:
> FIXME:, also you have extra //

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/RenderStyle.h:1638: static StyleNavigationValue
initialStyleNavigation() { DEFINE_STATIC_LOCAL(StyleNavigationValue, navi, ());
return navi; }
On 2013/06/20 19:45:10, esprehn wrote:
> don't abbreviate, what is navi?

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
File Source/core/rendering/style/StyleNavigationValue.h (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleNavigationValue.h:30:
:m_id(AtomicString("auto"))
On 2013/06/20 19:45:10, esprehn wrote:
> Missing space

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleNavigationValue.h:31: ,
m_target(AtomicString("current")) { }
On 2013/06/20 19:45:10, esprehn wrote:
> You don't need the AtomicString here, just call the constructor with
> m_target("current")

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleNavigationValue.h:33:
StyleNavigationValue(const char* id, const char* target)
On 2013/06/20 19:45:10, esprehn wrote:
> String? We don't really use const char* like this.

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleNavigationValue.h:34: :m_id(AtomicString(id))
On 2013/06/20 19:45:10, esprehn wrote:
> Same

Done.

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/17450016/diff/6001/Source/core/rendering/styl...
Source/core/rendering/style/StyleRareNonInheritedData.h:127: StyleNavigationData
m_nav;
On 2013/06/20 19:45:10, esprehn wrote:
> m_navigation ?

Done.

[Krzysztof Olczyk, 2013-07-24 07:13:03]

@esprehn, could you please take a look at my changes related to your comments?

[Krzysztof Olczyk, 2013-08-02 13:14:50]

Hi reviewers,

I have added tests to this feature.

[esprehn, 2013-08-08 03:39:40]

https://codereview.chromium.org/17450016/diff/52001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:2304: return false;
indent is wrong

https://codereview.chromium.org/17450016/diff/52001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/css/resolver/...
Source/core/css/resolver/StyleBuilderCustom.cpp:1245: StyleNavigationValue
navigationValue;
This should be handled by individual functions instead of using this old
deprecated method with the switch

https://codereview.chromium.org/17450016/diff/52001/Source/core/page/FocusCon...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:935: Element* anchor =
targetFrame->document()->findAnchor(value.id().string());
The spec doesn't seem to say we should find elements by name attribute in quirks
mode documents. This should getElementById(value.id())

https://codereview.chromium.org/17450016/diff/52001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:942: anchor->scrollIntoViewIfNeeded(false);
you shouldn't have to do this manually, doesn't the focus code do it for you?
Also you should do this _after_ calling setFocusedElement, since if it can't be
successfully focused you'll still scroll.

https://codereview.chromium.org/17450016/diff/52001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:946:
m_page->chrome().focusedNodeChanged(anchor);
This is done inside Document::setFocusedElement, I think you should just call
out to that instead.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleNavigationData.h (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:29: friend class RenderStyle;
RenderStyle should not need to be a friend

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:31: StyleNavigationData() { }
Leave off

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:37: , m_right(o.m_right)
indent is wrong.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleNavigationValue.h (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationValue.h:27: friend class RenderStyle;
no friends, this isn't needed

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationValue.h:31: , m_target("current") { }
wrong indent, and the braces are on the wrong line

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationValue.h:35: , m_target(target)
wrong indent

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationValue.h:40: return m_id == "auto";
This means you can't do #auto in the css?

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.cpp (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.cpp:43: , m_navigation()
leave these off, the default constructor does it for you

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.h:126: StyleNavigationData
m_navigation;
This adds 8 ptrs to this thing, I think you want OwnPtr<StyleNavigationData>

[Krzysztof Olczyk, 2013-12-04 13:56:50]

Hi esprehn,

after long time, I've finally come back to this CL.
Can you please take a look at them?
Basically, addressed your comments and changed the value application to use
function instead of a big-switch case.

https://codereview.chromium.org/17450016/diff/52001/Source/core/css/CSSParser...
File Source/core/css/CSSParser-in.cpp (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/css/CSSParser...
Source/core/css/CSSParser-in.cpp:2304: return false;
On 2013/08/08 03:39:40, esprehn wrote:
> indent is wrong

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/css/resolver/...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/css/resolver/...
Source/core/css/resolver/StyleBuilderCustom.cpp:1245: StyleNavigationValue
navigationValue;
On 2013/08/08 03:39:40, esprehn wrote:
> This should be handled by individual functions instead of using this old
> deprecated method with the switch

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/page/FocusCon...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:935: Element* anchor =
targetFrame->document()->findAnchor(value.id().string());
On 2013/08/08 03:39:40, esprehn wrote:
> The spec doesn't seem to say we should find elements by name attribute in
quirks
> mode documents. This should getElementById(value.id())

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:942: anchor->scrollIntoViewIfNeeded(false);
On 2013/08/08 03:39:40, esprehn wrote:
> you shouldn't have to do this manually, doesn't the focus code do it for you?
> Also you should do this _after_ calling setFocusedElement, since if it can't
be
> successfully focused you'll still scroll.

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:946:
m_page->chrome().focusedNodeChanged(anchor);
On 2013/08/08 03:39:40, esprehn wrote:
> This is done inside Document::setFocusedElement, I think you should just call
> out to that instead.

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleNavigationData.h (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:29: friend class RenderStyle;
On 2013/08/08 03:39:40, esprehn wrote:
> RenderStyle should not need to be a friend

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:31: StyleNavigationData() { }
On 2013/08/08 03:39:40, esprehn wrote:
> Leave off

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:37: , m_right(o.m_right)
On 2013/08/08 03:39:40, esprehn wrote:
> indent is wrong.

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleNavigationValue.h (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationValue.h:27: friend class RenderStyle;
On 2013/08/08 03:39:40, esprehn wrote:
> no friends, this isn't needed

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationValue.h:31: , m_target("current") { }
On 2013/08/08 03:39:40, esprehn wrote:
> wrong indent, and the braces are on the wrong line

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationValue.h:35: , m_target(target)
On 2013/08/08 03:39:40, esprehn wrote:
> wrong indent

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationValue.h:40: return m_id == "auto";
On 2013/08/08 03:39:40, esprehn wrote:
> This means you can't do #auto in the css?

Fixed.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.cpp (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.cpp:43: , m_navigation()
On 2013/08/08 03:39:40, esprehn wrote:
> leave these off, the default constructor does it for you

Done.

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/17450016/diff/52001/Source/core/rendering/sty...
Source/core/rendering/style/StyleRareNonInheritedData.h:126: StyleNavigationData
m_navigation;
On 2013/08/08 03:39:40, esprehn wrote:
> This adds 8 ptrs to this thing, I think you want OwnPtr<StyleNavigationData>

Done.

[Krzysztof Olczyk, 2014-04-07 10:36:19]

Hi esprehn & tkent,

could you please take a look at the CL, I've rebased again?
I would love to get it landed before it rots again.

[Krzysztof Olczyk, 2014-04-14 06:59:14]

On 2014/04/07 10:36:19, Krzysztof Olczyk wrote:
> Hi esprehn & tkent,
> 
> could you please take a look at the CL, I've rebased again?
> I would love to get it landed before it rots again.

Passing this review to fs@, as suggested by tkent, off-channel.

[fs, 2014-04-14 12:32:00]

I think you want to edit the description to be less "excusive", and more about
what's implemented (and not.)

I.e. AFAICS nav-* are no longer (in the ED) marked as "at risk" (but that could
of course change); AFAIK we (Opera) also have submitted tests (but they're not
yet "accepted"). Where you say "(not submitted by us)" about the original patch,
it might be more appropriate to credit the original author? I think you get the
idea.

https://codereview.chromium.org/17450016/diff/93001/LayoutTests/fast/spatial-...
File LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html (right):

https://codereview.chromium.org/17450016/diff/93001/LayoutTests/fast/spatial-...
LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html:58: <p
id="description">
You could use js-test framework's description(...) for this.

https://codereview.chromium.org/17450016/diff/93001/LayoutTests/fast/spatial-...
LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html:80: <div
id="console"></div>
The test framework should provide this too.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:7: * Copyright (C) 2013 Opera
Software ASA. All rights reserved.
Update year. (In all files where it's been added.)

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1370: static
PassRefPtr<CSSValue> valueForNavigationDirection(const RenderStyle& style,
CSSPropertyID propertyID)
The return type should now be PassRefPtrWillBeRawPtr<CSSValue> (for Oilpan
compat.)

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1393: RefPtr<CSSValueList> list
= CSSValueList::createSpaceSeparated();
...WillBeRawPtr (like above.)

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1394:
list->append(cssValuePool().createValue(navValue.id(),
CSSPrimitiveValue::CSS_STRING));
Spec says: "Computed value: as specified", so I'd expect the possibility of
'auto' here. Maybe add some basic tests for this?

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSProper...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSProper...
Source/core/css/CSSProperties.in:86: nav-down custom_all
Could you perhaps try to make these less "custom" - I think it should be
possible to only have custom_value (although a converter -> NavigationData ought
to work as well there I think.)

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
Source/core/css/parser/CSSPropertyParser.cpp:1075: } else if (num == 1 &&
(value->unit == CSSPrimitiveValue::CSS_STRING || value->unit ==
CSSPrimitiveValue::CSS_PARSER_HEXCOLOR)) {
This would accept "foo" (a literal string) as well. Maybe a CSS_PARSER_IDSEL
type is required.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
Source/core/css/parser/CSSPropertyParser.cpp:1076: value->unit =
CSSPrimitiveValue::CSS_STRING;
Probably better to avoid mutating |value| and just create the (string)value up
front.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
Source/core/css/parser/CSSPropertyParser.cpp:1079: RefPtr<CSSValueList> list =
CSSValueList::createSpaceSeparated();
...WillBeRawPtr

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
Source/core/css/parser/CSSPropertyParser.cpp:1081:
list->append(cssValuePool().createValue(value->string,
CSSPrimitiveValue::CSS_STRING));
This looks a bit lenient.

https://codereview.chromium.org/17450016/diff/93001/Source/core/frame/UseCoun...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/frame/UseCoun...
Source/core/frame/UseCounter.cpp:512: case CSSPropertyNavRight: return 456;
Why leave out 455?

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:927: DEFINE_STATIC_LOCAL(AtomicString,
Current, ("current"));
Maybe make this 'targetCurrent' or 'frameTargetCurrent' (and similarly for
'Root') or similar (i.e. a tad more specific.)

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:934: ASSERT(currentFrame);
Already dereference focusedOrMainFrame() above.

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:971: if (value.id().isNull() ||
value.isAuto())
When will id() be the nullAtom?

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:994: // If it's same with the current
focused, it should be consumed.
s/same with the current/the same as the currently/, and maybe "consumed" ->
"considered a valid navigation" or somesuch.

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyle.h:1348: void setNavigation(const
StyleNavigationData&);
I think it might end up easier to just have one setter for each property and
then use SET_VAR_WITH_SETTER and handle the potential NULL value in the
getter/setter on StyleRareNonInheritedData (this is related to the comment in
CSSProperties.in.)

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyle.h:1695: static StyleNavigationValue
initialStyleNavigation() { DEFINE_STATIC_LOCAL(StyleNavigationValue,
initial_navigation, ()); return initial_navigation; }
initial_navigation -> initialNavigation

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleNavigationData.h (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:38:
StyleNavigationData(NavigationDirection direction, const StyleNavigationValue&
value)
Does anything use this constructor? (Looks a bit dangerous, so hopefully not.)

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:70: void
SetProperty(NavigationDirection, const StyleNavigationValue&);
setProperty (but hopefully this won't be needed in the end.)

[Krzysztof Olczyk, 2014-04-17 13:48:39]

Hi fs,

I've applied your suggestions. Could you please take a look at CL again?

https://codereview.chromium.org/17450016/diff/93001/LayoutTests/fast/spatial-...
File LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html (right):

https://codereview.chromium.org/17450016/diff/93001/LayoutTests/fast/spatial-...
LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html:58: <p
id="description">
On 2014/04/14 12:32:01, fs wrote:
> You could use js-test framework's description(...) for this.

But you still need physical <div>, don't you?
I was also encouraged to use this way in other reviews.

https://codereview.chromium.org/17450016/diff/93001/LayoutTests/fast/spatial-...
LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html:80: <div
id="console"></div>
On 2014/04/14 12:32:01, fs wrote:
> The test framework should provide this too.

On 2014/04/14 12:32:01, fs wrote:
> You could use js-test framework's description(...) for this.

But you still need physical <div>, don't you?
I was also encouraged to use this way in other reviews.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:7: * Copyright (C) 2013 Opera
Software ASA. All rights reserved.
On 2014/04/14 12:32:01, fs wrote:
> Update year. (In all files where it's been added.)

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1370: static
PassRefPtr<CSSValue> valueForNavigationDirection(const RenderStyle& style,
CSSPropertyID propertyID)
On 2014/04/14 12:32:01, fs wrote:
> The return type should now be PassRefPtrWillBeRawPtr<CSSValue> (for Oilpan
> compat.)

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1393: RefPtr<CSSValueList> list
= CSSValueList::createSpaceSeparated();
On 2014/04/14 12:32:01, fs wrote:
> ...WillBeRawPtr (like above.)

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSComput...
Source/core/css/CSSComputedStyleDeclaration.cpp:1394:
list->append(cssValuePool().createValue(navValue.id(),
CSSPrimitiveValue::CSS_STRING));
On 2014/04/14 12:32:01, fs wrote:
> Spec says: "Computed value: as specified", so I'd expect the possibility of
> 'auto' here. Maybe add some basic tests for this?

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSProper...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/CSSProper...
Source/core/css/CSSProperties.in:86: nav-down custom_all
On 2014/04/14 12:32:01, fs wrote:
> Could you perhaps try to make these less "custom" - I think it should be
> possible to only have custom_value (although a converter -> NavigationData
ought
> to work as well there I think.)

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
Source/core/css/parser/CSSPropertyParser.cpp:1075: } else if (num == 1 &&
(value->unit == CSSPrimitiveValue::CSS_STRING || value->unit ==
CSSPrimitiveValue::CSS_PARSER_HEXCOLOR)) {
On 2014/04/14 12:32:01, fs wrote:
> This would accept "foo" (a literal string) as well. Maybe a CSS_PARSER_IDSEL
> type is required.

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
Source/core/css/parser/CSSPropertyParser.cpp:1076: value->unit =
CSSPrimitiveValue::CSS_STRING;
On 2014/04/14 12:32:01, fs wrote:
> Probably better to avoid mutating |value| and just create the (string)value up
> front.

This is basically what CSS_PARSER_* primitive values are for - to be later
mutated to correct type.
We can't resolve, at syntax parsing, whether it's IDSEL or HEXCOLOR. We need a
context to determine that.
#deadbeaf will denote the hex number of color in background-color property, but
will denote <div id="deadbeaf"> element in nav-up property.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
Source/core/css/parser/CSSPropertyParser.cpp:1079: RefPtr<CSSValueList> list =
CSSValueList::createSpaceSeparated();
On 2014/04/14 12:32:01, fs wrote:
> ...WillBeRawPtr

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
Source/core/css/parser/CSSPropertyParser.cpp:1081:
list->append(cssValuePool().createValue(value->string,
CSSPrimitiveValue::CSS_STRING));
On 2014/04/14 12:32:01, fs wrote:
> This looks a bit lenient.

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/frame/UseCoun...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/frame/UseCoun...
Source/core/frame/UseCounter.cpp:512: case CSSPropertyNavRight: return 456;
On 2014/04/14 12:32:01, fs wrote:
> Why leave out 455?

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
File Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:927: DEFINE_STATIC_LOCAL(AtomicString,
Current, ("current"));
On 2014/04/14 12:32:01, fs wrote:
> Maybe make this 'targetCurrent' or 'frameTargetCurrent' (and similarly for
> 'Root') or similar (i.e. a tad more specific.)

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:934: ASSERT(currentFrame);
On 2014/04/14 12:32:01, fs wrote:
> Already dereference focusedOrMainFrame() above.

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:971: if (value.id().isNull() ||
value.isAuto())
On 2014/04/14 12:32:01, fs wrote:
> When will id() be the nullAtom?

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/page/FocusCon...
Source/core/page/FocusController.cpp:994: // If it's same with the current
focused, it should be consumed.
On 2014/04/14 12:32:01, fs wrote:
> s/same with the current/the same as the currently/, and maybe "consumed" ->
> "considered a valid navigation" or somesuch.

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyle.h:1348: void setNavigation(const
StyleNavigationData&);
On 2014/04/14 12:32:01, fs wrote:
> I think it might end up easier to just have one setter for each property and
> then use SET_VAR_WITH_SETTER and handle the potential NULL value in the
> getter/setter on StyleRareNonInheritedData (this is related to the comment in
> CSSProperties.in.)

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
Source/core/rendering/style/RenderStyle.h:1695: static StyleNavigationValue
initialStyleNavigation() { DEFINE_STATIC_LOCAL(StyleNavigationValue,
initial_navigation, ()); return initial_navigation; }
On 2014/04/14 12:32:01, fs wrote:
> initial_navigation -> initialNavigation

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
File Source/core/rendering/style/StyleNavigationData.h (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:38:
StyleNavigationData(NavigationDirection direction, const StyleNavigationValue&
value)
On 2014/04/14 12:32:01, fs wrote:
> Does anything use this constructor? (Looks a bit dangerous, so hopefully not.)

Done.

https://codereview.chromium.org/17450016/diff/93001/Source/core/rendering/sty...
Source/core/rendering/style/StyleNavigationData.h:70: void
SetProperty(NavigationDirection, const StyleNavigationValue&);
On 2014/04/14 12:32:01, fs wrote:
> setProperty (but hopefully this won't be needed in the end.)

Done.

[fs, 2014-04-17 15:33:47]

Still missing tests for parsing/serialization and maybe there a should be test
for target-frame navigation?

https://codereview.chromium.org/17450016/diff/93001/LayoutTests/fast/spatial-...
File LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html (right):

https://codereview.chromium.org/17450016/diff/93001/LayoutTests/fast/spatial-...
LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html:58: <p
id="description">
On 2014/04/17 13:48:40, Krzysztof Olczyk wrote:
> On 2014/04/14 12:32:01, fs wrote:
> > You could use js-test framework's description(...) for this.
> 
> But you still need physical <div>, don't you?
> I was also encouraged to use this way in other reviews.

The framework will create the elements as needed (and insert into the body.) I
guess it's not crucial though.

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/17450016/diff/93001/Source/core/css/parser/CS...
Source/core/css/parser/CSSPropertyParser.cpp:1076: value->unit =
CSSPrimitiveValue::CSS_STRING;
On 2014/04/17 13:48:40, Krzysztof Olczyk wrote:
> On 2014/04/14 12:32:01, fs wrote:
> > Probably better to avoid mutating |value| and just create the (string)value
up
> > front.
> 
> This is basically what CSS_PARSER_* primitive values are for - to be later
> mutated to correct type.
> We can't resolve, at syntax parsing, whether it's IDSEL or HEXCOLOR. We need a
> context to determine that.
> #deadbeaf will denote the hex number of color in background-color property,
but
> will denote <div id="deadbeaf"> element in nav-up property.

Sure, it's needed for disambiguation, but that doesn't mean it by necessity
should turn into a string - hopefully this point is mute though with other
changes.

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1395:
list->append(cssValuePool().createValue(navValue.id(),
CSSPrimitiveValue::CSS_STRING));
What does this end up serializing to?

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/CSSGramm...
File Source/core/css/CSSGrammar.y (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/CSSGramm...
Source/core/css/CSSGrammar.y:9: *  Copyright (C) 2013 Opera Software ASA. All
rights reserved.
Missed a spot =)

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1069: case CSSPropertyNavDown: //
auto | <id> targetframe | inherit
"auto | <id> [ current | root | <target-name> ]? | inherit"

is what the current ED says.

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1080: value->unit =
CSSPrimitiveValue::CSS_STRING;
Now I just think that this is unnecessary - what actually needs this mutation?

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1083: if (value && value->unit ==
CSSPrimitiveValue::CSS_STRING) {
How about the current and root keywords? (Also consider how "current" (the
string) differs from current (the identifier/keyword.))

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1084: if
(value->string.startsWithIgnoringCase("_"))
What's the other case version of _? =P

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/resolver...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderCustom.cpp:81: #include
"core/rendering/style/StyleNavigationValue.h"
Still needed?

https://codereview.chromium.org/17450016/diff/133001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:5: * Copyright (C) 2014 Opera
Software ASA. All rights reserved.
Should probably be dropped now. (I don't see any changes in this file.)

[Krzysztof Olczyk, 2014-09-30 15:41:47]

Hi Fredrik,

could you please take a look at my new changes? It took a lot of time, but I
finally have had a time to address the comments you raised and add few
improvements.

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1395:
list->append(cssValuePool().createValue(navValue.id(),
CSSPrimitiveValue::CSS_STRING));
On 2014/04/17 at 15:33:48, fs wrote:
> What does this end up serializing to?

Right, it would have been converted to string without #.
Fixed it.

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/CSSGramm...
File Source/core/css/CSSGrammar.y (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/CSSGramm...
Source/core/css/CSSGrammar.y:9: *  Copyright (C) 2013 Opera Software ASA. All
rights reserved.
On 2014/04/17 15:33:48, fs wrote:
> Missed a spot =)

Done.

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1069: case CSSPropertyNavDown: //
auto | <id> targetframe | inherit
On 2014/04/17 15:33:48, fs wrote:
> "auto | <id> [ current | root | <target-name> ]? | inherit"
> 
> is what the current ED says.

Done.

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1080: value->unit =
CSSPrimitiveValue::CSS_STRING;
On 2014/04/17 15:33:48, fs wrote:
> Now I just think that this is unnecessary - what actually needs this mutation?

Because:
1) CSS_PARSER_HEXCOLOR won't be a nice type of nav-up property :)
2) As far as I understand the CSS code in Blink, all CSS_PARSER_ types should
not trespass this point,
   as those are only the auxiliary types for this kind of disambiguation.

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1083: if (value && value->unit ==
CSSPrimitiveValue::CSS_STRING) {
On 2014/04/17 at 15:33:48, fs wrote:
> How about the current and root keywords? (Also consider how "current" (the
string) differs from current (the identifier/keyword.))

Right. Done.

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1084: if
(value->string.startsWithIgnoringCase("_"))
On 2014/04/17 15:33:48, fs wrote:
> What's the other case version of _? =P

:)

I just can't see other non-case-ignoring version of that function in
CSSParserString class,
and as it makes no difference for _, I can't see a sense of converting it to
WTF::String.

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/resolver...
File Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderCustom.cpp:81: #include
"core/rendering/style/StyleNavigationValue.h"
On 2014/04/17 15:33:48, fs wrote:
> Still needed?

Done.

https://codereview.chromium.org/17450016/diff/133001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.cpp (right):

https://codereview.chromium.org/17450016/diff/133001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.cpp:5: * Copyright (C) 2014 Opera
Software ASA. All rights reserved.
On 2014/04/17 15:33:48, fs wrote:
> Should probably be dropped now. (I don't see any changes in this file.)

Done.

[Krzysztof Olczyk, 2014-09-30 15:41:48]

Hi Fredrik,

could you please take a look at my new changes? It took a lot of time, but I
finally have had a time to address the comments you raised and add few
improvements.

[fs, 2014-10-01 10:52:15]

https://codereview.chromium.org/17450016/diff/153001/LayoutTests/fast/spatial...
File LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html (right):

https://codereview.chromium.org/17450016/diff/153001/LayoutTests/fast/spatial...
LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html:27: function
testParsingAndSerialization()
There's no need to stuff all the tests in one file, so please move this to a
separate file. I also don't think that testing the CSSOM (CSSValue) structure is
that interesting. You can look at for instance LayoutTests/css-parser for a
simple framework for this.

https://codereview.chromium.org/17450016/diff/153001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/17450016/diff/153001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:1410:
selectorText.append(navValue.id());
String selectorText = "#" + navValue.id();

https://codereview.chromium.org/17450016/diff/153001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/17450016/diff/153001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1089: if
(value->string.startsWithIgnoringCase("_"))
Please add a short comment about this (since it's just mentioned in spec-prose
it's not obivous from the "grammar" excerpt above.)

https://codereview.chromium.org/17450016/diff/153001/Source/core/css/resolver...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/17450016/diff/153001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderConverter.cpp:624: if
(idSelector.startsWith('#'))
When does this string _not_ start with '#' when you get here? I.e. isn't that
supposed to be the canonical form?

So:

ASSERT(idSelector.startsWith('#'));
AtomicString id = idSelector.substring(1);

https://codereview.chromium.org/17450016/diff/153001/Source/core/page/FocusCo...
File Source/core/page/FocusController.h (right):

https://codereview.chromium.org/17450016/diff/153001/Source/core/page/FocusCo...
Source/core/page/FocusController.h:3: * Copyright (C) 2013, 2014 Opera Software
ASA. All rights reserved.
I don't think we can claim any copyright for 2013 =) (but IANAL)

https://codereview.chromium.org/17450016/diff/153001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/17450016/diff/153001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:1701: // The initial value is 'none'
for grid trac  ks.
Stray spaces added.

https://codereview.chromium.org/17450016/diff/153001/Source/core/rendering/st...
File Source/core/rendering/style/StyleNavigationValue.h (right):

https://codereview.chromium.org/17450016/diff/153001/Source/core/rendering/st...
Source/core/rendering/style/StyleNavigationValue.h:33: : m_auto(1)
Use 'true' here instead. (And similar for other instances below.)

https://codereview.chromium.org/17450016/diff/153001/Source/core/rendering/st...
Source/core/rendering/style/StyleNavigationValue.h:37: explicit
StyleNavigationValue(const String& id, ENavigationTarget target = Current)
Just pass AtomicString directly here (and below).

https://codereview.chromium.org/17450016/diff/153001/Source/core/rendering/st...
Source/core/rendering/style/StyleNavigationValue.h:72: unsigned m_auto : 1;
m_isAuto

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/CSSPrope...
File Source/core/css/CSSProperties.in (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/CSSPrope...
Source/core/css/CSSProperties.in:215: nav-down type_name=StyleNavigationValue,
initial=initialStyleNavigation, converter=convertStyleNavigationValue
Might want to consider adding a runtime feature for these properties (in
addition to the setting...) to prevent exposing these even when will not
actually work - for things like @supports(nav-...) etc. (Read: I'm not sure if
other users of Chromium will be happy about exposing these properties.)

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/resolver...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderConverter.cpp:36: #include
"core/css/CSSValuePool.h"
This include looks odd here - I don't see anything that needs/uses the value
pool?

https://codereview.chromium.org/17450016/diff/173001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyleConstants.h (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyleConstants.h:162: enum ENavigationTarget {
Current = 0, Root = 1, TargetName = 2 };
I think you can drop the explicit values, because AFAICS you don't rely on them
in any way.

https://codereview.chromium.org/17450016/diff/173001/Source/core/rendering/st...
File Source/core/rendering/style/StyleNavigationData.cpp (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/rendering/st...
Source/core/rendering/style/StyleNavigationData.cpp:2: * Copyright (C) 2014
Opera Software ASA. All rights reserved.
Use the "new" license blurb.

https://codereview.chromium.org/17450016/diff/173001/Source/core/rendering/st...
Source/core/rendering/style/StyleNavigationData.cpp:25: #include
"wtf/Assertions.h"
Unused?

[rune, 2014-10-01 11:04:17]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2014-10-01 11:04:18]

This CL adds new web-facing properties. It should be supported by an
intent-to-implement + a runtime flag before it's ready for shipping.

Also, this CL should be split into smaller CLs for faster reviews.

[Krzysztof Olczyk, 2014-10-01 11:29:47]

On 2014/10/01 at 11:04:18, rune wrote:
> This CL adds new web-facing properties. It should be supported by an
intent-to-implement + a runtime flag before it's ready for shipping.
> 
> Also, this CL should be split into smaller CLs for faster reviews.

There was an intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/u2lKAP3EiU4 
It's effective if --enable-spatial-navigation command flag is present, but I
will add a runtime flag also not to expose new CSS properties as fs suggested.

[rune, 2014-10-01 11:53:21]

On 2014/10/01 at 11:29:47, kolczyk wrote:
> On 2014/10/01 at 11:04:18, rune wrote:
> > This CL adds new web-facing properties. It should be supported by an
intent-to-implement + a runtime flag before it's ready for shipping.
> > 
> > Also, this CL should be split into smaller CLs for faster reviews.
> 
> There was an intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/u2lKAP3EiU4 
> It's effective if --enable-spatial-navigation command flag is present, but I
will add a runtime flag also not to expose new CSS properties as fs suggested.

Add that link to the description of the CL introducing the runtime flag.

[Timothy Loh, 2014-10-01 13:14:52]

timloh@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2014-10-01 13:14:54]

I only looked over the CSS parts since I'm not at all familiar with the
FocusController code.

https://codereview.chromium.org/17450016/diff/173001/LayoutTests/fast/spatial...
File LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html (right):

https://codereview.chromium.org/17450016/diff/173001/LayoutTests/fast/spatial...
LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html:29: e5_navUp =
window.getComputedStyle(document.getElementById("e5")).getPropertyCSSValue('nav-up');
getPropertyCSSValue and the CSSValue interface are deprecated and on their way
out, I wouldn't use them in new tests.

Some example tests that you might want (using
css-parser/resources/property-parsing-test.js):
assert_valid_value("nav-left", "auto");
assert_valid_value("nav-up", "#a", "#a current");
assert_valid_value("nav-right", "#bb current");
assert_valid_value("nav-down", "#ccc root");
assert_valid_value("nav-left", '#dd "hello world!"');

assert_invalid_value("nav-left", "#x bar");
assert_invalid_value("nav-up", "#x current 123");
assert_invalid_value("nav-right", "# current");

x.style.navLeft = "#a root"; assert_equals(getComputedStyle(x).navLeft, "#a
root");

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:7: * Copyright (C) 2014 Opera
Software ASA. All rights reserved.
I haven't seen these headers changed in a long time, maybe worthwhile to check
with other Opera employees if you haven't already.

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:415: case CSSPropertyNavDown:
This is unreachable. We'd only get here is these values were in
isKeywordPropertyID (which the nav-* properties aren't!)

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1076: } else if (value) {
value is never null here.

I would move this logic into a separate function (something like
PassRefPtrWillBeRawPtr<CSSValue> parseNav(), which returns nullptr if it fails
to parse). Then this branch would just be "parsedValue = parseNav()".

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1087: target =
parseValidPrimitive(value->id, value);
Don't you want to check if the id is CSSValueCurrent or CSSValueRoot?

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1092: }
You'll want to call m_valueList->next() here to indicate you've consumed the
current token. The code outside the huge switch checks there are no trailing
tokens.

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:1100: addProperty(propId,
propertyValue, important);
Don't addProperty, just break after setting parsedValue.

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/resolver...
File Source/core/css/resolver/StyleBuilderConverter.cpp (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderConverter.cpp:617: if
(!value->isValueList())
I like asserting that the value is what we expect (auto) in these cases.

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderConverter.cpp:621:
ASSERT(valueList->length() == 1 || valueList->length() == 2);
When is the length ever 1?

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/resolver...
Source/core/css/resolver/StyleBuilderConverter.cpp:628: return
StyleNavigationValue(idSelector);
As above; this seems unreachable to me.

https://codereview.chromium.org/17450016/diff/173001/Source/core/rendering/st...
File Source/core/rendering/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/rendering/st...
Source/core/rendering/style/StyleRareNonInheritedData.h:40: #include
"core/rendering/style/StyleNavigationData.h"
Can we just use a forward declaration here?

[rune, 2014-10-01 13:40:49]

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/CSSCompu...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/17450016/diff/173001/Source/core/css/CSSCompu...
Source/core/css/CSSComputedStyleDeclaration.cpp:7: * Copyright (C) 2014 Opera
Software ASA. All rights reserved.
On 2014/10/01 13:14:53, Timothy Loh wrote:
> I haven't seen these headers changed in a long time, maybe worthwhile to check
> with other Opera employees if you haven't already.

We shouldn't change it. Add the generic Chromium copyright/license reference in
new files and leave old ones unchanged.

[rune, 2014-12-17 10:34:23]

Is still being worked on?

[Julien Isorce Samsung, 2016-04-15 15:27:02]

On 2014/12/17 10:34:23, rune wrote:
> Is still being worked on?

Hi Krzysztof, do you plan to continue that work ? If not, any reason ?

[Anton Obzhirov, 2016-04-25 12:54:52]

On 2016/04/15 15:27:02, j.isorce wrote:
> On 2014/12/17 10:34:23, rune wrote:
> > Is still being worked on?
> 
> Hi Krzysztof, do you plan to continue that work ? If not, any reason ?

Hi guys,

We need this feature for our products so I've rebased the patch.
Since I couldn't submitted the patch here I created new issue:
https://codereview.chromium.org/1919813002
.
Please have a look.