Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
797463003:
spatnav: Allow focus move to a close-by not-fully-aligned node over a distant but fully-aligned nod… (Closed)
Descriptionspatnav: Allow focus move to a close-by not-fully-aligned node over a distant but fully-aligned node.
The original algorithm favors a fully-aligned node over any partially- or
none-aligned nodes no matter how far the former is. This is against
intuition. The improved algorithm gives a more-aligned node a weight so that
it's more likely to be chosen but not definitely.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188642
Patch Set 1 #
Total comments: 3
Patch Set 2 : use weight in orthogonal distance calculation. #
Total comments: 1
Patch Set 3 : address reviewer comments #Patch Set 4 : fix not-aligned node issue #
Total comments: 5
Patch Set 5 : Address reviewer comments #Patch Set 6 : rebaseline #Messages
Total messages: 35 (6 generated)
c.shu@samsung.com changed reviewers: + fs@opera.com, kolczyk@opera.com
This patch may be controversial but let's start the discussion.
On 2014/12/12 at 18:38:45, c.shu wrote: > This patch may be controversial but let's start the discussion. Hi c.shu, how have you chosen the values of the weights?
https://codereview.chromium.org/797463003/diff/1/Source/core/page/SpatialNavi... File Source/core/page/SpatialNavigation.cpp (right): https://codereview.chromium.org/797463003/diff/1/Source/core/page/SpatialNavi... Source/core/page/SpatialNavigation.cpp:691: RectsAlignment alignment = alignmentForRects(type, currentRect, nodeRect, viewSize); Should the weights be applied to (one of) the components of the distance calculation (above) rather than to the "compound expression"?
https://codereview.chromium.org/797463003/diff/1/Source/core/page/SpatialNavi... File Source/core/page/SpatialNavigation.cpp (right): https://codereview.chromium.org/797463003/diff/1/Source/core/page/SpatialNavi... Source/core/page/SpatialNavigation.cpp:691: RectsAlignment alignment = alignmentForRects(type, currentRect, nodeRect, viewSize); On 2014/12/15 11:36:09, fs wrote: > Should the weights be applied to (one of) the components of the distance > calculation (above) rather than to the "compound expression"? The distance calculation at L688 is based on the spec. Then I did the hack based on alignment result. The weights are determined by some experimental tests. It will be great if we can come up a better algorithm and integrate with the calculation above.
On 2014/12/15 07:58:49, Krzysztof Olczyk wrote: > On 2014/12/12 at 18:38:45, c.shu wrote: > > This patch may be controversial but let's start the discussion. > > Hi c.shu, > > how have you chosen the values of the weights? by experiments. :(
https://codereview.chromium.org/797463003/diff/1/Source/core/page/SpatialNavi... File Source/core/page/SpatialNavigation.cpp (right): https://codereview.chromium.org/797463003/diff/1/Source/core/page/SpatialNavi... Source/core/page/SpatialNavigation.cpp:691: RectsAlignment alignment = alignmentForRects(type, currentRect, nodeRect, viewSize); On 2014/12/15 18:17:37, c.shu wrote: > On 2014/12/15 11:36:09, fs wrote: > > Should the weights be applied to (one of) the components of the distance > > calculation (above) rather than to the "compound expression"? > > The distance calculation at L688 is based on the spec. The link at L687 does not lead to a spec - it leads to a note =) (see red box just at the top of it.) That being said, it doesn't hurt to read the section about the distance calculation to see the motivation for the different parts - it could probably even help in this case. I.e you may change this calculation any way you want - we're after all pretty far into heuristics-land here. > Then I did the hack based > on alignment result. The weights are determined by some experimental tests. It > will be great if we can come up a better algorithm and integrate with the > calculation above. The WICD distance calculation includes a weight already as I understand - the '2' that's applied to |orthogonalAxisDistance|, so it feels like that could be a good starting point for further tweaking.
> The WICD distance calculation includes a weight already as I understand - the > '2' that's applied to |orthogonalAxisDistance|, so it feels like that could be a > good starting point for further tweaking. I totally agree. I will work on that and thanks for the support.
The new patch adds penalty to orthogonal distance. This simplifies the code a lot. We may want to add weight to overlap later but so far all tests are passed.
On 2014/12/16 22:51:45, c.shu wrote: > The new patch adds penalty to orthogonal distance. This simplifies the code a > lot. We may want to add weight to overlap later but so far all tests are passed. That is indeed a lot code gone. Nice if we can indeed get rid of it. I'd suggest that you try to think up some tricky cases. For example: * non-square targets =) * the edge-conditions for the weights * anything else you can think up https://codereview.chromium.org/797463003/diff/20001/Source/core/page/Spatial... File Source/core/page/SpatialNavigation.cpp (right): https://codereview.chromium.org/797463003/diff/20001/Source/core/page/Spatial... Source/core/page/SpatialNavigation.cpp:538: weightedOrthogonalAxisDistance = yAxis.abs() * 30; Could you try to add some form of documentation to the weights (even if just "gave good results on tests/scenarios foo and bar"). I'm guessing that the higher weight for left/right/y-axis incorporates an assumption that targets are (much) wider than they are tall (which is often true for links for instance.)
On 2014/12/17 09:51:11, fs wrote: > On 2014/12/16 22:51:45, c.shu wrote: > > The new patch adds penalty to orthogonal distance. This simplifies the code a > > lot. We may want to add weight to overlap later but so far all tests are > passed. > > That is indeed a lot code gone. Nice if we can indeed get rid of it. > > I'd suggest that you try to think up some tricky cases. For example: > > * non-square targets =) > * the edge-conditions for the weights > * anything else you can think up I think the calculation is always based on rects. For non-square targets, it will use the enclosing rects. Otherwise, it will be too complicated. I will try to add more tests to cover edge-conditions. > https://codereview.chromium.org/797463003/diff/20001/Source/core/page/Spatial... > File Source/core/page/SpatialNavigation.cpp (right): > > https://codereview.chromium.org/797463003/diff/20001/Source/core/page/Spatial... > Source/core/page/SpatialNavigation.cpp:538: weightedOrthogonalAxisDistance = > yAxis.abs() * 30; > Could you try to add some form of documentation to the weights (even if just > "gave good results on tests/scenarios foo and bar"). I'm guessing that the > higher weight for left/right/y-axis incorporates an assumption that targets are > (much) wider than they are tall (which is often true for links for instance.) Do you think it's Ok I add comments for the weights and this CL is good to go? I can take care of edge-scenarios in the following patches.
On 2014/12/19 18:15:19, c.shu wrote: > On 2014/12/17 09:51:11, fs wrote: > > On 2014/12/16 22:51:45, c.shu wrote: > > > The new patch adds penalty to orthogonal distance. This simplifies the code > a > > > lot. We may want to add weight to overlap later but so far all tests are > > passed. > > > > That is indeed a lot code gone. Nice if we can indeed get rid of it. > > > > I'd suggest that you try to think up some tricky cases. For example: > > > > * non-square targets =) > > * the edge-conditions for the weights > > * anything else you can think up > > I think the calculation is always based on rects. For non-square targets, it > will use the enclosing rects. Otherwise, it will be too complicated. > I will try to add more tests to cover edge-conditions. Non-square also mean general rectangles (sides differing in length.) I noticed that squares were popoular in the tests =). > > > https://codereview.chromium.org/797463003/diff/20001/Source/core/page/Spatial... > > File Source/core/page/SpatialNavigation.cpp (right): > > > > > https://codereview.chromium.org/797463003/diff/20001/Source/core/page/Spatial... > > Source/core/page/SpatialNavigation.cpp:538: weightedOrthogonalAxisDistance = > > yAxis.abs() * 30; > > Could you try to add some form of documentation to the weights (even if just > > "gave good results on tests/scenarios foo and bar"). I'm guessing that the > > higher weight for left/right/y-axis incorporates an assumption that targets > are > > (much) wider than they are tall (which is often true for links for instance.) > > Do you think it's Ok I add comments for the weights and this CL is good to go? > I can take care of edge-scenarios in the following patches. There should absolutely be comments added regardless. I suppose we could accept adding tests later, but then we need to know that "later" is still "reasonably soon". Could you file a bug for that and mention it here when you add the comment?
Non-square also mean general rectangles (sides differing in length.) I noticed that squares were popoular in the tests =). Ok, I misunderstood this. :) I will add tests for general rects in this CL.
On 2014/12/19 19:50:42, c.shu wrote: > Non-square also mean general rectangles (sides differing in length.) I noticed > that squares were popoular in the tests =). > > Ok, I misunderstood this. :) I will add tests for general rects in this CL. fs, I updated the new test case so the targets are rectangular. Also the links in existing snav-fully-aligned-vertically.html and snav-fully-aligned-horizontally.html are rectangles. So they should be well covered. thanks.
I think we'd still like to have more tests for this, because magic constants tend to be fragile (not to mention difficult to reason about.) LGTM
The CQ bit was checked by c.shu@samsung.com
On 2014/12/25 16:39:03, fs (OoO) wrote: > I think we'd still like to have more tests for this, because magic constants > tend to be fragile (not to mention difficult to reason about.) > > LGTM will do. thanks, fs.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/797463003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
On 2014/12/29 22:41:04, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_blink_compile_dbg on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...) I found some problem with patch 3 and fixed it in the new patch set. A new test case is also added. Please take a look. Thanks.
https://codereview.chromium.org/797463003/diff/60001/Source/core/page/Spatial... File Source/core/page/SpatialNavigation.cpp (right): https://codereview.chromium.org/797463003/diff/60001/Source/core/page/Spatial... Source/core/page/SpatialNavigation.cpp:90: static bool areRectsNotAligned(FocusType type, const LayoutRect& a, const LayoutRect& b) Not the most obvious name IMHO. Maybe rectsIntersectOnOrthogonalAxis or something instead? (I.e. drop the negation from the name - and impl. - and add it to the call-site instead.) https://codereview.chromium.org/797463003/diff/60001/Source/core/page/Spatial... Source/core/page/SpatialNavigation.cpp:541: yAxis += currentRect.height() / 2; Why mutate the yAxis value here? Shouldn't you just add this to the weighted value? Similarly below. The additional bias also strike as potentially fairly large/small - i.e. why is this value a "good choice"?
https://codereview.chromium.org/797463003/diff/60001/Source/core/page/Spatial... File Source/core/page/SpatialNavigation.cpp (right): https://codereview.chromium.org/797463003/diff/60001/Source/core/page/Spatial... Source/core/page/SpatialNavigation.cpp:90: static bool areRectsNotAligned(FocusType type, const LayoutRect& a, const LayoutRect& b) On 2015/01/12 12:07:23, fs wrote: > Not the most obvious name IMHO. Maybe rectsIntersectOnOrthogonalAxis or > something instead? (I.e. drop the negation from the name - and impl. - and add > it to the call-site instead.) will do. https://codereview.chromium.org/797463003/diff/60001/Source/core/page/Spatial... Source/core/page/SpatialNavigation.cpp:541: yAxis += currentRect.height() / 2; On 2015/01/12 12:07:23, fs wrote: > Why mutate the yAxis value here? Shouldn't you just add this to the weighted > value? Similarly below. The additional bias also strike as potentially fairly > large/small - i.e. why is this value a "good choice"? The weight has no impact on 0. That's why I have to add a penalty on yAxis. This happens in the alignnotalign test. The returns of entryAndExitPointsforDirection are always 0 if the edges of the candidate rect and current rect are aligned, not matter if the candidate is on the same side or the other side of current node. I thought the height of the currentrect is better than a hardcoded one, but not sure.
https://codereview.chromium.org/797463003/diff/60001/Source/core/page/Spatial... File Source/core/page/SpatialNavigation.cpp (right): https://codereview.chromium.org/797463003/diff/60001/Source/core/page/Spatial... Source/core/page/SpatialNavigation.cpp:541: yAxis += currentRect.height() / 2; On 2015/01/12 18:03:38, c.shu wrote: > On 2015/01/12 12:07:23, fs wrote: > > Why mutate the yAxis value here? Shouldn't you just add this to the weighted > > value? Similarly below. The additional bias also strike as potentially fairly > > large/small - i.e. why is this value a "good choice"? > > The weight has no impact on 0. That's why I have to add a penalty on yAxis. This > happens in the alignnotalign test. The returns of entryAndExitPointsforDirection > are always 0 if the edges of the candidate rect and current rect are aligned, > not matter if the candidate is on the same side or the other side of current > node. I get the part about 0, but why not for instance (yAxis + bias) * ... rather than mutation of yAxis (which affects the computation of the 'real' distance too.) > I thought the height of the currentrect is better than a hardcoded one, > but not sure. Quite possible. FTR, what I'm trying to avoid is a "turn the knobs and hope it'll be fine" kind of situation.
> I get the part about 0, but why not for instance (yAxis + bias) * ... rather > than mutation of yAxis (which affects the computation of the 'real' distance > too.) You are right. I thought what I was doing is equivalent to (y + bias) * weight. I will fix it. > > > I thought the height of the currentrect is better than a hardcoded one, > > but not sure. > > Quite possible. FTR, what I'm trying to avoid is a "turn the knobs and hope > it'll be fine" kind of situation. I was trying to find a logical distance between the two rects. It's supposed to be the distance between the two rect centers, which is half height of current rect + half height of candidate rect + distance of the edges. Distance of the edges is already considered and it seems we should not consider the half height of the candidate rect. This is why I used the half height of the current rect. The result is that the penalty is now respect to the height of the current rect. I think it makes sense. If you agree, I will upload a new patch. Thanks for the review!
On 2015/01/13 15:50:54, c.shu wrote: ... > > > I thought the height of the currentrect is better than a hardcoded one, > > > but not sure. > > > > Quite possible. FTR, what I'm trying to avoid is a "turn the knobs and hope > > it'll be fine" kind of situation. > > I was trying to find a logical distance between the two rects. It's supposed to > be the > distance between the two rect centers, which is half height of current rect + > half height > of candidate rect + distance of the edges. Distance of the edges is already > considered > and it seems we should not consider the half height of the candidate rect. This > is why > I used the half height of the current rect. The result is that the penalty is > now > respect to the height of the current rect. I think it makes sense. If you agree, > I will upload > a new patch. Thanks for the review! I think you should try to "boil" the above text down a bit and add it to the comment.
On 2015/01/13 16:03:05, fs wrote: > On 2015/01/13 15:50:54, c.shu wrote: > ... > > > > I thought the height of the currentrect is better than a hardcoded one, > > > > but not sure. > > > > > > Quite possible. FTR, what I'm trying to avoid is a "turn the knobs and hope > > > it'll be fine" kind of situation. > > > > I was trying to find a logical distance between the two rects. It's supposed > to > > be the > > distance between the two rect centers, which is half height of current rect + > > half height > > of candidate rect + distance of the edges. Distance of the edges is already > > considered > > and it seems we should not consider the half height of the candidate rect. > This > > is why > > I used the half height of the current rect. The result is that the penalty is > > now > > respect to the height of the current rect. I think it makes sense. If you > agree, > > I will upload > > a new patch. Thanks for the review! > > I think you should try to "boil" the above text down a bit and add it to the > comment. What do you think about the latest CL, fs?
On 2015/01/16 19:47:50, c.shu wrote: > On 2015/01/13 16:03:05, fs wrote: > > On 2015/01/13 15:50:54, c.shu wrote: > > ... > > > > > I thought the height of the currentrect is better than a hardcoded one, > > > > > but not sure. > > > > > > > > Quite possible. FTR, what I'm trying to avoid is a "turn the knobs and > hope > > > > it'll be fine" kind of situation. > > > > > > I was trying to find a logical distance between the two rects. It's supposed > > to > > > be the > > > distance between the two rect centers, which is half height of current rect > + > > > half height > > > of candidate rect + distance of the edges. Distance of the edges is already > > > considered > > > and it seems we should not consider the half height of the candidate rect. > > This > > > is why > > > I used the half height of the current rect. The result is that the penalty > is > > > now > > > respect to the height of the current rect. I think it makes sense. If you > > agree, > > > I will upload > > > a new patch. Thanks for the review! > > > > I think you should try to "boil" the above text down a bit and add it to the > > comment. > > What do you think about the latest CL, fs? LGTM
The CQ bit was checked by c.shu@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/797463003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/page/SpatialNavigation.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/page/SpatialNavigation.cpp
Hunk #1 FAILED at 45.
Hunk #3 FAILED at 91.
Hunk #4 succeeded at 643 with fuzz 2.
Hunk #5 FAILED at 658.
Hunk #6 succeeded at 685 with fuzz 1.
3 out of 6 hunks FAILED -- saving rejects to file
Source/core/page/SpatialNavigation.cpp.rej
Patch: Source/core/page/SpatialNavigation.cpp
Index: Source/core/page/SpatialNavigation.cpp
diff --git a/Source/core/page/SpatialNavigation.cpp
b/Source/core/page/SpatialNavigation.cpp
index
e9ea56911a233c090ba660d29a03419265d2e80c..9608f4c56742f03dcc8be9b5ffe408f2cd03cc80
100644
--- a/Source/core/page/SpatialNavigation.cpp
+++ b/Source/core/page/SpatialNavigation.cpp
@@ -45,10 +45,6 @@ namespace blink {
using namespace HTMLNames;
-static RectsAlignment alignmentForRects(FocusType, const LayoutRect&, const
LayoutRect&, const LayoutSize& viewSize);
-static bool areRectsFullyAligned(FocusType, const LayoutRect&, const
LayoutRect&);
-static bool areRectsPartiallyAligned(FocusType, const LayoutRect&, const
LayoutRect&);
-static bool areRectsMoreThanFullScreenApart(FocusType, const LayoutRect&
curRect, const LayoutRect& targetRect, const LayoutSize& viewSize);
static bool isRectInDirection(FocusType, const LayoutRect&, const LayoutRect&);
static void deflateIfOverlapped(LayoutRect&, LayoutRect&);
static LayoutRect rectToAbsoluteCoordinates(LocalFrame* initialFrame, const
LayoutRect&);
@@ -59,7 +55,6 @@ FocusCandidate::FocusCandidate(Node* node, FocusType type)
, focusableNode(nullptr)
, enclosingScrollableBox(nullptr)
, distance(maxDistance())
- , alignment(None)
, isOffscreen(true)
, isOffscreenAfterScrolling(true)
{
@@ -92,154 +87,19 @@ bool isSpatialNavigationEnabled(const LocalFrame* frame)
return (frame && frame->settings() &&
frame->settings()->spatialNavigationEnabled());
}
-static RectsAlignment alignmentForRects(FocusType type, const LayoutRect&
curRect, const LayoutRect& targetRect, const LayoutSize& viewSize)
+static bool rectsIntersectOnOrthogonalAxis(FocusType type, const LayoutRect& a,
const LayoutRect& b)
{
- // If we found a node in full alignment, but it is too far away, ignore it.
- if (areRectsMoreThanFullScreenApart(type, curRect, targetRect, viewSize))
- return None;
-
- if (areRectsFullyAligned(type, curRect, targetRect))
- return Full;
-
- if (areRectsPartiallyAligned(type, curRect, targetRect))
- return Partial;
-
- return None;
-}
-
-static inline bool isHorizontalMove(FocusType type)
-{
- return type == FocusTypeLeft || type == FocusTypeRight;
-}
-
-static inline LayoutUnit start(FocusType type, const LayoutRect& rect)
-{
- return isHorizontalMove(type) ? rect.y() : rect.x();
-}
-
-static inline LayoutUnit middle(FocusType type, const LayoutRect& rect)
-{
- LayoutPoint center(rect.center());
- return isHorizontalMove(type) ? center.y(): center.x();
-}
-
-static inline LayoutUnit end(FocusType type, const LayoutRect& rect)
-{
- return isHorizontalMove(type) ? rect.maxY() : rect.maxX();
-}
-
-// This method checks if rects |a| and |b| are fully aligned either vertically
or
-// horizontally. In general, rects whose central point falls between the top or
-// bottom of each other are considered fully aligned.
-// Rects that match this criteria are preferable target nodes in move focus
changing
-// operations.
-// * a = Current focused node's rect.
-// * b = Focus candidate node's rect.
-static bool areRectsFullyAligned(FocusType type, const LayoutRect& a, const
LayoutRect& b)
-{
- LayoutUnit aStart, bStart, aEnd, bEnd;
-
switch (type) {
case FocusTypeLeft:
- aStart = a.x();
- bEnd = b.x();
- break;
case FocusTypeRight:
- aStart = b.x();
- bEnd = a.x();
- break;
+ return a.maxY() > b.y() && a.y() < b.maxY();
case FocusTypeUp:
- aStart = a.y();
- bEnd = b.y();
- break;
case FocusTypeDown:
- aStart = b.y();
- bEnd = a.y();
- break;
+ return a.maxX() > b.x() && a.x() < b.maxX();
default:
ASSERT_NOT_REACHED();
return false;
}
-
- if (aStart < bEnd)
- return false;
-
- aStart = start(type, a);
- bStart = start(type, b);
-
- LayoutUnit aMiddle = middle(type, a);
- LayoutUnit bMiddle = middle(type, b);
-
- aEnd = end(type, a);
- bEnd = end(type, b);
-
- // Picture of the totally aligned logic:
- //
- // Horizontal Vertical Horizontal Vertical
- // **************************** *****************************
- // * _ * _ _ _ _ * * _ * _ _ *
- // * |_| _ * |_|_|_|_| * * _ |_| * |_|_| *
- // * |_|....|_| * . * * |_|....|_| * . *
- // * |_| |_| (1) . * * |_| |_| (2) . *
- // * |_| * _._ * * |_| * _ _._ _ *
- // * * |_|_| * * * |_|_|_|_| *
- // * * * * * *
- // **************************** *****************************
-
- return (bMiddle >= aStart && bMiddle <= aEnd) // (1)
- || (aMiddle >= bStart && aMiddle <= bEnd); // (2)
-}
-
-// This method checks if rects |a| and |b| are partially aligned either
vertically or
-// horizontally. In general, rects whose either of edges falls between the top
or
-// bottom of each other are considered partially-aligned.
-// This is a separate set of conditions from "fully-aligned" and do not include
cases
-// that satisfy the former.
-// * a = Current focused node's rect.
-// * b = Focus candidate node's rect.
-static bool areRectsPartiallyAligned(FocusType type, const LayoutRect& a, const
LayoutRect& b)
-{
- LayoutUnit aStart = start(type, a);
- LayoutUnit bStart = start(type, b);
- LayoutUnit aEnd = end(type, a);
- LayoutUnit bEnd = end(type, b);
-
- // Picture of the partially aligned logic:
- //
- // Horizontal Vertical
- // ********************************
- // * _ * _ _ _ *
- // * |_| * |_|_|_| *
- // * |_|.... _ * . . *
- // * |_| |_| * . . *
- // * |_|....|_| * ._._ _ *
- // * |_| * |_|_|_| *
- // * |_| * *
- // * * *
- // ********************************
- //
- // ... and variants of the above cases.
- return (bStart >= aStart && bStart <= aEnd)
- || (bEnd >= aStart && bEnd <= aEnd);
-}
-
-static bool areRectsMoreThanFullScreenApart(FocusType type, const LayoutRect&
curRect, const LayoutRect& targetRect, const LayoutSize& viewSize)
-{
- ASSERT(isRectInDirection(type, curRect, targetRect));
-
- switch (type) {
- case FocusTypeLeft:
- return curRect.x() - targetRect.maxX() > viewSize.width();
- case FocusTypeRight:
- return targetRect.x() - curRect.maxX() > viewSize.width();
- case FocusTypeUp:
- return curRect.y() - targetRect.maxY() > viewSize.height();
- case FocusTypeDown:
- return targetRect.y() - curRect.maxY() > viewSize.height();
- default:
- ASSERT_NOT_REACHED();
- return true;
- }
}
// Return true if rect |a| is below |b|. False otherwise.
@@ -644,7 +504,6 @@ void distanceDataForNode(FocusType type, const
FocusCandidate& current, FocusCan
if (areElementsOnSameLine(current, candidate)) {
if ((type == FocusTypeUp && current.rect.y() > candidate.rect.y()) ||
(type == FocusTypeDown && candidate.rect.y() > current.rect.y())) {
candidate.distance = 0;
- candidate.alignment = Full;
return;
}
}
@@ -660,22 +519,37 @@ void distanceDataForNode(FocusType type, const
FocusCandidate& current, FocusCan
LayoutPoint entryPoint;
entryAndExitPointsForDirection(type, currentRect, nodeRect, exitPoint,
entryPoint);
- LayoutUnit xAxis = exitPoint.x() - entryPoint.x();
- LayoutUnit yAxis = exitPoint.y() - entryPoint.y();
+ LayoutUnit xAxis = (exitPoint.x() - entryPoint.x()).abs();
+ LayoutUnit yAxis = (exitPoint.y() - entryPoint.y()).abs();
LayoutUnit navigationAxisDistance;
- LayoutUnit orthogonalAxisDistance;
+ LayoutUnit weightedOrthogonalAxisDistance;
+
+ // Bias and weights are put to the orthogonal axis distance calculation
+ // so aligned candidates would have advantage over partially-aligned ones
+ // and then over not-aligned candidates. The bias is given to not-aligned
+ // candidates with respect to size of the current rect. The weight for
+ // left/right direction is given a higher value to allow navigation on
+ // common horizonally-aligned elements. The hardcoded values are based on
+ // tests and experiments.
+ const int orthogonalWeightForLeftRight = 30;
+ const int orthogonalWeightForUpDown = 2;
+ int orthogonalBias = 0;
switch (type) {
case FocusTypeLeft:
case FocusTypeRight:
- navigationAxisDistance = xAxis.abs();
- orthogonalAxisDistance = yAxis.abs();
+ navigationAxisDistance = xAxis;
+ if (!rectsIntersectOnOrthogonalAxis(type, currentRect, nodeRect))
+ orthogonalBias = currentRect.height() / 2;
+ weightedOrthogonalAxisDistance = (yAxis + orthogonalBias) *
orthogonalWeightForLeftRight;
break;
case FocusTypeUp:
case FocusTypeDown:
- navigationAxisDistance = yAxis.abs();
- orthogonalAxisDistance = xAxis.abs();
+ navigationAxisDistance = yAxis;
+ if (!rectsIntersectOnOrthogonalAxis(type, currentRect, nodeRect))
+ orthogonalBias = currentRect.width() / 2;
+ weightedOrthogonalAxisDistance = (xAxis + orthogonalBias) * orthogo…
(message too large)
The CQ bit was checked by c.shu@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/797463003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://src.chromium.org/viewvc/blink?view=rev&revision=188642 |
