No spacing between the bullet and text in a list-item

RESOLVED FIXED in mozilla32

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mats, Assigned: jfkthame)

Tracking

({regression, testcase})

Trunk
mozilla32
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
STR
1. load data:text/html,<ul><li>x

ACTUAL RESULTS
No spacing between the bullet and the "x"

EXPECTED RESULTS
Some spacing between the bullet and the "x"

ADDITIONAL INFORMATION
This is a regression in the last day or so.
The bug occurs in a local mozilla-inbound build on Linux with
a fresh profile.  It does NOT occur in Nightly 2014-05-28.
It's a regression of bug 1013160 which has not been merged into the trunk. Should I fix it in that bug or this bug?
This bug.
(Reporter)

Comment 3

4 years ago
I can confirm that backing out bug 1013160 locally fixes the problem.
Blocks: 1013160
OK.

After bug 1013160, numbers have a space appended as suffix instead of padding, however bullets are rendered in a different path which do not awared of the suffix. So what should the spacing depends on? Should we hardcoded some value for it? Or measure the width of space and add it to the padding? Or reintroduce some magic on padding?
Blocks: 966166
No longer blocks: 1013160
Blocks: 1013160
Actually, now that bug 1013160 is backed out, it could go in either bug.
Jonathan, what's your opinion about comment 4?
Flags: needinfo?(jfkthame)
(Reporter)

Comment 7

4 years ago
Created attachment 8430468 [details] [diff] [review]
wip

Something like this might be good enough.  (I guess it should assign
mPadding.left for RTL though)
(In reply to Mats Palmgren (:mats) from comment #7)
> Created attachment 8430468 [details] [diff] [review]
> wip
> 
> Something like this might be good enough.  (I guess it should assign
> mPadding.left for RTL though)

That seems good, thanks. Would you like to fix it? Or let me fix it in bug 1013160?
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Comment 9

4 years ago
You fixing it in bug 1013160 sounds fine to me :-)
(Assignee)

Comment 10

4 years ago
Yes, I guess simply adding fm->SpaceWidth() is fine. It means the spacing will be dependent on the font being used, but that's also the case for numbers now, so whatever... 

We should also make sure to add a reftest for this (both LTR and RTL) - unit testing should have caught this earlier.
Flags: needinfo?(jfkthame)
(In reply to Jonathan Kew (:jfkthame) from comment #10)
> Yes, I guess simply adding fm->SpaceWidth() is fine. It means the spacing
> will be dependent on the font being used, but that's also the case for
> numbers now, so whatever... 
> 
> We should also make sure to add a reftest for this (both LTR and RTL) - unit
> testing should have caught this earlier.

I don't have idea how to build a reftest for the spacing between this kind of bullets and text.
(Assignee)

Comment 12

4 years ago
(In reply to Xidorn Quan from comment #11)

> I don't have idea how to build a reftest for the spacing between this kind
> of bullets and text.

Hmm, yes... I thought perhaps we could compare this with a "fake" version generated using text spans, etc., but it's a bit tricky because the exact placement of the bullet is different here. Presumably that's because it is positioned based on bounding boxes rather than as a piece of text.
(Assignee)

Comment 13

4 years ago
Created attachment 8430753 [details] [diff] [review]
reftest for spacing between bullet and list item in <ul>.

Maybe something like this would be OK. It seems to work well for me locally; I've pushed it to try, to see if it survives across platforms: https://tbpl.mozilla.org/?tree=Try&rev=bb3e96298d11.
Attachment #8430753 - Flags: review?(matspal)
(Assignee)

Comment 14

4 years ago
Comment on attachment 8430753 [details] [diff] [review]
reftest for spacing between bullet and list item in <ul>.

Well, tryserver says that's too fragile in various cases. I'll see if we can do something more robust.
Attachment #8430753 - Flags: review?(matspal)
(Assignee)

Comment 15

4 years ago
Created attachment 8430884 [details] [diff] [review]
reftest for spacing between bullet and list item in <ul>.

Here's a version that should be much less fragile, with room for platform variations between fonts and with a couple of pixels' worth of padding on the background in case of antialiasing that extends beyond the bullet's nominal bounds. Looks to be coming up green on try: https://tbpl.mozilla.org/?tree=Try&rev=7b8ede1103f3, though not all results are in yet.
Attachment #8430884 - Flags: review?(matspal)
(Assignee)

Updated

4 years ago
Attachment #8430753 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8430884 - Flags: review?(matspal) → review+
(Assignee)

Comment 16

4 years ago
Reftest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b39cbf840af9

(The actual problem was fixed by updating the patch in bug 1013160.)
Assignee: nobody → jfkthame
Target Milestone: --- → mozilla32
https://hg.mozilla.org/mozilla-central/rev/b39cbf840af9
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.