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

Port to the Parley cursor rework #755

Merged
merged 12 commits into from
Nov 28, 2024
Merged

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Nov 22, 2024

The upstream PR has now been merged: linebender/parley#170

I've had to add a hack to workaround linebender/parley#186. This occurs because of the first pass of layout with zero available area.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @dfrg it seems like something has changed with line height? Is this expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t change individual line height but updated the overall layout dimensions to use the max_coord of each line so that the layout box encompasses the full text. There’s obviously something funky going on with how line metrics are calculated because these should be the same but the web compatible line height changes added some complex interactions and I haven’t had time to investigate that.

Copy link
Member Author

@DJMcNab DJMcNab Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I've just done some quick testing, and descenders still seem to be cut off when clipping to the layout box. See also https://xi.zulipchat.com/#narrow/channel/205635-text/topic/Some.20funny.20typography.20with.20Masonry

The descenders being cut off is:
image

@DJMcNab DJMcNab marked this pull request as ready for review November 28, 2024 09:37
@DJMcNab DJMcNab changed the title Port to linebender/parley#170 Port to the Parley cursor rework Nov 28, 2024
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for reviewers: These changes are only ports of changes done in linebender/parley#170, except for the set_width hack

@mwcampbell
Copy link
Contributor

My quick accessibility tests are all working.

@tomcur tomcur mentioned this pull request Nov 28, 2024
@mwcampbell
Copy link
Contributor

Wrote too soon. I just caught two accessibility bugs that were probably actually introduced in #616:

  1. The text area doesn't have the read only flag set if it's non-editable, like the multi-line document in the mason example.
  2. When calling PlainEditor::accessibility or LayoutAccessibility::build_nodes, the X and Y offsets need to be in window coordinates, with the scale factor applied, like the bounding rectangles that are set by the Masonry widget infrastructure. Perhaps this could be documented more clearly in AccessKit or Parley.

@DJMcNab
Copy link
Member Author

DJMcNab commented Nov 28, 2024

The text area doesn't have the read only flag set if it's non-editable, like the multi-line document in the mason example.

Ouch... that's a really embarrassing mistake. I think this was #754

Thanks for the clarification on point 2. I think a tiny bit of documentation would be good.

@mwcampbell
Copy link
Contributor

Looks like I need to modify Parley to take the scale factor into account when calculating the run bounding rectangles and character widths for AccessKit.

@mwcampbell
Copy link
Contributor

Alternatively, I could eliminate the need to apply the scale factor to individual node bounding rectangles, which would complicate the Parley API, if I instead modify Masonry to set an AccessKit transform on the root node.

@DJMcNab
Copy link
Member Author

DJMcNab commented Nov 28, 2024

if I instead modify Masonry to set an AccessKit transform on the root node.

I think that would be the idiomatic way within the current form of Xilem (there is a future question about pixel alignment)...

@mwcampbell
Copy link
Contributor

OK, let's land #764, then rebase this, and then you won't need to include the scale factor when calculating the x and y offsets.

Copy link
Member

@tomcur tomcur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm able to edit the text of Prose through selection and space, but I don't think that's an issue introduced here.

@DJMcNab DJMcNab added this pull request to the merge queue Nov 28, 2024
@mwcampbell
Copy link
Contributor

Now my quick accessibility tests all pass.

Merged via the queue into linebender:main with commit 4950e56 Nov 28, 2024
17 checks passed
@DJMcNab DJMcNab deleted the parley-cursor-rework branch November 28, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants