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

Improvements to gradients. #16666

Merged
merged 7 commits into from May 7, 2017

Improve convert_gradient_stops function.

Implement Specification from CSS Images 3.

Improvements:

1. Stops with positions less than preceding stops are moved.
     A common pattern is blue `70%, white 0` for sharp transitions.
2. Stop runs are correct if first stop has a position.
    The list `black 0%, red, gold` now renders the same as `black, red, gold`.
    Other runs may also be corrected.
3. Absolute length are no longer capped to 100%.
  • Loading branch information
pyfisch committed Apr 30, 2017
commit a956e3fd529715cc0ac39b23910f19e092c7c5a9
@@ -608,57 +608,80 @@ fn convert_gradient_stops(gradient_items: &[GradientItem],
// Determine the position of each stop per CSS-IMAGES § 3.4.
//
// FIXME(#3908, pcwalton): Make sure later stops can't be behind earlier stops.

This comment has been minimized.

@emilio

emilio May 6, 2017

Member

This FIXME can go away now, right?

This comment has been minimized.

@pyfisch

pyfisch May 7, 2017

Author Contributor

Obviously, I thought I had removed the FIXME.

let stop_items = gradient_items.iter().filter_map(|item| {

// Only keep the color stops. Discard the color items.

This comment has been minimized.

@emilio

emilio May 6, 2017

Member

"Only keep the color stop, discard the color interpolation hints."

This comment has been minimized.

@pyfisch

pyfisch May 7, 2017

Author Contributor

Thanks.

let mut stop_items = gradient_items.iter().filter_map(|item| {
match *item {
GradientItem::ColorStop(ref stop) => Some(stop),
GradientItem::ColorStop(ref stop) => Some(*stop),
_ => None,
}
}).collect::<Vec<_>>();

assert!(stop_items.len() >= 2);

// Run the algorithm from
// https://drafts.csswg.org/css-images-3/#color-stop-syntax

// Step 1: If nothing else is specified the first stop is

This comment has been minimized.

@emilio

emilio May 6, 2017

Member

I'd prefer to quote the spec directly, it makes it easier to grep:

// Step 1: If the first color stop does not have a position, set its position to 0%.
{
    // ...
}
// If the last color stop does not have a position, set its position to 100%.
{
    // ...
}

This comment has been minimized.

@pyfisch

pyfisch May 7, 2017

Author Contributor

Done for step 1.

// at 0% and the last stop is at 100%.
{
let first = stop_items.first_mut().unwrap();
if first.position.is_none() {
first.position = Some(LengthOrPercentage::Percentage(0.0));
}
}
{
let last = stop_items.last_mut().unwrap();
if last.position.is_none() {
last.position = Some(LengthOrPercentage::Percentage(1.0));
}
}

// Step 2: Move any stops placed before earlier stops to the
// same position as the preceding stop.
let mut state = stop_items.first().unwrap().position.unwrap();

This comment has been minimized.

@emilio

emilio May 6, 2017

Member

instead of state, what about last_stop_position?

Also, we could move it to its own function to make this one a bit less dense, but as you prefer.

This comment has been minimized.

@pyfisch

pyfisch May 7, 2017

Author Contributor

Wouldn't previous_stop_position be a better name? last_stop_position can also be stop_items.last().

for stop in stop_items.iter_mut() {

This comment has been minimized.

@emilio

emilio May 6, 2017

Member

You can use stop_items.iter_mut().skip(1), since there's no stop before the first one.

This comment has been minimized.

@pyfisch

pyfisch May 7, 2017

Author Contributor

Good catch.

if let Some(pos) = stop.position {
if position_to_offset(state, length) > position_to_offset(pos, length) {

This comment has been minimized.

@emilio

emilio May 6, 2017

Member

Perhaps renaming length to available_length would make this function a bit clearer, but no need to do it, I guess, since it's pre-existing.

This comment has been minimized.

@pyfisch

pyfisch May 7, 2017

Author Contributor

Inside fn position_to_offset it is called total_length. I will go with this name.

stop.position = Some(state);
}
state = stop.position.unwrap();
}
}

// Step 3: Evenly space stops without position.
let mut stops = Vec::with_capacity(stop_items.len());
let mut stop_run = None;
for (i, stop) in stop_items.iter().enumerate() {
let offset = match stop.position {
None => {
if stop_run.is_none() {
// Initialize a new stop run.
let start_offset = if i == 0 {
0.0
} else {
// `unwrap()` here should never fail because this is the beginning of
// a stop run, which is always bounded by a length or percentage.
position_to_offset(stop_items[i - 1].position.unwrap(), length)
};
let (end_index, end_offset) =
match stop_items[i..]
.iter()
.enumerate()
.find(|&(_, ref stop)| stop.position.is_some()) {
None => (stop_items.len() - 1, 1.0),
Some((end_index, end_stop)) => {
// `unwrap()` here should never fail because this is the end of
// a stop run, which is always bounded by a length or
// percentage.
(end_index,
position_to_offset(end_stop.position.unwrap(), length))
}
};
// `unwrap()` here should never fail because this is the beginning of
// a stop run, which is always bounded by a length or percentage.
let start_offset =
position_to_offset(stop_items[i - 1].position.unwrap(), length);
// `unwrap()` here should never fail because this is the end of
// a stop run, which is always bounded by a length or percentage.
let (end_index, end_stop) = stop_items[(i + 1)..]
.iter()
.enumerate()
.find(|&(_, ref stop)| stop.position.is_some())
.unwrap();
let end_offset = position_to_offset(end_stop.position.unwrap(), length);
stop_run = Some(StopRun {
start_offset: start_offset,
end_offset: end_offset,
start_index: i,
stop_count: end_index - i,
start_index: i - 1,
stop_count: end_index,
})
}

let stop_run = stop_run.unwrap();
let stop_run_length = stop_run.end_offset - stop_run.start_offset;
if stop_run.stop_count == 0 {
stop_run.end_offset
} else {
stop_run.start_offset +
stop_run_length * (i - stop_run.start_index) as f32 /
(stop_run.stop_count as f32)
}
stop_run.start_offset +
stop_run_length * (i - stop_run.start_index) as f32 /
((2 + stop_run.stop_count) as f32)
}
Some(position) => {
stop_run = None;
@@ -2692,12 +2715,10 @@ struct StopRun {

fn position_to_offset(position: LengthOrPercentage, Au(total_length): Au) -> f32 {
match position {
LengthOrPercentage::Length(Au(length)) => {
(1.0f32).min(length as f32 / total_length as f32)
}
LengthOrPercentage::Length(Au(length)) => length as f32 / total_length as f32,
LengthOrPercentage::Percentage(percentage) => percentage as f32,
LengthOrPercentage::Calc(calc) =>
(1.0f32).min(calc.percentage() + (calc.length().0 as f32) / (total_length as f32)),
calc.percentage() + (calc.length().0 as f32) / (total_length as f32),
}
}

ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.