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 ComputedStyle 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

It is based on the patch submitted by Krzysztof Olczyk
https://codereview.chromium.org/17450016

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.

R=holte@chromium.org, isherman@chromium.org, rune@opera.com, timloh@chromium.org, esprehn@chromium.org, fs@opera.com, tkent@chromium.org, kolczyk@opera.com

[Ilya Sherman, 2016-05-17 18:21:35]

Is this CL ready for review?  Please note that you need to "Publish+Mail
Comments" to alert reviewers that you'd like a review -- it's a somewhat odd
quirk of the Rietveld codereview system.

[Anton Obzhirov, 2016-05-18 09:55:39]

On 2016/05/17 18:21:35, Ilya Sherman wrote:
> Is this CL ready for review?  Please note that you need to "Publish+Mail
> Comments" to alert reviewers that you'd like a review -- it's a somewhat odd
> quirk of the Rietveld codereview system.

Hi Ilya, thanks for hint - I didn't know that!
Yes it is, plz have a look.

[Anton Obzhirov, 2016-05-18 09:57:19]

Hi guys,
I rebased the original patch and updated the test slightly. Plz have a look.

[Anton Obzhirov, 2016-05-18 10:07:33]

Description was changed from

==========
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 ComputedStyle 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

It is based on the patch submitted by Krzysztof Olczyk
https://codereview.chromium.org/17450016

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.

R=holte@chromium.org, isherman@chromium.org, rune@opera.com,
timloh@chromium.org, esprehn@chromium.org, fs@opera.com, tkent@chromium.org,
kolczyk@opera.com
BUG=
==========

to

==========
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 ComputedStyle 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

It is based on the patch submitted by Krzysztof Olczyk
https://codereview.chromium.org/17450016

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.

R=holte@chromium.org, isherman@chromium.org, rune@opera.com,
timloh@chromium.org, esprehn@chromium.org, fs@opera.com, tkent@chromium.org,
kolczyk@opera.com
BUG=
==========

[fs, 2016-05-18 15:52:16]

Description was changed from

==========
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 ComputedStyle 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

It is based on the patch submitted by Krzysztof Olczyk
https://codereview.chromium.org/17450016

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.

R=holte@chromium.org, isherman@chromium.org, rune@opera.com,
timloh@chromium.org, esprehn@chromium.org, fs@opera.com, tkent@chromium.org,
kolczyk@opera.com
BUG=
==========

to

==========
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 ComputedStyle 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

It is based on the patch submitted by Krzysztof Olczyk
https://codereview.chromium.org/17450016

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.

R=holte@chromium.org, isherman@chromium.org, rune@opera.com,
timloh@chromium.org, esprehn@chromium.org, fs@opera.com, tkent@chromium.org,
kolczyk@opera.com
==========

[fs, 2016-05-18 15:52:51]

Description was changed from

==========
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 ComputedStyle 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

It is based on the patch submitted by Krzysztof Olczyk
https://codereview.chromium.org/17450016

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.

R=holte@chromium.org, isherman@chromium.org, rune@opera.com,
timloh@chromium.org, esprehn@chromium.org, fs@opera.com, tkent@chromium.org,
kolczyk@opera.com
==========

to

==========
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 ComputedStyle 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

It is based on the patch submitted by Krzysztof Olczyk
https://codereview.chromium.org/17450016

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.

R=holte@chromium.org, isherman@chromium.org, rune@opera.com,
timloh@chromium.org, esprehn@chromium.org, fs@opera.com, tkent@chromium.org,
kolczyk@opera.com
==========

[fs, 2016-05-18 15:55:06]

(Re-formatted the description/subject a bit.)

