Intent to Implement and Ship: CSS3 nav-up/down/left/right properties

279 views
Skip to first unread message

Krzysztof Olczyk

unread,
Jun 21, 2013, 4:46:34 AM6/21/13
to blin...@chromium.org, Giuseppe Pascale, Christian Söderström, Mostyn Bramley-Moore

Primary eng/PM emails


Eng: kol...@opera.com

PM: chr...@opera.com


Spec

CSS3 UI
(http://dev.w3.org/csswg/css-ui/#nav-dir)


Summary

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


User agents for devices with directional navigation keys respond by navigating the focus according to four respective nav-* directional navigation properties (nav-up, nav-right, nav-down, nav-left)


Motivation



Regarding implementations / use:
- these properties have been supported in Presto core for quite some time
- these properties are widely used by the TV industry (both devices and apps) as it's an easy way to define D-Pad navigation.
- WebKit-based TV browsers from major TV manufacturers support it

Compatibility Risk

Although these properties are marked as "at risk" in the specification, there are no known issues with their definition, so the "at risk" was only due to lack of known implementations and 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
.


There are already implementations of these feature in products already in market:

- WebKit-based TV browsers from major TV manufacturers support it 

- These properties have been supported in Presto core for quite some time

- We have heard of other companies planning to submit related patches for WebKit.

The similar patch was filed to Webkit  (not submitted by us) - https://bugs.webkit.org/show_bug.cgi?id=66027


The www-style discussion on the feature: http://lists.w3.org/Archives/Public/www-style/2013Jun/0332.html


Ongoing technical constraints

None


Will this feature be supported on all five Blink platforms (Windows, Mac, Linux, Chrome OS and Android)?

Yes*


OWP launch tracking bug?

No


Row on feature dashboard?

No


Requesting approval to ship?

Yes.*


* However, the CSS attributes will be operational only if spatial navigation pref is enabled.

(see https://codereview.chromium.org/13811041)



--  
Best regards,
Krzysztof Olczyk

Opera Software ASA
TV & Connected devices
http://www.opera.com/business/tv/

Adam Barth

unread,
Jun 21, 2013, 11:17:26 AM6/21/13
to Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström, Mostyn Bramley-Moore
Do you plan to use the existing spatial navigation code?  If so, you should be aware that the existing code is of questionable quality.  Reading through it a couple months ago, I noticed a number of potential security vulnerabilities.  If you're using that code, do you plan to improve it?  I'd recommend both a complete code audit and substantial fuzzing.

Adam

Antonio Gomes

unread,
Jun 21, 2013, 11:29:53 AM6/21/13
to Adam Barth, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström, Mostyn Bramley-Moore
I beleive you have tools in place for fuzzing this code? Would you mind to point them? As the original author, I could help with improving that.

PS: with css3 nav-up/down/left/right you specify to node to focus on  given a direction. Then no spatial navigation should be involved. Please correct me if I am wrong.
--
--Antonio Gomes

Adam Barth

unread,
Jun 21, 2013, 11:53:46 AM6/21/13
to Antonio Gomes, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström, Mostyn Bramley-Moore
On Fri, Jun 21, 2013 at 8:29 AM, Antonio Gomes <toni...@gmail.com> wrote:
I believe you have tools in place for fuzzing this code? Would you mind to point them? As the original author, I could help with improving that.

The primary tools we use to fuzz the code is ClusterFuzz:


Unfortunately, it's not a tool that you can run yourself because it's built on Google infrastructure.

PS: with css3 nav-up/down/left/right you specify to node to focus on given a direction. Then no spatial navigation should be involved. Please correct me if I am wrong.

I'm not sure what you mean by this comment.  Do you mean that CSS3 lets you poke at the spatial navigation code without enabling the spatial navigation setting?

My point is that if Krzysztof wants Blink to commit to shipping the spatial navigation code, that comes with the responsibility to improve and maintain the code, including with respect to security.  I might not be searching correctly, but I don't see that Krzysztof has contributed any patches to Blink:

abarth@quadzen:~/git/blink/src/third_party/WebKit$ git log --author=olczyk
abarth@quadzen:~/git/blink/src/third_party/WebKit$ 

Krzysztof, I might recommend a more conservative approach:

1) Start out with a plain "intent to implement" without simultaneously requesting approval to ship.
2) Improve the spatial navigation code, for example by finding and fixing security issues and improving spec compliance.
3) Once you've got a solid track record of contributing to Blink, then send an intent to ship.

