Fix the rightOf and below conditions for overlapping case.

Fix the rightOf and below conditions for overlapping case.

For the overlapping rects, |a| is considered to be below |b| if both edges of |a|
are below the respective ones of |b|. In the code, we should make sure the rects
are actually overlapping. For example,
 __       __
| a|  __ |b'|
|__| | b||  |
     |__||__|
Before the patch, |b| and |b'| are considered below |a|, which is not intuitively
true. With the patch, they are no longer below |a|.
The rightOf-case is handled in an analog fashion.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186731

[c.shu, 2014-12-03 20:49:13]

c.shu@samsung.com changed reviewers:
+ kolczyk@opera.com, tkent@chromium.org

[Krzysztof Olczyk, 2014-12-04 09:21:05]

lgtm

[c.shu, 2014-12-04 14:11:39]

The CQ bit was checked by c.shu@samsung.com

[commit-bot: I haz the power, 2014-12-04 14:12:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773333002/1

[commit-bot: I haz the power, 2014-12-04 14:12:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-04 14:12:39]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[c.shu, 2014-12-04 14:16:18]

On 2014/12/04 14:12:39, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

tkent, could you rubberstamp this CL? thanks

[tkent, 2014-12-05 01:14:37]

tkent@chromium.org changed reviewers:
+ fs@opera.com
- tkent@chromium.org

[tkent, 2014-12-05 01:14:37]

I don't review snav patches.  +fs

[fs, 2014-12-05 09:42:04]

What does this fix, could you add a test (rather than just removing one -
without motivation)?

TBH I think this is more a matter of a confusing comment - ie. where it says
"overlapped" in the comments it should say "overlapping y-extents" (resp.
"x-extents") or some such.

Also, please line-wrap the description.

[c.shu, 2014-12-05 16:12:14]

On 2014/12/05 09:42:04, fs wrote:
> What does this fix, could you add a test (rather than just removing one -
> without motivation)?
> 
> TBH I think this is more a matter of a confusing comment - ie. where it says
> "overlapped" in the comments it should say "overlapping y-extents" (resp.
> "x-extents") or some such.
> 
> Also, please line-wrap the description.
 __       __
| a|  __ |b'|
|__| | b||  |
  __ |__||__|
 | c|
 |__|
My intention in this patch is to remove the cases above that consider |b| or
|b'| is below |a|. They can be simply reached by Right key. It is fine though if
there a |c|, the Down key will set the focus to |c| since it's partially aligned
to |a| and |b| is not aligned. But I think giving a fully-aligned but
very-far-distance element higher priority than a partially-aligned but
much-closer element is not desirable. What do you think?

[Krzysztof Olczyk, 2014-12-08 07:36:39]

On 2014/12/05 at 16:12:14, c.shu wrote:
> On 2014/12/05 09:42:04, fs wrote:
> > What does this fix, could you add a test (rather than just removing one -
> > without motivation)?
> > 
> > TBH I think this is more a matter of a confusing comment - ie. where it says
> > "overlapped" in the comments it should say "overlapping y-extents" (resp.
> > "x-extents") or some such.
> > 
> > Also, please line-wrap the description.
>  __       __
> | a|  __ |b'|
> |__| | b||  |
>   __ |__||__|
>  | c|
>  |__|
> My intention in this patch is to remove the cases above that consider |b| or
|b'| is below |a|. They can be simply reached by Right key. It is fine though if
there a |c|, the Down key will set the focus to |c| since it's partially aligned
to |a| and |b| is not aligned. But I think giving a fully-aligned but
very-far-distance element higher priority than a partially-aligned but
much-closer element is not desirable. What do you think?

@fs: c.shu is editing the code I added to spatnav and I generally agree this is
a desirable change. Before my change the overlapping elements were never
reachable because they were neither right of nor below. But looks like the
condition I added
was to permissive and as in the description above, b's are easily reachable with
right-of relation and it is not very intuitive to consider them right-of too.

[fs, 2014-12-08 09:55:51]

On 2014/12/08 07:36:39, Krzysztof Olczyk wrote:
> On 2014/12/05 at 16:12:14, c.shu wrote:
> > On 2014/12/05 09:42:04, fs wrote:
> > > What does this fix, could you add a test (rather than just removing one -
> > > without motivation)?
> > > 
> > > TBH I think this is more a matter of a confusing comment - ie. where it
says
> > > "overlapped" in the comments it should say "overlapping y-extents" (resp.
> > > "x-extents") or some such.
> > > 
> > > Also, please line-wrap the description.
> >  __       __
> > | a|  __ |b'|
> > |__| | b||  |
> >   __ |__||__|
> >  | c|
> >  |__|
> > My intention in this patch is to remove the cases above that consider |b| or
> |b'| is below |a|. They can be simply reached by Right key. It is fine though
if
> there a |c|, the Down key will set the focus to |c| since it's partially
aligned
> to |a| and |b| is not aligned. But I think giving a fully-aligned but
> very-far-distance element higher priority than a partially-aligned but
> much-closer element is not desirable. What do you think?
> 
> @fs: c.shu is editing the code I added to spatnav and I generally agree this
is
> a desirable change. Before my change the overlapping elements were never
> reachable because they were neither right of nor below. But looks like the
> condition I added
> was to permissive and as in the description above, b's are easily reachable
with
> right-of relation and it is not very intuitive to consider them right-of too.

Now this certainly counts as a motivation, and should preferably go in the
description/commit message. Add a test to that and we're good to go =).

[c.shu, 2014-12-08 17:10:07]

> Now this certainly counts as a motivation, and should preferably go in the
> description/commit message. Add a test to that and we're good to go =).

Thanks for the review, guys. I updated the description and added new tests.

[fs, 2014-12-08 17:17:06]

Thanks, lgtm.

But please line-wrap the description and mention that the rightOf-case is
handled in an analog fashion.

[c.shu, 2014-12-08 19:53:16]

The CQ bit was checked by c.shu@samsung.com

[commit-bot: I haz the power, 2014-12-08 19:53:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773333002/20001

[c.shu, 2014-12-08 19:55:01]

On 2014/12/08 17:17:06, fs wrote:
> Thanks, lgtm.
> 
> But please line-wrap the description and mention that the rightOf-case is
> handled in an analog fashion.

thanks! done.

[commit-bot: I haz the power, 2014-12-08 21:28:18]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186731