Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-nav-1] Remove kAlignWeight? #4567

Open
hugoholgersson opened this issue Dec 6, 2019 · 5 comments
Open

[css-nav-1] Remove kAlignWeight? #4567

hugoholgersson opened this issue Dec 6, 2019 · 5 comments

Comments

@hugoholgersson
Copy link

Today Alignment() always returns a value between 0 and 5. [1]

That's a very small number so Alignment is, in practice, only used to break ties (when two candidates are at the same distance we take the one that is more aligned [2]).

As we only use alignment to break ties, alignment might as well be a number between 0 and 1. This means we can remove the multiplication of alignWeight from the formula?

[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/page/spatial_navigation.cc?q=file:spatial+symbol:Alignment&sq=package:chromium
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1594724/11/third_party/blink/web_tests/fast/spatial-navigation/snav-aligned-insiders.html

@anawhj anawhj closed this as completed Dec 10, 2019
@anawhj anawhj reopened this Dec 10, 2019
@anawhj
Copy link
Member

anawhj commented Dec 10, 2019

Great question! I would explain how to come up with kAlignWeight: 5 as a heuristic estimation.

The alignment factor has recently added to the distance formula for prioritizing quite aligned candidates. However, what the new factor applies into the existing distance formula has several consideration. As you mentioned, when two candidates are at the same distance, a number between 0 and 1 would have no problem. But we need to consider the case of none the same distance as per [1] of the following image.

Without the alignment factor in [1], B is the best candidate from A with a right direction, but we want it to C as the best candidate. That's one of the reasons that we added the alignment factor, so the value of kAlignWeight was a number much more than 5 at first for even covering 490px from A to B.

However, we found unreachability and jumping cases like as [2] above. With the alignment factor more than 5 in [2], F could be the best candidate from D with a down direction so that E becomes an unreachable element from D and F. In the text-based web contents (e.g. Wikipedia), it could happen a lot between hyperlinks. In the background, we set kAlignWeight to 5 for avoiding the unreachable case.

That's the history about kAlignWeight: 5. It's a heuristic estimation, so seems no perfect solution on which all people agree. If I miss something or you have another great idea, please feel free to share it.

@hugoholgersson
Copy link
Author

Thanks for explaining how you came up with 5.

From also reading http://crbug.com/1032430, I understand you chose 5 to compensate for the removal of navigation_axis_distance?

Perhaps it's cleaner to keep navigation_axis_distance in the formula (Chrome has had it for years) and make kAlignWeight = 1?

Having hardcoded pixels in the formula seems unscalable anyway?

To better handle different layouts, zoom levels etc the forumla should, ideally, only use ratios based on actual rects (focus candidate's and current focus' size) - not hardcoded pixel values?

@jeonghee27
Copy link
Contributor

jeonghee27 commented Dec 11, 2019

navigation_axis_distance and kAlignWeight = 5 were actually determined Independently.

We thought navigation_axis_distance duplicated with the Euclidiant distance.
And we wanted to consider the degree of aligned when they were at the same distance..

In case you suggested (keep navigation_axis_distance and kAlignWeight = 1? ) on example [1] Hyojin attached,

B's navigation_axis_distance will be 499.
C's navigation_axis_distance will be 500.
B's kAlignWeight might be 0.1
C's kAlignWeight might be 0.9

Distance = ... + navigation_axis_distance - kAlignWeight
So, B will be best target in case. But we hope C.

I agree your below point.

To better handle different layouts, zoom levels etc the forumla should, ideally, only use ratios based on actual rects (focus candidate's and current focus' size) - not hardcoded pixel values?

I'm worried that 1 might be too small.

@anawhj
Copy link
Member

anawhj commented Dec 24, 2019

I summarize the current status on some factor and constant of SpatNav distance formula.

  • Spec: navigation_axis_distance(DROPPED), kAlignWeight(5)
  • Blink: navigation_axis_distance(KEEP), kAlignWeight(5)
  • Hugo's suggestion: navigation_axis_distance(KEEP), kAlignWeight(1)

With Hugo's suggestion, the focus moves from A to B on the example [1] above in an improper way.
I agree to keep navigation_axis_distance which has been maintained in Blink for years though.

From css-nav specification point of view, the factor and weight has been defined as non-normative,
so we could keep the current SpatNav distance formula in Blink as is without any change for now.

Please share your opinion on it, or you can close this now.

@hugoholgersson
Copy link
Author

I found an example where Chrome's current formula doesn't work well:

not_aligned_enough

When going right from "Agreements", I'd expect focus to stay on the same line (and go to "export controls"). Instead we go to the link on the line below because it's, with the current formula, a tiny bit "closer".

If you can find a formula (preferably without constants) that works in this example, that would be lovely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants