From d1bcffec88c8191ff37cc70f50ce537ba4b74cea Mon Sep 17 00:00:00 2001 From: ldm0 Date: Wed, 9 Sep 2026 08:50:13 +0800 Subject: [PATCH] fix(images): read rendered dimensions from layout snapshots --- .../element/images/dimensions.rs | 88 +++++++++++++------ .../runtime/page_vm/tests/rendering_update.rs | 1 + .../rendering_update/image_dimensions.rs | 65 ++++++++++++++ .../src/script_vm/tests/browser_api/images.rs | 48 ++++++++++ .../fixtures/image-attribute-dimensions.js | 13 +++ .../fixtures/image-layout-dimensions-setup.js | 19 ++++ .../tests/fixtures/image-layout-dimensions.js | 3 + 7 files changed, 212 insertions(+), 25 deletions(-) create mode 100644 moli-renderer-v8/src/runtime/page_vm/tests/rendering_update/image_dimensions.rs create mode 100644 moli-renderer-v8/tests/fixtures/image-attribute-dimensions.js create mode 100644 moli-renderer-v8/tests/fixtures/image-layout-dimensions-setup.js create mode 100644 moli-renderer-v8/tests/fixtures/image-layout-dimensions.js diff --git a/moli-renderer-v8/src/native_bridge/element/images/dimensions.rs b/moli-renderer-v8/src/native_bridge/element/images/dimensions.rs index 7dc954a4d9..f5cab40f13 100644 --- a/moli-renderer-v8/src/native_bridge/element/images/dimensions.rs +++ b/moli-renderer-v8/src/native_bridge/element/images/dimensions.rs @@ -2,14 +2,18 @@ use crate::document_runtime::DomHandle; use crate::webidl; use super::super::super::{JsContextHost, node::node_runtime_and_handle_from_object_or_detached}; -use super::super::{element_attribute, parse_non_negative_dimension, set_reflected_attribute}; +use super::super::{ + element_attribute, geometry::observable_element_metrics, set_reflected_attribute, +}; pub(in crate::native_bridge) fn image_width_getter_function<'s>( scope: &mut v8::PinScope<'s, '_>, args: v8::FunctionCallbackArguments<'s>, mut rv: v8::ReturnValue<'s, v8::Value>, ) { - rv.set_uint32(image_width_value(scope, args.this())); + if let Some(value) = image_dimension_value(scope, args.this(), true) { + rv.set_uint32(value); + } } pub(in crate::native_bridge) fn image_width_setter_function<'s>( @@ -26,7 +30,9 @@ pub(in crate::native_bridge) fn image_height_getter_function<'s>( args: v8::FunctionCallbackArguments<'s>, mut rv: v8::ReturnValue<'s, v8::Value>, ) { - rv.set_uint32(image_height_value(scope, args.this())); + if let Some(value) = image_dimension_value(scope, args.this(), false) { + rv.set_uint32(value); + } } pub(in crate::native_bridge) fn image_height_setter_function<'s>( @@ -44,36 +50,68 @@ pub(in crate::native_bridge) fn image_height_setter_function<'s>( rv.set_undefined(); } -fn image_width_value<'s>( +fn image_dimension_value<'s>( scope: &mut v8::PinScope<'s, '_>, object: v8::Local<'s, v8::Object>, -) -> u32 { + horizontal: bool, +) -> Option { let Ok((runtime_ptr, handle)) = node_runtime_and_handle_from_object_or_detached(scope, object) else { - return 0; + return Some(0); }; let runtime = unsafe { &*runtime_ptr }; - element_attribute(runtime, handle, "width") - .map(|value| parse_non_negative_dimension(Some(value))) - .filter(|value| *value > 0) - .or_else(|| image_intrinsic_dimensions(runtime, handle).map(|(width, _)| width)) - .unwrap_or(0) + let attribute = if horizontal { "width" } else { "height" }; + if runtime.layout_policy().uses_real_layout() { + // Like Blink's LayoutBoxWidth/Height, use the content box before CSS + // transforms and with absolute zoom removed. SynchronousGeometry keeps + // Moli's existing frozen-tree contract: this is not a forced refresh. + match observable_element_metrics( + runtime, + handle, + moli_layout::LayoutFlushReason::SynchronousGeometry, + ) { + Ok(Some(metrics)) => { + let size = if horizontal { + metrics.content_size.width + } else { + metrics.content_size.height + }; + return Some(size.round() as u32); + } + Ok(None) => {} + Err(error) => { + let message = format!("Layout failed while reading image {attribute}: {error}"); + if let Some(message) = crate::util::v8_string(scope, &message) { + let exception = v8::Exception::error(scope, message); + scope.throw_exception(exception); + } + return None; + } + } + } + // No rendered box (or Mock): a valid zero attribute is different from an + // absent/invalid one and must not fall back to the decoded natural size. + Some( + element_attribute(runtime, handle, attribute) + .as_deref() + .and_then(parse_dimension_attribute) + .or_else(|| { + image_intrinsic_dimensions(runtime, handle) + .map(|(width, height)| if horizontal { width } else { height }) + }) + .unwrap_or(0), + ) } -fn image_height_value<'s>( - scope: &mut v8::PinScope<'s, '_>, - object: v8::Local<'s, v8::Object>, -) -> u32 { - let Ok((runtime_ptr, handle)) = node_runtime_and_handle_from_object_or_detached(scope, object) - else { - return 0; - }; - let runtime = unsafe { &*runtime_ptr }; - element_attribute(runtime, handle, "height") - .map(|value| parse_non_negative_dimension(Some(value))) - .filter(|value| *value > 0) - .or_else(|| image_intrinsic_dimensions(runtime, handle).map(|(_, height)| height)) - .unwrap_or(0) +fn parse_dimension_attribute(value: &str) -> Option { + // Blink's ParseHTMLNonNegativeInteger accepts a digit prefix and -0, but + // rejects overflow, negative nonzero values and non-HTML whitespace. + let value = value.trim_start_matches([' ', '\t', '\r', '\n', '\u{000c}']); + let negative = value.starts_with('-'); + let digits = value.strip_prefix(['+', '-']).unwrap_or(value); + let end = digits.bytes().take_while(u8::is_ascii_digit).count(); + let parsed = digits[..end].parse::().ok()?; + (!negative || parsed == 0).then_some(parsed) } fn set_image_unsigned_long_attribute_on_object<'s>( diff --git a/moli-renderer-v8/src/runtime/page_vm/tests/rendering_update.rs b/moli-renderer-v8/src/runtime/page_vm/tests/rendering_update.rs index ecf340a220..db45d78638 100644 --- a/moli-renderer-v8/src/runtime/page_vm/tests/rendering_update.rs +++ b/moli-renderer-v8/src/runtime/page_vm/tests/rendering_update.rs @@ -1,5 +1,6 @@ use super::*; +mod image_dimensions; mod transform_precision; use base64::Engine as _; diff --git a/moli-renderer-v8/src/runtime/page_vm/tests/rendering_update/image_dimensions.rs b/moli-renderer-v8/src/runtime/page_vm/tests/rendering_update/image_dimensions.rs new file mode 100644 index 0000000000..3cdf57cb13 --- /dev/null +++ b/moli-renderer-v8/src/runtime/page_vm/tests/rendering_update/image_dimensions.rs @@ -0,0 +1,65 @@ +use super::*; + +#[tokio::test(flavor = "current_thread")] +async fn image_dimensions_use_rendered_content_size_without_forcing_a_refresh() { + run_page_vm_async_test(async move { + let loader = + crate::network::ResourceRequestClient::new(&FetchConfig::default()).expect("loader"); + let mut page_vm = test_page_vm_with_loader_and_document_url( + &loader, + Vec::new(), + Url::parse("https://example.com/image-dimensions.html")?, + ); + page_vm.vm_mut().eval(include_str!( + "../../../../../tests/fixtures/image-layout-dimensions-setup.js" + ))?; + page_vm.vm_mut().sync_live_document_style_sources(); + let viewport = moli_layout::LayoutViewport::new(200, 600, 1.0); + page_vm + .vm_mut() + .screenshot_layout_snapshot(viewport)? + .expect("image dimensions layout"); + let source = include_str!("../../../../../tests/fixtures/image-layout-dimensions.js"); + let result = page_vm + .vm_mut() + .eval(&format!("JSON.stringify({source})"))?; + assert_eq!( + serde_json::from_str::(&result)?, + serde_json::json!({ + "css": [40,30,0,0], "override": [40,30,0,0], "edges": [40,30,0,0], + "borderbox": [30,20,0,0], "transformed": [40,30,0,0], + "zoomed": [40,30,0,0], "vertical": [40,30,0,0], + "fractional": [41,31,0,0], "hidden": [33,22,0,0] + }), + "shared fixture must match Chromium's content-box image dimensions" + ); + let passes = page_vm.vm().layout_pass_observability_for_test().1; + assert_eq!( + page_vm.vm_mut().eval( + r#" +const image = document.getElementById('css'); +image.style.width = '70px'; image.style.height = '50px'; +image.width = 120; image.height = 90; +[image.width,image.height].join('|') +"# + )?, + "40|30", + "synchronous getters must retain the published snapshot after mutation" + ); + assert_eq!(page_vm.vm().layout_pass_observability_for_test().1, passes); + page_vm + .vm_mut() + .screenshot_layout_snapshot(viewport)? + .expect("refreshed image dimensions layout"); + assert_eq!( + page_vm + .vm_mut() + .eval("[image.width,image.height].join('|')")?, + "70|50", + "a rendering checkpoint publishes the new CSS size, not the HTML attributes" + ); + Ok::<_, anyhow::Error>(()) + }) + .await + .expect("image dimension geometry fixture should run"); +} diff --git a/moli-renderer-v8/src/script_vm/tests/browser_api/images.rs b/moli-renderer-v8/src/script_vm/tests/browser_api/images.rs index af04174cbb..e458c38557 100644 --- a/moli-renderer-v8/src/script_vm/tests/browser_api/images.rs +++ b/moli-renderer-v8/src/script_vm/tests/browser_api/images.rs @@ -6,6 +6,54 @@ use tokio::{ const ONE_BY_ONE_GIF: &[u8] = b"GIF89a\x01\0\x01\0\x80\0\0\0\0\0\xff\xff\xff!\xf9\x04\x01\0\0\0\0,\0\0\0\0\x01\0\x01\0\0\x02\x02D\x01\0;"; +#[tokio::test] +async fn image_dimensions_without_layout_preserve_zero_and_parse_html_integer_prefixes() { + let loader = ResourceRequestClient::new(&moli_fetch::FetchConfig::default()).expect("loader"); + loader.set_image_fetch_enabled(true); + let mut vm = new_storage_page_task_executor_test_vm_with_loader( + "https://image-dimensions.test/page.html", + &loader, + ); + vm.set_fetch_subresource_interception(true, Some(crate::types::SubresourceResourceType::Image)); + vm.eval("globalThis.dimensionImage = new Image(); dimensionImage.src = 'image.gif'") + .expect("detached image request"); + let pending = vm.take_pending_subresource_fetch_infos(); + assert_eq!(pending.len(), 1); + vm.fulfill_pending_subresource_fetch( + pending[0].internal_id, + 200, + vec![("Content-Type".into(), "image/gif".into())], + one_by_one_gif_response_body(), + ) + .expect("decoded image response"); + run_next_image_event_task(&mut vm, &loader, "image dimension decode").await; + let source = include_str!("../../../../tests/fixtures/image-attribute-dimensions.js"); + let result = vm + .eval(&format!("JSON.stringify({source})")) + .expect("attribute fixture"); + assert_eq!( + serde_json::from_str::(&result).expect("attribute JSON"), + serde_json::json!([ + [1, 1], + [0, 0], + [0, 0], + [0, 0], + [0, 0], + [1, 1], + [7, 7], + [8, 8], + [1, 1], + [1, 1], + [1, 1], + [2, 2], + [4294967295_u32, 4294967295_u32], + [1, 1], + [1, 1] + ]), + "unrendered image dimensions must match the shared Chromium fixture" + ); +} + fn one_by_one_gif_response_body() -> crate::runtime::RendererSyntheticResponseBody { crate::runtime::RendererSyntheticResponseBody::from_bytes(ONE_BY_ONE_GIF.to_vec()) } diff --git a/moli-renderer-v8/tests/fixtures/image-attribute-dimensions.js b/moli-renderer-v8/tests/fixtures/image-attribute-dimensions.js new file mode 100644 index 0000000000..733d53cdb8 --- /dev/null +++ b/moli-renderer-v8/tests/fixtures/image-attribute-dimensions.js @@ -0,0 +1,13 @@ +(() => { + const image = globalThis.dimensionImage; + if (image.naturalWidth !== 1 || image.naturalHeight !== 1) throw new Error('decoded 1x1 image required'); + const values = [null, '0', '0junk', '-0', '-00px', '-1', '+7px', ' \t\n8tail', + '\v8', '\u00a08', '1.9', '2e2', '4294967295', '4294967296', '999999999999']; + return values.map(value => { + for (const name of ['width','height']) { + if (value === null) image.removeAttribute(name); + else image.setAttribute(name,value); + } + return [image.width,image.height]; + }); +})() diff --git a/moli-renderer-v8/tests/fixtures/image-layout-dimensions-setup.js b/moli-renderer-v8/tests/fixtures/image-layout-dimensions-setup.js new file mode 100644 index 0000000000..e68c8a64fc --- /dev/null +++ b/moli-renderer-v8/tests/fixtures/image-layout-dimensions-setup.js @@ -0,0 +1,19 @@ +(() => { + document.head.innerHTML = ``; + document.body.innerHTML = ` + + + +`; + return 'installed'; +})() diff --git a/moli-renderer-v8/tests/fixtures/image-layout-dimensions.js b/moli-renderer-v8/tests/fixtures/image-layout-dimensions.js new file mode 100644 index 0000000000..adc108846a --- /dev/null +++ b/moli-renderer-v8/tests/fixtures/image-layout-dimensions.js @@ -0,0 +1,3 @@ +(() => Object.fromEntries([...document.images].map(image => [ + image.id, [image.width, image.height, image.naturalWidth, image.naturalHeight] +])))()