Xidorn Quan [:xidorn] UTC+11 Assignee
	Description • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 1 • 10 years ago

Screenshot for the testcase

	Jonathan Kew [:jfkthame]
	Comment 2 • 10 years ago

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?

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 3 • 10 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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 4 • 10 years ago

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

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 5 • 10 years ago

This patch can be applied before the patchset for bug 966166.

	Jonathan Kew [:jfkthame]
	Comment 6 • 10 years ago

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.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 7 • 10 years ago

Complain if you are unhappy with my code style.

	Jonathan Kew [:jfkthame]
	Comment 8 • 10 years ago

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.

	Carsten Book [:Tomcat]
	Comment 9 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/594aa4f3603a

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/594aa4f3603a