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

Fix bullet point's vertical alignment of list items #1315

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Dec 29, 2020

Fixes #1314

Description

Align the bullet point vertically with the text line.

How

The marker's (bullet point) render rectangle calculation has been changed.

Origin rectangle

The line fragment rectangle was used as the origin for the calculation, after some checks I realised that unfortunately it's not providing a uniform size (first and last items are providing different heights). For this reason I swapped it to use the used rectangle version (lineFragmentUsedRect) which it provides the same height for all items.

Vertical alignment

The Y offset now is calculated for center it vertically which also required to set the proper height, calculated from the glyph size.

Screenshot

fix_emoji_bullet_point.mp4

How to test

The issue was spotted in the WordPress app which uses a different font family so the issue has to be tested there or modifying the default font for the content in Aztec's example project.

  1. Go to a post/page.
  2. Create list with different items.
  3. Add a emoji character to one of the items.

Expected result:
Bullet points are properly aligned with the text line.

@SergioEstevao
Copy link
Contributor

Thanks for the PR @fluiddot !
I had a look at the code and it looks correct, I also tested on the Example app and it looks the lists are drawing correctly.

@guarani You may want to test these changes using Gutenberg to see if all is ok.

@guarani
Copy link
Contributor

guarani commented Dec 30, 2020

Thanks for the PR @fluiddot !
I had a look at the code and it looks correct, I also tested on the Example app and it looks the lists are drawing correctly.

@guarani You may want to test these changes using Gutenberg to see if all is ok.

Thanks for taking a look @SergioEstevao! It's good to have an Aztec expert take a look 😄

I'll try this out tomorrow, as I didn't get the chance to look today while I was wrapping up Xposts in Gutenberg.

@fluiddot fluiddot added this to the Next Stable milestone Dec 30, 2020
@fluiddot fluiddot marked this pull request as ready for review December 30, 2020 08:14
@fluiddot
Copy link
Contributor Author

Ok, I've marked the PR as ready for review 🎊 .
Thanks @SergioEstevao and @guarani for reviewing it ❤️ !

Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

I tested in Gutenberg and it looks good. Like you mention here, the line still jumps, but now the bullet point also moves down to stay vertically centered, which I think is the most important thing. I tested nested lists and both ordered and unordered lists, all works well from my perspective.

On an iPhone 11 running the Gutenberg demo app with this branch of Aztec, I saw there's a slight delay between the line jumping down and the bullet point jumping down. It's only a few hundred milliseconds I'd say, but still noticeable. I'd say it's not worth looking into now, but thought I'd mention it.

Copy link
Contributor

@SergioEstevao SergioEstevao left a comment

Choose a reason for hiding this comment

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

Looking good!

@guarani
Copy link
Contributor

guarani commented Jan 4, 2021

@fluiddot can this be merged and a new release made on https://github.com/wordpress-mobile/AztecEditor-iOS/releases? I think when that's ready, we can include this in the new Gutenberg Mobile release that is going to be cut today.

@fluiddot fluiddot modified the milestones: Next Stable, 1.19.4 Jan 4, 2021
@fluiddot
Copy link
Contributor Author

fluiddot commented Jan 4, 2021

I've done some tests and I found out that although the bullet point is vertically centered, in some cases it still looks like a bit displaced to the bottom.

This PR improves the way the bullet point is rendered compared to the original bug but unfortunately it doesn't fix it 100%, I think we should review in the future how the bullet point position is calculated.

Examples

Ordered list:
In this case the text line is being pushed some pixels down when emojis are added.

Screenshot 2021-01-04 at 15 32 59

Unordered list:
Similar to the previous case the text line is displaced.

Screenshot 2021-01-04 at 15 36 25

Text box

As you can see in the following screenshots the bullet point is centered, the problem is that the text box has more space at the top compared to the bottom.

Text without emojis:

Screenshot 2021-01-04 at 15 40 50

Text with emojis:

Screenshot 2021-01-04 at 15 38 35

@fluiddot
Copy link
Contributor Author

fluiddot commented Jan 5, 2021

@fluiddot can this be merged and a new release made on https://github.com/wordpress-mobile/AztecEditor-iOS/releases? I think when that's ready, we can include this in the new Gutenberg Mobile release that is going to be cut today.

After being discussed, since this is a visual glitch it's not so critical to fix it in the new release, we can wait for the next one. This will give me time to rethink the approach and apply some changes in the following days.

@fluiddot fluiddot removed this from the 1.19.4 milestone Jan 5, 2021
@fluiddot fluiddot marked this pull request as draft January 5, 2021 10:58
@fluiddot fluiddot self-assigned this Jan 20, 2021
@kenoButler
Copy link

Hello, is this fix still coming?

@fluiddot
Copy link
Contributor Author

Hello, is this fix still coming?

I changed this PR to draft a long time ago (#1315 (comment)), my idea was to explore other approaches to solve the issues I spotted (#1315 (comment)). However, it's been a while since I stopped working in the PR and I'm not planning to resume it in the short term. I invite other contributors to take over it if there's interest in addressing the issue, thanks 🙇 !

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

Successfully merging this pull request may close these issues.

Text with emojis is not properly aligned with the bullet point
5 participants