If you've been doing (2) for a while already and I just didn't find it with my search, then moving to step (3) is more appropriate.

Dominic Mazzoni

unread,
Jun 21, 2013, 12:05:30 PM6/21/13
to Adam Barth, Antonio Gomes, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström, Mostyn Bramley-Moore
On Fri, Jun 21, 2013 at 8:53 AM, Adam Barth <aba...@chromium.org> wrote:
On Fri, Jun 21, 2013 at 8:29 AM, Antonio Gomes <toni...@gmail.com> wrote:
I believe you have tools in place for fuzzing this code? Would you mind to point them? As the original author, I could help with improving that.

The primary tools we use to fuzz the code is ClusterFuzz:


Unfortunately, it's not a tool that you can run yourself because it's built on Google infrastructure.

Does the feature already have good test coverage? If so, ClusterFuzz should already be covering it.

- Dominic

Antonio Gomes

unread,
Jun 21, 2013, 12:06:12 PM6/21/13
to Adam Barth, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström, Mostyn Bramley-Moore
On Fri, Jun 21, 2013 at 11:53 AM, Adam Barth <aba...@chromium.org> wrote:
On Fri, Jun 21, 2013 at 8:29 AM, Antonio Gomes <toni...@gmail.com> wrote:
PS: with css3 nav-up/down/left/right you specify to node to focus on given a direction. Then no spatial navigation should be involved. Please correct me if I am wrong.

I'm not sure what you mean by this comment.  Do you mean that CSS3 lets you poke at the spatial navigation code without enabling the spatial navigation setting?


Sorry, it was unclear, indeed. Lets try with an example (quoted from the spec link below):
button#b1 {
	top:0; left:50%;
	nav-index:1;
	nav-right:#b2; nav-left:#b4;
	nav-down:#b2; nav-up:#b4;
}

As far as I could understand, if one presses the "right" arrow key, the implementation just focuses on element
#b2. For that, there is no need for Spatial Navigation.


--
--Antonio Gomes

Adam Barth

unread,
Jun 21, 2013, 12:18:32 PM6/21/13
to Antonio Gomes, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström, Mostyn Bramley-Moore
Ah!  Thank you for clarifying.  I thought the two were coupled.

I haven't looked at this code then, and I don't have an opinion about its quality.  :)

Adam

Adam Barth

unread,
Jun 21, 2013, 12:25:50 PM6/21/13
to Dominic Mazzoni, Antonio Gomes, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström, Mostyn Bramley-Moore
We can look at the code again to confirm, but my recollection from studying the spatial navigation code a couple ago was that I found a number of vulnerability.  That either means the tests are insufficient, ClusterFuzz is unable to find the vulnerabilities, or we haven't invested in fixing whatever vulnerabilities ClusterFuzz has found because we aren't shipping the spatial navigation code.

Of course, it's also possible I'm mistaken and these vulnerabilities don't exist.  :)

Adam

Antonio Gomes

unread,
Jun 21, 2013, 12:32:09 PM6/21/13
to Adam Barth, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström, Mostyn Bramley-Moore
Ah, in fact you were right: after re-looking the spec, it has a <auto> value where the User Agent is responsible for moving the focus around in response to an arrow key press. In this case, Spatial Navigation could get integrated to it, I believe.

--
--Antonio Gomes

Mostyn Bramley-Moore

unread,
Jun 21, 2013, 3:45:42 PM6/21/13
to Antonio Gomes, Adam Barth, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström
On 06/21/2013 05:29 PM, Antonio Gomes wrote:
> I beleive you have tools in place for fuzzing this code? Would you mind
> to point them? As the original author, I could help with improving that.
>
> PS: with css3 nav-up/down/left/right you specify to node to focus on
> given a direction. Then no spatial navigation should be involved. Please
> correct me if I am wrong.

I think it's worth clarifying some terminology here...