Please check the old CL for any unresolved issues that could still apply (adding
a runtime enabled feature is one such issue IIRC.)

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html
(right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/spatial-navigation/snav-css-nav-direction.html:27:
function testParsingAndSerialization()
Use a separate test for parsing/serializing. LayoutTests/css-parser/ has some
helpers that should make this easy IIRC.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:1329: static
RawPtr<CSSValue> valueForNavigationDirection(const ComputedStyle& style,
CSSPropertyID propertyID)
RawPtr<T> is no more. Use T* here and in all other places in the CL.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:1356:
RawPtr<CSSValue> idSel = CSSStringValue::create(selectorText);
This should be a hash (not a string) - doesn't look like this serializes
correctly (doesn't round-trip.)

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1416: if (id ==
CSSValueAuto)
range.peek().id() == ...

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1419: String
selectorText = "";
Don't need to initialize this, it'll be overwritten below.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1421: if (type
== HashToken) {
Should only accept HashToken here or?

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1431:
RawPtr<CSSValue> idSel = CSSStringValue::create(selectorText);
idSelector

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1439: if
(targetText.startsWith("_"))
The way I'm reading the spec, this should not be a "hard" error (fail parsing):

"Error handling: if it does start with an underscore, "_parent" navigates to the
parent frame, "_root" is treated as root, and other values navigate to a frame
by that name if it exists. If the specified target frame does not exist, the
parameter will be treated as the keyword current, which means to simply use the
frame that the element is in."

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:1442: } else if
(type == EOFToken) {
Maybe check range.atEnd() instead.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:983:
ASSERT(list->length() == 1 || list->length() == 2);
list.length()

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:985:
ASSERT(value.isStringValue());
|value| will be a CSSValueList here, so you probably mean
list.item(0)->isStringValue() here.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:987: if
(idSelector.startsWith('#'))
The CSS parser should be providing a canonical enough representation that this
shouldn't be needed. (I.e either there's either always a '#' or never a '#'.)

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:993: if
(value.isStringValue())
Like above, presumably this should be list.item(1)->isStringValue()

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:996:
ASSERT(value.isPrimitiveValue() && (toCSSPrimitiveValue(value).getValueID() ==
CSSValueRoot || toCSSPrimitiveValue(value).getValueID() == CSSValueCurrent));
Again, |value| is still the parameter (the list). (Some additional local
variables would probably make sense here.)

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounter.cpp:572: 
Drop dangling vspace.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.cpp:1322: bool
FocusController::advanceCSSNavigationFocus(WebFocusType type)
Could this be refactored into something that advanceFocusDirectionally(...)
could call? (And do away with most of the entry-checks...)

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.cpp:1342: case
WebFocusTypeBackward:
We should never see these in this code-path IIRC, so these could go with
WebFocusTypeNone below.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.cpp:1369: Frame* targetFrame
= 0;
nullptr

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/page/FocusController.cpp:1391: //    return
setFocusedElement(targetElement, targetFrame);
Remove.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyle.h:961: StyleNavigationValue
navUp() const { return rareNonInheritedData->m_navigation->up(); }
const StyleNavigationValue&?

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:191: enum
ENavigationTarget { Current = 0, Root = 1, TargetName = 2 };
No need to specify values here. It might be worth considering making this an
enum class or otherwise add additional qualification to make these unique
(Current and Root feels like common enough.)

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/StyleNavigationData.cpp (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleNavigationData.cpp:2: * Copyright (C)
2014 Opera Software ASA. All rights reserved.
Assuming this isn't covered by the same license as the header, this should
probably use the standard license blurb
(http://dev.chromium.org/blink/coding-style#TOC-License)

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/StyleNavigationData.h (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleNavigationData.h:37: bool
operator==(const StyleNavigationData& o) const
Move definition to implementation file. (Each StyleNavigationValue looks like
it'll expand to a sizable chunk of code.)

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/StyleNavigationValue.h (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleNavigationValue.h:34: : m_auto(1)
1 -> true

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleNavigationValue.h:38: explicit
StyleNavigationValue(const String& id, ENavigationTarget target = Current)
Maybe make this AtomicString and then cast/et.c as needed when setting these up.
Same for all cases.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleNavigationValue.h:39: : m_auto(0)
0 -> false

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleNavigationValue.h:45: : m_auto(0)
false

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.cpp (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleRareNonInheritedData.cpp:5: *
Copyright (C) 2016 Samsung Electronics. All rights reserved.
Ditto.

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h (right):

https://codereview.chromium.org/1919813002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/style/StyleRareNonInheritedData.h:8: * Copyright
(C) 2016 Samsung Electronics. All rights reserved.
Haven't seen any of these be added in quite a while, and I'm not sure if trivial
changes like these qualify, but IANAL. I'd vote for not adding them though.

[Ilya Sherman, 2016-05-18 21:05:00]

histograms.xml lgtm

[Timothy Loh, 2016-05-19 01:52:50]

A couple of process things:
- Has there been an intent to implement about this? This sort of feature should
have an intent sent to blink-dev, see
http://www.chromium.org/blink#launch-process. You'll also need a
chromestatus.com entry and a tracking bug.
- The last line of the change description should be BUG=12345 (with the
appropriate bug number)

[Krzysztof Olczyk, 2016-05-19 05:39:00]

On 2016/05/19 at 01:52:50, timloh wrote:
> A couple of process things:
> - Has there been an intent to implement about this? This sort of feature
should have an intent sent to blink-dev, see
http://www.chromium.org/blink#launch-process. You'll also need a
chromestatus.com entry and a tracking bug.

I have sent out an intent to implement quite a long time ago, when sending
original CL that this one forks. 

> - The last line of the change description should be BUG=12345 (with the
appropriate bug number)

[Julien Isorce Samsung, 2016-05-19 09:56:42]

On 2016/05/19 05:39:00, Krzysztof Olczyk wrote:
> On 2016/05/19 at 01:52:50, timloh wrote:
> > A couple of process things:
> > - Has there been an intent to implement about this? This sort of feature
> should have an intent sent to blink-dev, see
> http://www.chromium.org/blink#launch-process. You'll also need a
> http://chromestatus.com entry and a tracking bug.
> 
> I have sent out an intent to implement quite a long time ago, when sending
> original CL that this one forks. 
> 

The intent to implement is there:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/u2lKAP3EiU4
but it is not listed in the blink-intent-tracker https://bit.ly/blinkintents .

[Anton Obzhirov, 2016-05-19 13:20:29]

On 2016/05/19 09:56:42, Julien Isorce wrote:
> On 2016/05/19 05:39:00, Krzysztof Olczyk wrote:
> > On 2016/05/19 at 01:52:50, timloh wrote:
> > > A couple of process things:
> > > - Has there been an intent to implement about this? This sort of feature
> > should have an intent sent to blink-dev, see
> > http://www.chromium.org/blink#launch-process. You'll also need a
> > http://chromestatus.com entry and a tracking bug.
> > 
> > I have sent out an intent to implement quite a long time ago, when sending
> > original CL that this one forks. 
> > 
> 
> The intent to implement is there:
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/u2lKAP3EiU4
> but it is not listed in the blink-intent-tracker https://bit.ly/blinkintents .

I can see from the intent it's got two LGTMs from Adam Barth and Eric Seidel.
http://www.chromium.org/blink#launch-process says
...
Optionally run an origin trial for your feature. To do this email blink-dev
using the "Intent to Experiment" template. Wait for LGTMs from at least 3 API
Owners.
...

Does it mean the intent needs to get 3 LGTM from
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/AP...
I can create http://chromestatus.com entry since it doesn't exist and create the
bug.
Is it OK to reuse the existing intent?

[esprehn, 2016-05-19 21:31:05]

That intent was 3 years ago and since then both abarth@ and eseidel@ have left
the project. You should send another one. :)

[Anton Obzhirov, 2016-05-20 14:02:06]

On 2016/05/19 21:31:05, esprehn wrote:
> That intent was 3 years ago and since then both abarth@ and eseidel@ have left
> the project. You should send another one. :)

Will do :)

[Anton Obzhirov, 2016-05-23 12:44:45]

On 2016/05/20 14:02:06, a.obzhirov wrote:
> On 2016/05/19 21:31:05, esprehn wrote:
> > That intent was 3 years ago and since then both abarth@ and eseidel@ have
left
> > the project. You should send another one. :)
> 
> Will do :)

Bug created:
https://bugs.chromium.org/p/chromium/issues/detail?id=614003

[lgombos, 2016-06-09 18:53:53]

Description was changed from

==========
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 ComputedStyle 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

It is based on the patch submitted by Krzysztof Olczyk
https://codereview.chromium.org/17450016

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.

R=holte@chromium.org, isherman@chromium.org, rune@opera.com,
timloh@chromium.org, esprehn@chromium.org, fs@opera.com, tkent@chromium.org,
kolczyk@opera.com
==========

to

==========
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 ComputedStyle 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

It is based on the patch submitted by Krzysztof Olczyk
https://codereview.chromium.org/17450016

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.

R=holte@chromium.org, isherman@chromium.org, rune@opera.com,
timloh@chromium.org, esprehn@chromium.org, fs@opera.com, tkent@chromium.org,
kolczyk@opera.com
==========