Summary: | Implementation for "nav-up" "nav-down" "nav-left" "nav-right" properties in CSS3-UI module | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | kyounga <kyounga.ra> | ||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED INVALID | ||||||||||||
Severity: | Enhancement | CC: | 7raivis, ap, benjamin, cfleizach, changseok, cmarcelo, cshu, efidler, eoconnor, gyuyoung.kim, jcraig, kennyluck, macpherson, mario, sangwhan, sergio, shanestephens, simon.fraser, syoichi, tonikitoo, webkit.review.bot, yael | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
URL: | http://dev.w3.org/csswg/css3-ui/#nav-dir | ||||||||||||
Attachments: |
|
Description
kyounga
2011-08-10 18:33:27 PDT
Note that this specification is still an Editors Draft, which means that it may change at any time. As mentioned in antonio's question on webkit-dev, I wonder what is difference between spatial navigation and this feature. If operation is similar to Spatial Navigation, it seems this feature can be implemented by Spatial Navigation. And, I'm not sure if this spec is sufficiently stable. (In reply to comment #2) > As mentioned in antonio's question on webkit-dev, I wonder what is difference between spatial navigation and this feature. If operation is similar to Spatial Navigation, it seems this feature can be implemented by Spatial Navigation. > > And, I'm not sure if this spec is sufficiently stable. I think these property is new feature in addition to a "spatial navigation" which browsers provide by default. If <a> element with "nav-up:auto" has focused and the user press "up arrow key", the browser use a spatial navigation, or if there is "nav-up:#foo", the browser send a focus the element with "id=foo" attribute. So, I think if I can reuse the fragment link function, it might NOT be a big deal. The definition of the nav-* properties is odd. No other CSS property has bare selectors as values, so we don't have any support for parsing those right now. I'd hold off implementing this until the CSS WG has had a chance to discuss this further. If you want to kick-start discussion, please email www-style. Created attachment 106113 [details]
initial patch for review.
I know css-style are discussing on using ID selector as CSS value.
just made an initial implementation only for review.
There are several issues to be discussed in my patch
1. Changed CSSGrammar.y
- Added IDSEL as a value, checked for #15360 issue (IDSEL was removed by this issue.)
2. Added a new class member to RenderStyle, instead of pointer or smart pointer.
- Is it memory waste? less memory fragmentation? good performance?
- I think that this property won't be used frequently but if it is used once, it may be used frequently in one page.
Simple unit-tests are completed.
- no "nav-*" property.
- simple "nav-*" property from CSS3-UI example.
- scrollable page.
- inherit value
- invalid target name = no work.
- target name = between two frames.
Comment on attachment 106113 [details]
initial patch for review.
please provide a patch that applies to ToT
Created attachment 123237 [details]
initial patch for the latest webkit.
Review this, please.
- Opera's supported already it.
- These properties are used popularly on web content in TV industry
Comment on attachment 123237 [details] initial patch for the latest webkit. View in context: https://bugs.webkit.org/attachment.cgi?id=123237&action=review patch still does not apply it seems. anyways, some nitpicks: > Source/WebCore/page/FocusController.cpp:795 > + RenderObject* renderer = focusedNode ? focusedNode->renderer() : 0; > + > + if (!renderer) drop extran line here. > Source/WebCore/page/FocusController.cpp:833 > + if (target == "current") > + targetFrame = curFrame; > + else if (target == "root") > + targetFrame = curFrame->tree()->top(); DEFINE_STATIC_LOCAL(AtomicString, Current, ("current")); DEFINE_STATIC_LOCAL(AtomicString, Root, ("root")) > Source/WebCore/page/FocusController.cpp:844 > + if (!anchorNode) > + return false; > + // if it's same with the current focused, it should be consumed. add an extra line here. if -> If > Source/WebCore/page/FocusController.cpp:852 > + bool successfullyFocused = setFocusedNode(anchorNode, targetFrame); > + > + if (successfullyFocused) drop extra line here. > Source/WebCore/page/FocusController.cpp:854 > + m_page->chrome()->focusedNodeChanged(anchorNode); > + return true; add an extra line here. Created attachment 123446 [details]
Patch_v2 is including the reviewer's comment.
I've attached a new patch following the comment from Antonio Gomes.
My patch includes two new files.
I build and tested this patch only on my mac, with XCode project.
I think that's a why the build bot makes fail log for cr-linux, qt, gtk, win.
("style" also has fail log even though I checked check-webkit-style script)
Should I modify project files or build scripts for other platform?
Honestly, I don't know how to do.
Is there someone to review this? Review this patch, Please.... I think it's a useful feature and not a big deal, except css parser change. Any comment is welcome. Comment on attachment 123446 [details] Patch_v2 is including the reviewer's comment. View in context: https://bugs.webkit.org/attachment.cgi?id=123446&action=review > Source/WebCore/ChangeLog:3 > + Need a short description and bug URL (OOPS!) It looks you need to study how to write changelog first before submitting a patch. Please read webkit contribution guideline first. Comment on attachment 123446 [details] Patch_v2 is including the reviewer's comment. View in context: https://bugs.webkit.org/attachment.cgi?id=123446&action=review In my opinion, this patch should to have test cases as well. Then, I think reviewers can start to review this patch. By the way, I wonder if you notify this new feature to webkit-dev. If you didn't it, you have to do it first. >> Source/WebCore/ChangeLog:3 >> + Need a short description and bug URL (OOPS!) > > It looks you need to study how to write changelog first before submitting a patch. Please read webkit contribution guideline first. WebKit contribution guidance. - http://www.webkit.org/coding/contributing.html > Source/WebCore/css/CSSStyleApplyProperty.cpp:1625 > + || primitiveValue->primitiveType() == CSSPrimitiveValue::CSS_PARSER_HEXCOLOR) Wrong indentation for *||* operator. > Source/WebCore/css/CSSStyleSelector.cpp:4465 > + for (size_t i = 0; i < 2; i++) { WebKit prefers to use ++i instead of i++. > Source/WebCore/css/CSSStyleSelector.h:355 > + StyleNavigationValue getStyleNavigationValue(CSSValue*); Nit : Add an empty line. > Source/WebCore/page/FocusController.cpp:840 > + // TODO:// which one is right? WebKit prefer to use FIXME: instead of TODO: > Source/WebCore/rendering/style/StyleNavigationData.h:2 > + * Copyright (C) 2011 Kyounga Ra (kyounga.ra@gmail.com) s/2011/2012/g > Source/WebCore/rendering/style/StyleNavigationData.h:33 > + StyleNavigationData(const StyleNavigationData& o) Use *explicit* keyword. The CSS WG agreed to remove this feature recently. See discussion in bug 95178. Marking as Resolved/Invalid. *** Bug 95178 has been marked as a duplicate of this bug. *** (In reply to comment #13) > The CSS WG agreed to remove this feature recently. See discussion in bug 95178. Marking as Resolved/Invalid. Not "remove", but just "don't include in level 3", which means they will likely get postponed to level 4. Created attachment 207442 [details]
patch for latest webkit
patch for latest webkit.
webkit-style is checked.
Even though nav-dir properties was dropped from CSS3, it is still demanded from next CSS4 or TV industry.
Comment on attachment 207442 [details] patch for latest webkit View in context: https://bugs.webkit.org/attachment.cgi?id=207442&action=review It needs review on the style change. I only reviewed it overly and specially the FocusController change. 1) it needs a proper changelog entry 2) it needs tests. maybe lots... r- for now. It looks to be in the right track though. > Source/WebCore/ChangeLog:9 > + Support directional focus navigation properties(nav-[dir]) in CSS UI module. > + https://bugs.webkit.org/show_bug.cgi?id=66027 > + > + Reviewed by NOBODY (OOPS!). > + > + No new tests (OOPS!). > + Please explain your solution here, preferably with per function change entries (see below) > Source/WebCore/page/FocusController.cpp:945 > + // If we were in the autoscroll/panScroll mode we want to stop it. > + curFrame->eventHandler()->stopAutoscrollTimer(); this needs a test, I would say. > Source/WebCore/page/FocusController.cpp:962 > + if (focused == anchor) > + return true; this needs a test. > Source/WebCore/page/FocusController.cpp:964 > + anchor->scrollIntoViewIfNeeded(false); add a comment saying what "false" is. e.g. (false /*bleh*/) (In reply to comment #16) > Even though nav-dir properties was dropped from CSS3, it is still demanded from next CSS4 or TV industry. While I agree with you that directional navigation is necessary,implementing this portion of the current specification is short-sighted. The draft has serious problems that will result in formal objections to the W3C if they remained unaddressed. This needs a feature flag (since it does not have platform integration, you can probably enable it by default), an experimental prefix, and tests; a lot of tests. (In reply to comment #18) > (In reply to comment #16) > > Even though nav-dir properties was dropped from CSS3, it is still demanded from next CSS4 or TV industry. > > While I agree with you that directional navigation is necessary,implementing this portion of the current specification is short-sighted. The draft has serious problems that will result in formal objections to the W3C if they remained unaddressed. Let's be constructive. Can you share which problems should block this? Objections sent to the CSS working group on www-style list. http://lists.w3.org/Archives/Public/www-style/2013May/0076.html Another FYI, nav-up/down/left/right were pushed to CSS4 but according to Leif: "[CSS3] retains nav-up, nav-down, nav-left, nav-right [and marked as at-risk, but], nav-index is still postponed." http://lists.w3.org/Archives/Public/www-style/2013Jul/0711.html |