mode/hint: Add hints-alignment-x slot

[MaxGyver83]

Description

Add hints-alignment-x slot to allow placing the hint overlays to the left or the right of a link. Possible values: :left, :on-top (current behavior, default), :right.

Fixes #3179.

Screenshots

:left:

:on-top:

:right:

Discussion

The left alignment is achieved by setting transform: translate(-100%, 0);. This translation is applied after ensuring that style.left is ≥ 0. This is why some hints may be not completely visible (like AA in the first screenshot). Any ideas how to fix? UPDATE: This is now fixed by checking the position of all hints in a loop after they were added to the DOM. AFAIU, the width of a hint overlay isn't known until then.

Checklist:

Everything in this checklist is required for each PR. Please do not approve a PR that does not have all of these items.

• Git hygiene:
  • I have pulled from master before submitting this PR
  • There are no merge conflicts.
• My code follows the style guidelines for Common Lisp code. See:
  • Norvig & Pitman's Tutorial on Good Lisp Programming Style (PDF)
  • Google Common Lisp Style Guide
• I have performed a self-review of my own code.
• My code has been reviewed by at least one peer. (The peer review to approve a PR counts. The reviewer must download and test the code.)
• Documentation:
  • All my code has docstrings and :documentations written in the aforementioned style. (It's OK to skip the docstring for really trivial parts.)
  • I have updated the existing documentation to match my changes.
  • I have commented my code in hard-to-understand areas.
  • I have updated the changelog.lisp with my changes if it's anything user-facing (new features, important bug fix, compatibility breakage).
    • Changelog update should be a separate commit.
  • I have added a migration.lisp entry for all compatibility-breaking changes.
  • (If this changes something about the features showcased on Nyxt website) I have these changes described in the new/existing article at Nyxt website or will notify one of maintainters to do so.
• Compilation and tests:
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works. (If possible.)
  • I ran the tests locally ((asdf:test-system :nyxt) and (asdf:test-system :nyxt/gi-gtk)) and they pass.

[aadcg]

@MaxGyver83 we need to account for the fact that the hints must be drawn within limits. See the case on the top right in the screenshot below. That can be handled via ps:max.

[MaxGyver83]

@MaxGyver83 we need to account for the fact that the hints must be drawn within limits. See the case on the top right in the screenshot below. That can be handled via ps:max.

I'm not sure how to do this. Lets say document.body.clientWidth is 1920. And the position of the KI hint overlay is 1900. I don't know if 1900 is too much. I need the width of the hint overlay to calculate if the right border is within the viewport. (ps:chain window (get-computed-style element)) seems to be not yet available during the creation of element.

Should I correct the position afterwards (after calling create-hint-overlay)?

UPDATE: Now I'm fixing the position of hint overlays in a second loop (after the hint elements were added to the DOM).

[aadcg]

@MaxGyver83 I'm coming back to this PR in a week. Thanks.

[aadcg]

@MaxGyver83 I think this is going in a good direction but the implementation is too cumbersome - the second loop seems inappropriate.

In the same way that ps:max is being used to ensure that the top-left corner is within viewport, I think that a similar strategy could be used as to ensure that the bottom-right corner is too via window.innerWidth and window.innerHeight.

[MaxGyver83]

@MaxGyver83 I think this is going in a good direction but the implementation is too cumbersome - the second loop seems inappropriate.

In the same way that ps:max is being used to ensure that the top-left corner is within viewport, I think that a similar strategy could be used as to ensure that the bottom-right corner is too via window.innerWidth and window.innerHeight.

@aadcg :

• The default (hint-alignment-x = :on-top) is simple: The top-left corner of the hint is set and must be >= 0.
• :left: Here, the top-left corner of the hint overlay is set and then it's shifted by 50% of its width to the left. But this width is unknown. This is why I don't know the new top-left position which would be one argument of ps:max.
• :right: Here, the top-left corner of the hint overlay is set and the right edge might be outside the viewport. Same problem as for :left: The width of the hint overlay is unknown and the position of the top-right (or bottom-right) corner can't be calculated.

This is why I let the renderer do the calculations and check (and adjust) the values afterwards (second loop).
Do you know how I can calculate the width of the hint overlays? It depends on the number of characters, which characters are used (if the font isn't monospace) and all the CSS (paddings, margins, border width).

UPDATE:
I have tried using HTMLElement: offsetWidth property but it returns always 0 in the first loop. I think at this point the actual width of the hint overlays is not yet determined.

UPDATE2:
@aadcg: The current hint overlay implementation guarantees only that hints don't exceed the left border of the viewport, but it can happen for the right side: Check the hint overlay for the "C" link on this hint test page. Same problem: We don't know if the right side will be outside the viewport. (This case is probably very seldom as it happens only for very short links on the very right side of the page.)

[aadcg]

@MaxGyver83 I've thought again about this and my conclusions seem to be aligned with yours.

Given issue #3250, which impacts hint mode before this PR, it makes little sense to request from your PR that all hints must be fully drawn in viewport. They should be, however, partially visible. I believe that the PR meets this requirement, even if the added loop is deleted. I'm sorry that I have requested from your PR to meet a goal that we don't currently meet, as I was unaware of #3250.

I suggest deleting the aforementioned loop and reviewing the code once again. create-hint-overlay became a rather complex function, and it would be great to abstract the key ideas into smaller pieces. Your description of how the location of the hints is computed should be clear in the implementation. For instance, perhaps a small function that implements the idea behind (ps:@ element style transform) "translate(-100%, 0)") paired with a proper docstring would made the implementation more readable.

Does that make sense? Thanks.

[MaxGyver83]

@MaxGyver83 I've thought again about this and my conclusions seem to be aligned with yours.

Given issue #3250, which impacts hint mode before this PR, it makes little sense to request from your PR that all hints must be fully drawn in viewport. They should be, however, partially visible. I believe that the PR meets this requirement, even if the added loop is deleted. I'm sorry that I have requested from your PR to meet a goal that we don't currently meet, as I was unaware of #3250.

I suggest deleting the aforementioned loop and reviewing the code once again. create-hint-overlay became a rather complex function, and it would be great to abstract the key ideas into smaller pieces. Your description of how the location of the hints is computed should be clear in the implementation. For instance, perhaps a small function that implements the idea behind (ps:@ element style transform) "translate(-100%, 0)") paired with a proper docstring would made the implementation more readable.

