Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

min/max-width/height, replaced elements #25207

Merged
merged 19 commits into from Dec 10, 2019
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a2c2b29
Move `clamp_*` functions to methods of `Length`
SimonSapin Dec 7, 2019
ce7e84b
Replace `percent_resolved_*` functions with methods
SimonSapin Dec 7, 2019
999dd72
Account for min/max-width in outer intrinsic sizing
SimonSapin Dec 7, 2019
bf96988
Add min/max-width/height support for `inline-block`
SimonSapin Dec 7, 2019
c40583b
Move replaced box used size computation to a method of `ReplacedConte…
SimonSapin Dec 7, 2019
b73eb49
Add sizing of inline replaced boxes
SimonSapin Dec 7, 2019
8996be3
Don’t assume replaced elements have an intrinsic size
SimonSapin Dec 7, 2019
80b2b5f
Fix min/max-content of replaced boxes
SimonSapin Dec 7, 2019
f43dc3a
Remove inline/block_size from AbsolutelyPositionedFragment
SimonSapin Dec 7, 2019
e86222d
Remove AbsoluteBoxOffsets’s type parameter
SimonSapin Dec 7, 2019
14ddf39
Rename ReplacedContent::used_size to used_size_as_if_inline_element
SimonSapin Dec 7, 2019
f09c14a
impl From<&'_ DefiniteContainingBlock> for ContainingBlock
SimonSapin Dec 7, 2019
1fcdde9
Move two AbsoluteBoxOffsets fields into a Vec2
SimonSapin Dec 7, 2019
1fa20e9
Implement replaced abspos
SimonSapin Dec 7, 2019
c07c980
Delayed initialization over mutation
SimonSapin Dec 8, 2019
2906722
Conditionsals over closures
SimonSapin Dec 8, 2019
be8df1d
Move `solve_axis` function to module level
SimonSapin Dec 8, 2019
a17db21
Struct with named fields over large tuple
SimonSapin Dec 8, 2019
53ce714
Fix a “Accessing content size that was not requested” panic
SimonSapin Dec 8, 2019
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Don’t assume replaced elements have an intrinsic size

  • Loading branch information
SimonSapin committed Dec 10, 2019
commit 8996be3c5efa4ef93fd0af332a05bbcbb4905810
@@ -17,7 +17,6 @@ use std::sync::Arc;
use style::dom::TNode;
use style::properties::ComputedValues;
use style::selector_parser::PseudoElement;
use style::values::computed::Length;

#[derive(Clone, Copy)]
pub enum WhichPseudoElement {
@@ -299,7 +298,10 @@ impl Drop for BoxSlot<'_> {
pub(crate) trait NodeExt<'dom>: 'dom + Copy + LayoutNode + Send + Sync {
fn is_element(self) -> bool;
fn as_text(self) -> Option<String>;
fn as_image(self) -> Option<(Option<Arc<NetImage>>, Vec2<Length>)>;

/// Returns the image if it’s loaded, and its size in image pixels
/// adjusted for `image_density`.
fn as_image(self) -> Option<(Option<Arc<NetImage>>, Vec2<f64>)>;

This comment has been minimized.

Copy link
@nox

nox Dec 9, 2019

Member

Why the type change, and why not CSSFloat?

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Dec 9, 2019

Author Member

It’s not Length because it is measured in image pixels rather than CSS px.

I wanted to make it u32 but it’s already divided by script::dom::htmlimageelement::ImageRequest::current_pixel_density. I’m not sure what that density means exactly or if diving there is correct.

It could be CSSFloat. It probably wouldn’t affect much since it’s not kept in memory for a long time.

fn first_child(self) -> Option<Self>;
fn next_sibling(self) -> Option<Self>;
fn parent_node(self) -> Option<Self>;
@@ -328,22 +330,22 @@ where
}
}

fn as_image(self) -> Option<(Option<Arc<NetImage>>, Vec2<Length>)> {
fn as_image(self) -> Option<(Option<Arc<NetImage>>, Vec2<f64>)> {
let node = self.to_threadsafe();
let (resource, metadata) = node.image_data()?;
let (width, height) = resource
.as_ref()
.map(|image| (image.width, image.height))
.or_else(|| metadata.map(|metadata| (metadata.width, metadata.height)))
.unwrap_or((0, 0));
let (mut width, mut height) = (width as f32, height as f32);
let (mut width, mut height) = (width as f64, height as f64);
if let Some(density) = node.image_density().filter(|density| *density != 1.) {
width = (width as f64 / density) as f32;
height = (height as f64 / density) as f32;
width = width / density;
height = height / density;
}
let size = Vec2 {
x: Length::new(width),
y: Length::new(height),
x: width,
y: height,
};
Some((resource, size))
}
@@ -4,20 +4,36 @@

use crate::dom_traversal::NodeExt;
use crate::fragments::{Fragment, ImageFragment};
use crate::geom::{flow_relative, physical};
use crate::geom::flow_relative::{Rect, Vec2};
use crate::geom::physical;
use crate::style_ext::ComputedValuesExt;
use crate::ContainingBlock;
use net_traits::image::base::Image;
use servo_arc::Arc as ServoArc;
use std::sync::Arc;
use style::properties::ComputedValues;
use style::values::computed::{Length, LengthOrAuto};
use style::values::CSSFloat;
use style::Zero;

#[derive(Debug)]
pub(crate) struct ReplacedContent {
pub kind: ReplacedContentKind,
pub intrinsic_size: physical::Vec2<Length>,

/// * Raster images always have an instrinsic width and height, with 1 image pixel = 1px.
/// The intrinsic ratio should be based on dividing those.
/// See https://github.com/w3c/csswg-drafts/issues/4572 for the case where either is zero.
/// PNG specifically disallows this but I (SimonSapin) am not sure about other formats.
///
/// * Form controls have both intrinsic width and height **but no intrinsic ratio**.
/// See https://github.com/w3c/csswg-drafts/issues/1044 and
/// https://drafts.csswg.org/css-images/#intrinsic-dimensions “In general, […]”
///
/// * For SVG, see https://svgwg.org/svg2-draft/coords.html#SizingSVGInCSS
/// and again https://github.com/w3c/csswg-drafts/issues/4572.
intrinsic_width: Option<Length>,
intrinsic_height: Option<Length>,
intrinsic_ratio: Option<CSSFloat>,
}

#[derive(Debug)]
@@ -27,10 +43,21 @@ pub(crate) enum ReplacedContentKind {

impl ReplacedContent {
pub fn for_element<'dom>(element: impl NodeExt<'dom>) -> Option<Self> {
if let Some((image, intrinsic_size)) = element.as_image() {
if let Some((image, intrinsic_size_in_dots)) = element.as_image() {
// FIXME: should 'image-resolution' (when implemented) be used *instead* of
// `script::dom::htmlimageelement::ImageRequest::current_pixel_density`?

// https://drafts.csswg.org/css-images-4/#the-image-resolution
let dppx = 1.0;

let width = (intrinsic_size_in_dots.x as CSSFloat) / dppx;
let height = (intrinsic_size_in_dots.y as CSSFloat) / dppx;
return Some(Self {
kind: ReplacedContentKind::Image(image),
intrinsic_size,
intrinsic_width: Some(Length::new(width)),
intrinsic_height: Some(Length::new(height)),
// FIXME https://github.com/w3c/csswg-drafts/issues/4572
intrinsic_ratio: Some(width / height),
});
}
None
@@ -39,7 +66,7 @@ impl ReplacedContent {
pub fn make_fragments<'a>(
&'a self,
style: &ServoArc<ComputedValues>,
size: flow_relative::Vec2<Length>,
size: Vec2<Length>,
) -> Vec<Fragment> {
match &self.kind {
ReplacedContentKind::Image(image) => image
@@ -48,8 +75,8 @@ impl ReplacedContent {
.map(|image_key| {
Fragment::Image(ImageFragment {
style: style.clone(),
rect: flow_relative::Rect {
start_corner: flow_relative::Vec2::zero(),
rect: Rect {
start_corner: Vec2::zero(),
size,
},
image_key,
@@ -66,12 +93,21 @@ impl ReplacedContent {
&self,
containing_block: &ContainingBlock,
style: &ComputedValues,
) -> flow_relative::Vec2<Length> {
) -> Vec2<Length> {
let mode = style.writing_mode;
// FIXME(nox): We shouldn't pretend we always have a fully known intrinsic size.
let intrinsic_size = self.intrinsic_size.size_to_flow_relative(mode);
// FIXME(nox): This can divide by zero.
let intrinsic_ratio = intrinsic_size.inline.px() / intrinsic_size.block.px();
let intrinsic_size = physical::Vec2 {
x: self.intrinsic_width,
y: self.intrinsic_height,
};

This comment has been minimized.

Copy link
@nox

nox Dec 9, 2019

Member

Why not store the intrinsic size as a physical::Vec2<Option<_>>?

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Dec 9, 2019

Author Member

Could be either. An earlier version of the comment on the field declaration talked about "all three components"

let intrinsic_size = intrinsic_size.size_to_flow_relative(mode);
let intrinsic_ratio = self.intrinsic_ratio.map(|width_over_height| {
// inline-size over block-size
if style.writing_mode.is_vertical() {
1. / width_over_height
} else {
width_over_height
}
});

let box_size = style.box_size().percentages_relative_to(containing_block);
let min_box_size = style
@@ -82,25 +118,94 @@ impl ReplacedContent {
.max_box_size()
.percentages_relative_to(containing_block);

let clamp = |inline_size: Length, block_size: Length| {
(
inline_size.clamp_between_extremums(min_box_size.inline, max_box_size.inline),
block_size.clamp_between_extremums(min_box_size.block, max_box_size.block),
)
let default_object_size = || {
// FIXME:
// “If 300px is too wide to fit the device, UAs should use the width of
// the largest rectangle that has a 2:1 ratio and fits the device instead.”
// “height of the largest rectangle that has a 2:1 ratio, has a height not greater
// than 150px, and has a width not greater than the device width.”
physical::Vec2 {
x: Length::new(300.),
y: Length::new(150.),
}
.size_to_flow_relative(mode)
};
let clamp = |inline_size: Length, block_size: Length| Vec2 {
inline: inline_size.clamp_between_extremums(min_box_size.inline, max_box_size.inline),
block: block_size.clamp_between_extremums(min_box_size.block, max_box_size.block),
Comment on lines +133 to +135

This comment has been minimized.

Copy link
@nox

nox Dec 9, 2019

Member

Wondering if we should have a meethod for that, don't we do that for non-replaced things elsewhere in the code?

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Dec 9, 2019

Author Member

I grepped through the code and didn’t find another instance of computing both at the same time.

In block layout for example, we compute the used width before recurring and the used height after.

};
// https://drafts.csswg.org/css2/visudet.html#min-max-widths
// https://drafts.csswg.org/css2/visudet.html#min-max-heights
let (inline_size, block_size) = match (box_size.inline, box_size.block) {
match (box_size.inline, box_size.block) {

This comment has been minimized.

Copy link
@nox

nox Dec 9, 2019

Member

Returning Vec2<_> values made the code more verbose, IMO, but it's not very important.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Dec 9, 2019

Author Member

For some reason I thought this change helped having an early return, but it looks like it wasn’t necessary after all.

(LengthOrAuto::LengthPercentage(inline), LengthOrAuto::LengthPercentage(block)) => {
clamp(inline, block)
},
(LengthOrAuto::LengthPercentage(inline), LengthOrAuto::Auto) => {
clamp(inline, inline / intrinsic_ratio)
let block = if let Some(i_over_b) = intrinsic_ratio {

This comment has been minimized.

Copy link
@nox

nox Dec 9, 2019

Member

This name confuses me, please just name it ratio.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Dec 9, 2019

Author Member

The problem these new names try to address is that when A divided by B is called a ratio, B divided by A is also called a ratio although they’re not interchangeable. Code that uses that ratio sometimes to multiple and sometimes to divide a length, having a variable name that indicates the “direction” of the ratio helps check if those are correct by doing pseudo-dimensional analysis.

We could declare that an aspect ratio is by convention always a width divided by a height. But this code work in flow-relative dimensions and accesses a ratio field in physical dimensions. I’ve added inline_size_over_block_size_intrinsic_ratio to make that conversion.

I’m open to other names, but I’d prefer if they indicate the “direction” of the ratio somehow. Or maybe we could provide that indication in doc-comments and code comments?

This comment has been minimized.

Copy link
@nox

nox Dec 9, 2019

Member

Fair enough, I don't mind the name.

inline / i_over_b
} else if let Some(block) = intrinsic_size.block {
block
} else {
default_object_size().block
};
clamp(inline, block)
},
(LengthOrAuto::Auto, LengthOrAuto::LengthPercentage(block)) => {
clamp(block * intrinsic_ratio, block)
let inline = if let Some(i_over_b) = intrinsic_ratio {
block * i_over_b
} else if let Some(inline) = intrinsic_size.inline {
inline
} else {
default_object_size().inline
};
clamp(inline, block)
},
(LengthOrAuto::Auto, LengthOrAuto::Auto) => {
let inline_size =
match (intrinsic_size.inline, intrinsic_size.block, intrinsic_ratio) {
(Some(inline), _, _) => inline,
(None, Some(block), Some(i_over_b)) => {
// “used height” in CSS 2 is always gonna be the intrinsic one,
// since it is available.
block * i_over_b
},
// FIXME
//
// “If 'height' and 'width' both have computed values of 'auto'
// and the element has an intrinsic ratio but no intrinsic height or width,
// […]”
//
// In this `match` expression this would be an additional arm here:
//
// ```
// (Vec2 { inline: None, block: None }, Some(_)) => {…}
// ```
//
// “[…] then the used value of 'width' is undefined in CSS 2.
// However, it is suggested that, if the containing block's width
// does not itself depend on the replaced element's width,
// then the used value of 'width' is calculated from the constraint
// equation used for block-level, non-replaced elements in normal flow.”
_ => default_object_size().inline,
};
let block_size = if let Some(block) = intrinsic_size.block {
block
} else if let Some(i_over_b) = intrinsic_ratio {
// “used width” in CSS 2 is what we just computed above
inline_size / i_over_b
} else {
default_object_size().block
};