"Spatial navigation" is the act of moving the focus/highlight from the
currently focused element to another suitable element, using (typically)
four-way arrow keys. This is useful in setups where a pointing input
device is not available (eg TV with an IR remote) or not suitable (eg
for users with impaired mobility). In many situations, this is
preferable to tab navigation.

"Heuristic spatial navigation" is spatial navigation where the target
element is determined by some automated algorithm. This is what the
existing code in blink does. Note that while this works well on simple
grid based layouts, it is an intractable problem on complicated web page
layouts.

The CSS3 nav-up/down/left/right properties (aka "CSS3 nav-dir") are a
simple way for web page authors to specify the spatial navigation
transitions from a given element. When present, these transitions can
override the heuristic spatial navigation algorithm- as done in Presto,
and in Blink if this CL manages to pass muster:
https://codereview.chromium.org/17450016/. Note that with this CL, if
spatial navigation is disabled, the CSS3 nav-dir properties are ignored.


-Mostyn.

> On Fri, Jun 21, 2013 at 11:17 AM, Adam Barth <aba...@chromium.org
> <mailto:aba...@chromium.org>> wrote:
>
> Do you plan to use the existing spatial navigation code? If so, you
> should be aware that the existing code is of questionable quality.
> Reading through it a couple months ago, I noticed a number of
> potential security vulnerabilities. If you're using that code, do
> you plan to improve it? I'd recommend both a complete code audit
> and substantial fuzzing.
>
> Adam
>
>
> On Fri, Jun 21, 2013 at 1:46 AM, Krzysztof Olczyk <kol...@opera.com
> <mailto:kol...@opera.com>> wrote:
>
> __
>
> Primary eng/PM emails
>
>
> Eng: kol...@opera.com <mailto:kol...@opera.com>
>
> PM: chr...@opera.com <mailto:chr...@opera.com>
--
Mostyn Bramley-Moore
mos...@opera.com

Mostyn Bramley-Moore

unread,
Jun 21, 2013, 4:00:00 PM6/21/13
to Adam Barth, Antonio Gomes, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström
Krzysztof should have posted an intent to implement only. The
functionality that his CL adds is hidden behind an existing runtime flag
(--enable-spatial-navigation).

We agree with you that the spatial navigation code is not in a fit state
to ship yet. We're working in parallel to improve this code in a few
places, and hope to submit more CLs soon.

-Mostyn.

> On Fri, Jun 21, 2013 at 11:17 AM, Adam Barth <aba...@chromium.org
> <mailto:aba...@chromium.org>> wrote:
>
> Do you plan to use the existing spatial navigation code? If so,
> you should be aware that the existing code is of questionable
> quality. Reading through it a couple months ago, I noticed a
> number of potential security vulnerabilities. If you're using
> that code, do you plan to improve it? I'd recommend both a
> complete code audit and substantial fuzzing.
>
> Adam
>
>
> On Fri, Jun 21, 2013 at 1:46 AM, Krzysztof Olczyk
> <kol...@opera.com <mailto:kol...@opera.com>> wrote:
>
> __
>
> Primary eng/PM emails
>
>
> Eng: kol...@opera.com <mailto:kol...@opera.com>
>
> PM: chr...@opera.com <mailto:chr...@opera.com>
--
Mostyn Bramley-Moore
mos...@opera.com

Adam Barth

unread,
Jun 21, 2013, 4:51:39 PM6/21/13
to Mostyn Bramley-Moore, Antonio Gomes, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström
Krzysztof should have posted an intent to implement only.  The functionality that his CL adds is hidden behind an existing runtime flag (--enable-spatial-navigation).

LGTM to implement.
 
We agree with you that the spatial navigation code is not in a fit state to ship yet.  We're working in parallel to improve this code in a few places, and hope to submit more CLs soon.

Great.  I need to chase down whether the issues I was worried about are reason security bugs.

Eric Seidel

unread,
Jun 24, 2013, 2:28:46 PM6/24/13
to Adam Barth, Mostyn Bramley-Moore, Antonio Gomes, Krzysztof Olczyk, blink-dev, Giuseppe Pascale, Christian Söderström
LGTM to implement.
Reply all
Reply to author
Forward
0 new messages