Does that make sense? Thanks.

Sounds good. I'll update this pull request.

[MaxGyver83]

I suggest deleting the aforementioned loop ...

Done in commit 756bdd7.

I have also removed the transform/translate part. This was only a workaround to set the hint's right edge/border position. I think it clearer to set the style.right position using window.innerWidth (I think you mentioned this before).

Now, I check the position for a min and a max value: It should be >= 0 and not too close to the window border to make sure the hints are at least partly visible. I'm using now a magic number (25 px). This is probably better than nothing (until we have found a better solution).

create-hint-overlay became a rather complex function, and it would be great to abstract the key ideas into smaller pieces. Your description of how the location of the hints is computed should be clear in the implementation. For instance, perhaps a small function that implements the idea behind (ps:@ element style transform) "translate(-100%, 0)") paired with a proper docstring would made the implementation more readable.

This is still open. I tried this at first for the example you mentioned but without success. I guess I have to copy the element as argument to a new function and then return the modified element. Can I pass an element pointer/reference?

[aadcg]

@MaxGyver83, sorry for the late reply. With respect to the functionality itself, I think it's good enough.

My suggestion is to improve the readability and maintainability of create-hint-overlay, which has become too long and cumbersome. It could benefit from being factored into smaller parts, in particular the logic behind setting the style depending on the value of hints-alignment-x.

I guess I have to copy the element as argument to a new function and then return the modified element. Can I pass an element pointer/reference?

You don't need to return the element if you're just interested in the side effects.

(defun test (element) (setf (ps:@ element style top) "2px") nil)

To check how it compiles to JS, pass it as an argument to ps:ps and see that it returns the JS code below.

function test(element) {
    element.style.top = '2px';
    return null;
};

---

Let me know if you're interested in giving it a try. Regardless, one of us can take over and finish this PR.

[MaxGyver83]

You don't need to return the element if you're just interested in the side effects.

(defun test (element) (setf (ps:@ element style top) "2px") nil)

To check how it compiles to JS, pass it as an argument to ps:ps and see that it returns the JS code below.

function test(element) {
    element.style.top = '2px';
    return null;
};

Thank you!

Let me know if you're interested in giving it a try. Regardless, one of us can take over and finish this PR.

I'll give it a try! (I have started with a rebase + squash.)

no longer relevant

[MaxGyver83]

@aadcg:
Here I'm trying to move some code from create-hint-overlay to a new function set-alignment:
MaxGyver83:nyxt:hints-next-to-links-dev
The build fails with:

Unhandled SIMPLE-ERROR in thread #<SB-THREAD:THREAD tid=50675 "main thread" RUNNING
                                    {10013D0133}>:
  Compilation warning: The function PARENSCRIPT:CHAIN is called with two arguments, but wants exactly one.

ps:chain is called with multiple arguments in many places. Can you tell me what's wrong here?

[MaxGyver83]

Regardless, one of us can take over and finish this PR.

This pull request has been open for quite a while. If you want to get this merged quickly, it's no problem for me if you finish this PR.

[aadcg]

@MaxGyver83 the issue is that you're writing a regular CL function using parenscript syntax. The defun clause must be enclosed by define-parenscript-async.

I'll take over and finish this PR in time for the next 3-series release.

Thanks @MaxGyver83!

[aadcg]

Superseded by #3302.

Thanks @MaxGyver83!