let i_over_b = if let Some(i_over_b) = intrinsic_ratio {
i_over_b
} else {
return clamp(inline_size, block_size);
};

// https://drafts.csswg.org/css2/visudet.html#min-max-widths
// “However, for replaced elements with an intrinsic ratio and both
// 'width' and 'height' specified as 'auto', the algorithm is as follows”
enum Violation {
None,
Below(Length),
@@ -119,87 +224,84 @@ impl ReplacedContent {
}
};
match (
violation(
intrinsic_size.inline,
min_box_size.inline,
max_box_size.inline,
),
violation(intrinsic_size.block, min_box_size.block, max_box_size.block),
violation(inline_size, min_box_size.inline, max_box_size.inline),
violation(block_size, min_box_size.block, max_box_size.block),
) {
// Row 1.
(Violation::None, Violation::None) => {
(intrinsic_size.inline, intrinsic_size.block)
(Violation::None, Violation::None) => Vec2 {
inline: inline_size,
block: block_size,
},
// Row 2.
(Violation::Above(max_inline_size), Violation::None) => {
let block_size =
(max_inline_size / intrinsic_ratio).max(min_box_size.block);
(max_inline_size, block_size)
(Violation::Above(max_inline_size), Violation::None) => Vec2 {
inline: max_inline_size,
block: (max_inline_size / i_over_b).max(min_box_size.block),
},
// Row 3.
(Violation::Below(min_inline_size), Violation::None) => {
let block_size =
(min_inline_size / intrinsic_ratio).clamp_below_max(max_box_size.block);
(min_inline_size, block_size)
(Violation::Below(min_inline_size), Violation::None) => Vec2 {
inline: min_inline_size,
block: (min_inline_size / i_over_b).clamp_below_max(max_box_size.block),
},
// Row 4.
(Violation::None, Violation::Above(max_block_size)) => {
let inline_size =
(max_block_size * intrinsic_ratio).max(min_box_size.inline);
(inline_size, max_block_size)
(Violation::None, Violation::Above(max_block_size)) => Vec2 {
inline: (max_block_size * i_over_b).max(min_box_size.inline),
block: max_block_size,
},
// Row 5.
(Violation::None, Violation::Below(min_block_size)) => {
let inline_size =
(min_block_size * intrinsic_ratio).clamp_below_max(max_box_size.inline);
(inline_size, min_block_size)
(Violation::None, Violation::Below(min_block_size)) => Vec2 {
inline: (min_block_size * i_over_b).clamp_below_max(max_box_size.inline),
block: min_block_size,
},
// Rows 6-7.
(Violation::Above(max_inline_size), Violation::Above(max_block_size)) => {
if max_inline_size.px() / intrinsic_size.inline.px() <=
max_block_size.px() / intrinsic_size.block.px()
if max_inline_size.px() / inline_size.px() <=
max_block_size.px() / block_size.px()
{
// Row 6.
let block_size =
(max_inline_size / intrinsic_ratio).max(min_box_size.block);
(max_inline_size, block_size)
Vec2 {
inline: max_inline_size,
block: (max_inline_size / i_over_b).max(min_box_size.block),
}
} else {
// Row 7.
let inline_size =
(max_block_size * intrinsic_ratio).max(min_box_size.inline);
(inline_size, max_block_size)
Vec2 {
inline: (max_block_size * i_over_b).max(min_box_size.inline),
block: max_block_size,
}
}
},
// Rows 8-9.
(Violation::Below(min_inline_size), Violation::Below(min_block_size)) => {
if min_inline_size.px() / intrinsic_size.inline.px() <=
min_block_size.px() / intrinsic_size.block.px()
if min_inline_size.px() / inline_size.px() <=
min_block_size.px() / block_size.px()
{
// Row 8.
let inline_size = (min_block_size * intrinsic_ratio)
.clamp_below_max(max_box_size.inline);
(inline_size, min_block_size)
Vec2 {
inline: (min_block_size * i_over_b)
.clamp_below_max(max_box_size.inline),
block: min_block_size,
}
} else {
// Row 9.
let block_size = (min_inline_size / intrinsic_ratio)
.clamp_below_max(max_box_size.block);
(min_inline_size, block_size)
Vec2 {
inline: min_inline_size,
block: (min_inline_size / i_over_b)
.clamp_below_max(max_box_size.block),
}
}
},
// Row 10.
(Violation::Below(min_inline_size), Violation::Above(max_block_size)) => {
(min_inline_size, max_block_size)
(Violation::Below(min_inline_size), Violation::Above(max_block_size)) => Vec2 {
inline: min_inline_size,
block: max_block_size,
},
// Row 11.
(Violation::Above(max_inline_size), Violation::Below(min_block_size)) => {
(max_inline_size, min_block_size)
(Violation::Above(max_inline_size), Violation::Below(min_block_size)) => Vec2 {
inline: max_inline_size,
block: min_block_size,
},
}
},
};
flow_relative::Vec2 {
inline: inline_size,
block: block_size,
}
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.