Text in bullet shows vertical-positioning discrepancy

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Assignee

Description

5 years ago
Posted file testcase
Unknown characters in bullet frame may have blurred border while those in normal text won't. This is because the text in bullet isn't snapped to pixel before rendering.

This bug has no effect in the current version since there is no unknown character generated by existing counter styles. However, it becomes a problem when @counter-style is implemented (bug 966166), as then the bullet can have arbitrary characters.

This bug causes reftests of counter-style failed in some platform: https://tbpl.mozilla.org/?tree=Try&rev=a70c73bdfaba

The attachment is a testcase.
Assignee

Comment 1

5 years ago
Posted image screenshot
Screenshot for the testcase
Assignee

Updated

5 years ago
Blocks: 966166
Actually, the problem is more general. If you look at the Windows8 reftest failures in that try run, they don't involve "unknown" characters (hexboxes); Win8 finds fonts for all the characters needed, yet it still shows a vertical-positioning discrepancy.

Possibly occurs when the bullet character(s) can't be found in the specified font-family, and so fallback occurs and finds a font with different line metrics?
Assignee

Comment 3

5 years ago
I pushed a patch [1] to the try server, and this patch seems to solve most of the problems [2]. But I'm not sure whether it is the correct place to fix this problem.

[1]: https://hg.mozilla.org/try/rev/5fc430278faf
[2]: https://tbpl.mozilla.org/?tree=Try&rev=c38bb7f3301a
Flags: needinfo?(jfkthame)
Assignee

Comment 4

5 years ago
Posted patch patch (obsolete) — Splinter Review
After investivating the code for normal text, I think nsLayoutUtils::GetSnappedBaselineY should be used instead of RoundAppUnitsToNearestDevPixels here. Otherwise, it should be good.

https://tbpl.mozilla.org/?tree=Try&rev=3906fe76495f
Attachment #8424232 - Flags: review?(jfkthame)
Flags: needinfo?(jfkthame)
Assignee

Comment 5

5 years ago
Posted patch patch (obsolete) — Splinter Review
This patch can be applied before the patchset for bug 966166.
Assignee: nobody → quanxunzhen
Attachment #8424232 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8424232 - Flags: review?(jfkthame)
Attachment #8424312 - Flags: review?(jfkthame)
Assignee

Updated

5 years ago
Summary: Unknown characters in bullet may have blurred border → Text in bullet shows vertical-positioning discrepancy
Comment on attachment 8424312 [details] [diff] [review]
patch

Review of attachment 8424312 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this looks good, thanks. Just one small nit to fix before it lands, please.

::: layout/generic/nsBulletFrame.cpp
@@ +425,3 @@
>      break;
>    }
> +  }

Are the extra braces really needed here? I don't think so, AFAICS; but if we do add them, we need to adjust the indentation.
Attachment #8424312 - Flags: review?(jfkthame) → review+
Assignee

Comment 7

5 years ago
Complain if you are unhappy with my code style.
Attachment #8424312 - Attachment is obsolete: true
Attachment #8424317 - Flags: review+
Attachment #8424317 - Flags: feedback?(jfkthame)
Comment on attachment 8424317 [details] [diff] [review]
patch, r=jfkthame

Review of attachment 8424317 [details] [diff] [review]:
-----------------------------------------------------------------

OK with me. I don't think there's a single "ideal" style here - I'd probably have arranged it differently, but this looks clear enough.
Attachment #8424317 - Flags: feedback?(jfkthame) → feedback+
Assignee

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/594aa4f3603a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.