From af0dac4e28ca8a8a8726af0f9e58a665532b1ffb Mon Sep 17 00:00:00 2001 From: avitex Date: Tue, 8 Feb 2022 22:38:46 +1100 Subject: [PATCH 1/4] Remove dead comment --- piet-web/src/text/grapheme.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/piet-web/src/text/grapheme.rs b/piet-web/src/text/grapheme.rs index b459bb943..e36fda9de 100644 --- a/piet-web/src/text/grapheme.rs +++ b/piet-web/src/text/grapheme.rs @@ -4,11 +4,6 @@ use web_sys::CanvasRenderingContext2d; use super::hit_test_line_position; -// currently copied and pasted from cairo backend. -// -// However, not cleaning up because cairo and web implementations should diverge soon; and putting this -// code in `piet` core doesn't really make sense as it's implementation specific. -// /// get grapheme boundaries, intended to act on a line of text, not a full text layout that has /// both horizontal and vertial components pub(crate) fn get_grapheme_boundaries( From 7fc9c51ad7a899673396d7286cc3a65327291774 Mon Sep 17 00:00:00 2001 From: avitex Date: Tue, 8 Feb 2022 22:32:51 +1100 Subject: [PATCH 2/4] Cache font setting --- piet-web/src/lib.rs | 2 +- piet-web/src/text.rs | 51 ++++++++++++++++++++------------------------ 2 files changed, 24 insertions(+), 29 deletions(-) diff --git a/piet-web/src/lib.rs b/piet-web/src/lib.rs index 78c8989fd..76fd8f82e 100644 --- a/piet-web/src/lib.rs +++ b/piet-web/src/lib.rs @@ -236,7 +236,7 @@ impl RenderContext for WebRenderContext<'_> { fn draw_text(&mut self, layout: &Self::TextLayout, pos: impl Into) { // TODO: bounding box for text self.ctx.save(); - self.ctx.set_font(&layout.font.get_font_string()); + self.ctx.set_font(&layout.font_setting); let brush = layout.color().make_brush(self, || layout.size().to_rect()); self.set_brush(&brush, true); let pos = pos.into(); diff --git a/piet-web/src/text.rs b/piet-web/src/text.rs index 79836e527..76cbf13da 100644 --- a/piet-web/src/text.rs +++ b/piet-web/src/text.rs @@ -3,7 +3,6 @@ mod grapheme; mod lines; -use std::borrow::Cow; use std::fmt; use std::ops::RangeBounds; use std::rc::Rc; @@ -37,6 +36,7 @@ pub struct WebTextLayout { // Calculated on build pub(crate) line_metrics: Vec, + pub(crate) font_setting: Rc, size: Size, trailing_ws_width: f64, color: Color, @@ -118,22 +118,6 @@ impl WebFont { self.size = size; self } - - pub(crate) fn get_font_string(&self) -> String { - let style_str = match self.style { - FontStyle::Normal => Cow::from("normal"), - FontStyle::Italic => Cow::from("italic"), - FontStyle::Oblique(None) => Cow::from("italic"), - FontStyle::Oblique(Some(angle)) => Cow::from(format!("oblique {}deg", angle)), - }; - format!( - "{} {} {}px \"{}\"", - style_str, - self.weight, - self.size, - self.family.name() - ) - } } impl TextLayoutBuilder for WebTextLayoutBuilder { @@ -169,10 +153,28 @@ impl TextLayoutBuilder for WebTextLayoutBuilder { .with_weight(self.defaults.weight) .with_style(self.defaults.style); + let build_font_setting = |style: &dyn fmt::Display| { + format!( + r#"{} {} {}px "{}""#, + style, + font.weight, + font.size, + font.family.name() + ) + }; + let font_setting = match font.style { + FontStyle::Normal => build_font_setting(&"normal"), + FontStyle::Italic => build_font_setting(&"italic"), + FontStyle::Oblique(None) => build_font_setting(&"oblique"), + FontStyle::Oblique(Some(angle)) => { + build_font_setting(&format_args!("oblique {}deg", angle)) + } + }; let mut layout = WebTextLayout { ctx: self.ctx, font, text: self.text, + font_setting: font_setting.into(), line_metrics: Vec::new(), size: Size::ZERO, trailing_ws_width: 0.0, @@ -223,19 +225,12 @@ impl TextLayout for WebTextLayout { } fn hit_test_point(&self, point: Point) -> HitTestPoint { - self.ctx.set_font(&self.font.get_font_string()); - // internal logic is using grapheme clusters, but return the text position associated - // with the border of the grapheme cluster. - // null case if self.text.is_empty() { return HitTestPoint::default(); } - // this assumes that all heights/baselines are the same. - // Uses line bounding box to do hit testpoint, but with coordinates starting at 0.0 at - // first baseline - let first_baseline = self.line_metrics.get(0).map(|l| l.baseline).unwrap_or(0.0); + self.ctx.set_font(&self.font_setting); // check out of bounds above top // out of bounds on bottom during iteration @@ -276,7 +271,7 @@ impl TextLayout for WebTextLayout { } fn hit_test_text_position(&self, idx: usize) -> HitTestPosition { - self.ctx.set_font(&self.font.get_font_string()); + self.ctx.set_font(&self.font_setting); let idx = idx.min(self.text.len()); assert!(self.text.is_char_boundary(idx)); // first need to find line it's on, and get line start offset @@ -311,8 +306,8 @@ impl WebTextLayout { fn update_width(&mut self, new_width: impl Into>) { // various functions like `text_width` are stateful, and require - // the context to be configured correcttly. - self.ctx.set_font(&self.font.get_font_string()); + // the context to be configured correctly. + self.ctx.set_font(&self.font_setting); let new_width = new_width.into().unwrap_or(std::f64::INFINITY); let mut line_metrics = lines::calculate_line_metrics(&self.text, &self.ctx, new_width, self.font.size); From 21fe911f79b90da8f810a0afdd4108aca56335cf Mon Sep 17 00:00:00 2001 From: avitex Date: Tue, 8 Feb 2022 22:33:52 +1100 Subject: [PATCH 3/4] Enable failing test for wasm hit test point --- piet-common/tests/text.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/piet-common/tests/text.rs b/piet-common/tests/text.rs index a7aec7aa7..4e09ebdb8 100644 --- a/piet-common/tests/text.rs +++ b/piet-common/tests/text.rs @@ -216,8 +216,7 @@ fn hit_test_point_rounding() { } #[test] -//FIXME: wasm is failing this, and i haven't investigated -//#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] +#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn hit_test_point_outside() { let mut factory = make_factory(); let unit_width = factory.get_mono_width(12.0); From ede7a30bf22487f8c8ae357401a480adabc5fb6b Mon Sep 17 00:00:00 2001 From: avitex Date: Tue, 8 Feb 2022 22:34:24 +1100 Subject: [PATCH 4/4] Fix point Y hit test for top line It looks like this was never updated for a change in what is considered an anchor point from the baseline to the top of the layout box. This change checks that the hit point Y is past the new anchor point. See: https://github.com/linebender/piet/issues/311#issuecomment-697849630 --- piet-web/src/text.rs | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/piet-web/src/text.rs b/piet-web/src/text.rs index 76cbf13da..8a2a96d54 100644 --- a/piet-web/src/text.rs +++ b/piet-web/src/text.rs @@ -232,36 +232,40 @@ impl TextLayout for WebTextLayout { self.ctx.set_font(&self.font_setting); - // check out of bounds above top - // out of bounds on bottom during iteration - let mut is_y_inside = true; - if point.y < -1.0 * first_baseline { - is_y_inside = false - }; + // Get the top line metric. + let first_y_offset = self + .line_metrics + .get(0) + .map(|lm| lm.y_offset) + .unwrap_or(0.0); + + // Check point Y is within the top bound of the top line. + let mut is_y_inside = point.y >= first_y_offset; - let mut lm = self + // Get the first bottom line metric that overshoots point Y. + let bottom_lm = self .line_metrics .iter() - .skip_while(|l| l.y_offset + l.height < point.y); - let lm = lm - .next() + // Find line that overshoots point Y. + .find(|lm| lm.y_offset + lm.height >= point.y) .or_else(|| { - // This means it went over the last line, so return the last line. + // In this case we went over the last line, so return it. is_y_inside = false; self.line_metrics.last() }) .cloned() .unwrap_or_else(|| { + // In this case, we have no line metrics, so return a default. is_y_inside = false; Default::default() }); - // Then for the line, do hit test point - // Trailing whitespace is remove for the line - let line = &self.text[lm.start_offset..lm.end_offset]; + // For the bottom line, hit test the line point. + // Trailing whitespace is removed for the line. + let line = &self.text[bottom_lm.start_offset..bottom_lm.end_offset]; let mut htp = hit_test_line_point(&self.ctx, line, point); - htp.idx += lm.start_offset; + htp.idx += bottom_lm.start_offset; if !is_y_inside { htp.is_inside = false;