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.

Sign up for GitHub

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

Jump to bottom

Dot in li element is not in the middle #478

Open
mndzielski opened this issue May 12, 2020 · 9 comments
Open

Dot in li element is not in the middle #478

mndzielski opened this issue May 12, 2020 · 9 comments
Assignees
@danfickle

Comments

@mndzielski
Copy link

Hi,

I'd like to report that dot in li element is align to top. In browser is align to middle. It's quite annoying.

Another issue is fact that dot is not fully rendering when font-size is large.

HTML example:

<head>
<style>
#e1 {
 font-size: 40px;
}

#e2 {
 font-size: 55px;
}
</style>
</head>
<body>

<ul>
<li id="e1">Hello world</li>
<li id="e2">Hello world</li>
</ul>

</body>
</html>

And result:

image
@syjer
Copy link
Contributor

syjer commented May 23, 2020

hi @mndzielski , I'll have a look

@mndzielski
Copy link
Author

mndzielski commented Aug 27, 2020 •
edited
Loading

@danfickle @syjer
Is there any update on this issue?
image

@syjer
Copy link
Contributor

syjer commented Aug 27, 2020

hi @mndzielski currently not, I was side tracked on other stuff, but I'll have look in this period.

@mndzielski
Copy link
Author

Ok,
so my temporary solution for that issue is:

ul { list-style: none; }

ul li::before { content: "\2022"; font-weight: bold; display: inline-block; width: 1em; margin-left: -1em; }

Hope this may help others

image

@syjer
Copy link
Contributor

syjer commented Aug 27, 2020

Currently trying to understand the issue of vertical positioning:

looking at the code in ListItemPainter.java:

int y = getReferenceBaseline(c, box) - (int)strutMetrics.getAscent() / 2  - marker.getDiameter() / 2;

seems to skew a little bit too much on the top.

Adding some code to visualize it:

c.getOutputDevice().drawDebugOutline(c, box, FSRGBColor.GREEN);
c.getOutputDevice().setColor(FSRGBColor.RED);
c.getOutputDevice().drawRect(x, getReferenceBaseline(c, box), 1000, 1);
c.getOutputDevice().setColor(FSRGBColor.BLUE);
c.getOutputDevice().drawRect(x, getReferenceBaseline(c, box) - (int)strutMetrics.getAscent(), 1000, 1);

We can see the issue of the vertical positioning:

Screenshot from 2020-08-27 18-45-13

Basically, we center exactly with the full line height, which is visually not exactly correct.

@syjer
Copy link
Contributor

syjer commented Aug 27, 2020

For the horizontal alignement:

if (style.getDirection() == IdentValue.LTR) {
            x += -marker.getLayoutWidth() + marker.getDiameter();
        }

Adding + marker.getDiameter(); seems to do the trick

@syjer
Copy link
Contributor

syjer commented Aug 27, 2020 •
edited
Loading

doing this in ListItemPainter.drawGlyph:

        if (style.getDirection() == IdentValue.LTR) {
            x += -marker.getLayoutWidth() + marker.getDiameter();
        }
        if (style.getDirection() == IdentValue.RTL){
            x += box.getMarkerData().getReferenceLine().getWidth() + marker.getLayoutWidth();
        }

        //

        c.getOutputDevice().drawDebugOutline(c, box, FSRGBColor.GREEN);

        c.getOutputDevice().setColor(FSRGBColor.RED);
        c.getOutputDevice().drawRect(x, getReferenceBaseline(c, box), 1000, 1);


        c.getOutputDevice().setColor(FSRGBColor.BLUE);


        int bottomLine = getReferenceBaseline(c, box);
        int top = bottomLine - (int) (strutMetrics.getAscent() / 1.5);
        c.getOutputDevice().drawRect(x, top, 1000, 1);
        //

        c.getOutputDevice().setColor(new FSRGBColor(255, 255, 0));
        c.getOutputDevice().drawRect(x, bottomLine - (bottomLine-top)/2, 1000, 1);
        //


        c.getOutputDevice().setColor(FSRGBColor.BLACK);
        int y = bottomLine - (bottomLine-top) /2 - marker.getDiameter() / 2 ;

Using 1.5 as a criteria for finding the "top" of the line does not seems to bad as a heuristic.

we get
Screenshot from 2020-08-27 19-11-22

which is quite decent.

l'll need to test with the RTL and look if there are any corner case.

WDYT @danfickle ?

(looking at the failing tests, the placement of the disc seems to be a little better now:

before:
columns-nested-unbalanced---1---expected

after:

columns-nested-unbalanced---1---actual

)

@syjer syjer mentioned this issue Aug 28, 2020
Merged
@danfickle
Copy link
Owner

@syjer, I'm happy to go with your judgement on this one. It does look better with your heuristic. I'm sure you're well ahead of me, but remember to test(at least informally) with at least a couple of fonts and font sizes.

Thanks again.

@syjer
Copy link
Contributor

syjer commented Aug 29, 2020 •
edited
Loading

@danfickle yes, I'm currently looking at the RTL case (and adding some fine tuning for the x positioning for the LTR one)

I've noticed that the list (ul, ol) in chrome&co have a css property "padding-inline-start: 40px" which, in function if it's a LTR or RTL case, will add the necessary padding on the right or left.

As far as I've noticed, we don't really support this property, as we define in our default css:

ol, ul, dir,
menu            { padding-left: 40px }

Which work really well for LTR, but clearly not for RTL.

So I'm wondering if I should fix first the LTR case and submit the PR, and then try to tackle the RTL one which seems more complex, as we need to support a new css property.

danfickle added a commit that referenced this issue Aug 31, 2020
@danfickle
Merge pull request #542 from syjer/478-li-style-decoration-positioning
3f42c9a 
#478 change list decoration placement logic
@danfickle danfickle self-assigned this Jan 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@danfickle danfickle

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@syjer @danfickle @mndzielski