Skip to content

Commit

Permalink
Auto merge of #29676 - Loirooriol:text-decoration-px, r=mrobinson
Browse files Browse the repository at this point in the history
Round text decoration thickness to >= 1 device pixels

Previously the thickness coming from font metrics could be something like 0.7px, so with 1dppx it would be painted as either 1 or 0 device pixels.

Enforcing at least 1 device pixel ensures that the decoration will be visible, and rounding to an integral amount of device pixels ensures that the thickness won't vary depending on the position.

The specification requires this behavior when text-decoration-thickness is set to a length or percentage. It's not clear if it should also happen by default, but this seems to match other browsers (except for WebKit rounding up instead of to the nearest integer).

The test text-decoration-thickness-from-zero-sized-font.html is now failing because of #29675.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #29668 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because the exact appearance of text decorations is UA-dependent, so testing doesn't seem much feasible.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
  • Loading branch information
bors-servo committed Apr 27, 2023
2 parents 0e8d0b4 + 668c4c8 commit 3e3bd9c
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 6 deletions.
18 changes: 15 additions & 3 deletions components/layout/display_list/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2064,6 +2064,15 @@ impl Fragment {

// Create display items for text decorations.
let text_decorations = self.style().get_inherited_text().text_decorations_in_effect;
let dppx = state
.layout_context
.style_context
.device_pixel_ratio()
.get();
let round_to_nearest_device_pixel = |value: Au| -> Au {
// Round to the nearest integer device pixel, ensuring at least one device pixel.
Au::from_f32_px((value.to_f32_px() * dppx).round().max(1.0) / dppx)
};

let logical_stacking_relative_content_box = LogicalRect::from_physical(
self.style.writing_mode,
Expand All @@ -2077,7 +2086,8 @@ impl Fragment {
stacking_relative_box.start.b = logical_stacking_relative_content_box.start.b +
metrics.ascent -
metrics.underline_offset;
stacking_relative_box.size.block = metrics.underline_size;
stacking_relative_box.size.block =
round_to_nearest_device_pixel(metrics.underline_size);
self.build_display_list_for_text_decoration(
state,
&text_color,
Expand All @@ -2089,7 +2099,8 @@ impl Fragment {
// Overline
if text_decorations.overline {
let mut stacking_relative_box = logical_stacking_relative_content_box;
stacking_relative_box.size.block = metrics.underline_size;
stacking_relative_box.size.block =
round_to_nearest_device_pixel(metrics.underline_size);
self.build_display_list_for_text_decoration(
state,
&text_color,
Expand Down Expand Up @@ -2144,7 +2155,8 @@ impl Fragment {
let mut stacking_relative_box = logical_stacking_relative_content_box;
stacking_relative_box.start.b =
stacking_relative_box.start.b + metrics.ascent - metrics.strikeout_offset;
stacking_relative_box.size.block = metrics.strikeout_size;
stacking_relative_box.size.block =
round_to_nearest_device_pixel(metrics.strikeout_size);
self.build_display_list_for_text_decoration(
state,
&text_color,
Expand Down
11 changes: 8 additions & 3 deletions components/layout_2020/display_list/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,11 @@ impl Fragment {

let color = fragment.parent_style.clone_color();
let font_metrics = &fragment.font_metrics;
let dppx = builder.context.style_context.device_pixel_ratio().get();
let round_to_nearest_device_pixel = |value: Length| -> Length {
// Round to the nearest integer device pixel, ensuring at least one device pixel.
Length::new((value.px() * dppx).round().max(1.0) / dppx)
};

// Underline.
if fragment
Expand All @@ -220,7 +225,7 @@ impl Fragment {
{
let mut rect = rect;
rect.origin.y = rect.origin.y + font_metrics.ascent - font_metrics.underline_offset;
rect.size.height = font_metrics.underline_size;
rect.size.height = round_to_nearest_device_pixel(font_metrics.underline_size);
self.build_display_list_for_text_decoration(fragment, builder, &rect, color);
}

Expand All @@ -230,7 +235,7 @@ impl Fragment {
.contains(TextDecorationLine::OVERLINE)
{
let mut rect = rect;
rect.size.height = font_metrics.underline_size;
rect.size.height = round_to_nearest_device_pixel(font_metrics.underline_size);
self.build_display_list_for_text_decoration(fragment, builder, &rect, color);
}

Expand All @@ -252,7 +257,7 @@ impl Fragment {
let mut rect = rect;
rect.origin.y = rect.origin.y + font_metrics.ascent - font_metrics.strikeout_offset;
// XXX(ferjm) This does not work on MacOS #942
rect.size.height = font_metrics.strikeout_size;
rect.size.height = round_to_nearest_device_pixel(font_metrics.strikeout_size);
self.build_display_list_for_text_decoration(fragment, builder, &rect, color);
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[text-decoration-thickness-from-zero-sized-font.html]
expected: FAIL
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[text-decoration-thickness-from-zero-sized-font.html]
expected: FAIL

0 comments on commit 3e3bd9c

Please sign in to comment.