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

Handle 3- and 4- valued <position>s in style #13042

Merged
merged 2 commits into from Aug 29, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions components/style/properties/longhand/background.mako.rs
Expand Up @@ -107,8 +107,10 @@ ${helpers.predefined_type("background-color", "CSSColor",
pub fn get_initial_specified_value() -> SpecifiedValue {
use values::specified::Percentage;
Position {
horizontal: specified::LengthOrPercentage::Percentage(Percentage(0.0)),
vertical: specified::LengthOrPercentage::Percentage(Percentage(0.0)),
horiz_keyword: None,
horiz_position: Some(specified::LengthOrPercentage::Percentage(Percentage(0.0))),
vert_keyword: None,
vert_position: Some(specified::LengthOrPercentage::Percentage(Percentage(0.0))),
}
}

Expand Down
12 changes: 8 additions & 4 deletions components/style/values/specified/basic_shape.rs
Expand Up @@ -265,8 +265,10 @@ impl Circle {
} else {
// Defaults to origin
Position {
horizontal: LengthOrPercentage::Percentage(Percentage(0.5)),
vertical: LengthOrPercentage::Percentage(Percentage(0.5)),
horiz_keyword: None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Store it as center center

horiz_position: Some(LengthOrPercentage::Percentage(Percentage(0.5))),
vert_keyword: None,
vert_position: Some(LengthOrPercentage::Percentage(Percentage(0.5))),
}
};
Ok(Circle {
Expand Down Expand Up @@ -329,8 +331,10 @@ impl Ellipse {
} else {
// Defaults to origin
Position {
horizontal: LengthOrPercentage::Percentage(Percentage(0.5)),
vertical: LengthOrPercentage::Percentage(Percentage(0.5)),
horiz_keyword: None,
horiz_position: Some(LengthOrPercentage::Percentage(Percentage(0.5))),
vert_keyword: None,
vert_position: Some(LengthOrPercentage::Percentage(Percentage(0.5))),
}
};
Ok(Ellipse {
Expand Down
212 changes: 170 additions & 42 deletions components/style/values/specified/position.rs
Expand Up @@ -17,39 +17,104 @@ use values::specified::{LengthOrPercentage, Percentage};
#[derive(Debug, Clone, PartialEq, Copy)]
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
pub struct Position {
pub horizontal: LengthOrPercentage,
pub vertical: LengthOrPercentage,
pub horiz_keyword: Option<Keyword>,
pub horiz_position: Option<LengthOrPercentage>,
pub vert_keyword: Option<Keyword>,
pub vert_position: Option<LengthOrPercentage>,
}

impl ToCss for Position {
fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
try!(self.horizontal.to_css(dest));
// TODO: canaltinova: We should add keywords, probably?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this could also be just a keyword. Print the components in order, with spaces between them if they exist.

Copy link
Contributor Author

@canova canova Aug 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually if background position is center bottom 10px, Position::parse function sends center as value and it stores as 50% bottom 10px. Keywords are storing if second keyword is explicitly defined. Otherwise it's assuming LengthOrPercentage. Is it a bad behaviour? If so, we have to check PositionComponents at Position::parse whether if they are keyword or length.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is not the right behavior. This was done because basic-shape has different requirements for Position serialization, which I now know to not generalize to all positions.

We should store keywords as-is, and only reorder. The ToCss impl should be relatively simple, so that we can add any additional complexity to the basic-shape side. It's fine if you break basic-shape for now, I'll fix it later.

if let Some(horiz_pos) = self.horiz_position {
if let Some(horiz_key) = self.horiz_keyword {
try!(horiz_key.to_css(dest));
};
try!(horiz_pos.to_css(dest));
};

try!(dest.write_str(" "));
try!(self.vertical.to_css(dest));

if let Some(vert_pos) = self.vert_position {
if let Some(vert_key) = self.vert_keyword {
try!(vert_key.to_css(dest));
};
try!(vert_pos.to_css(dest));
};
Ok(())
}
}

impl HasViewportPercentage for Position {
fn has_viewport_percentage(&self) -> bool {
self.horizontal.has_viewport_percentage() || self.vertical.has_viewport_percentage()
let horiz_viewport = if let Some(horiz_pos) = self.horiz_position {
horiz_pos.has_viewport_percentage()
} else {
false
};

let vert_viewport = if let Some(vert_pos) = self.vert_position {
vert_pos.has_viewport_percentage()
} else {
false
};
horiz_viewport || vert_viewport
}
}
// http://dev.w3.org/csswg/css2/colors.html#propdef-background-position

#[derive(Clone, PartialEq, Copy)]
pub enum PositionComponent {
LengthOrPercentage(LengthOrPercentage),
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
pub enum Keyword {
Center,
Left,
Right,
Top,
Bottom,
}

// http://dev.w3.org/csswg/css2/colors.html#propdef-background-position
#[derive(Clone, PartialEq, Copy)]
pub enum PositionComponent {
Length(LengthOrPercentage),
Keyword(Keyword),
}

impl Position {
pub fn new(first: PositionComponent, second: PositionComponent)
pub fn new(first_position: Option<PositionComponent>, second_position: Option<PositionComponent>,
first_keyword: Option<PositionComponent>, second_keyword: Option<PositionComponent>)
-> Result<Position, ()> {
let (horiz, vert) = match (category(first), category(second)) {
// Check firts and second positions, this is more like for 2 value backgrounds.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment spelling

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix thanks!

let (mut horiz, mut vert) = if let Some(first_pos) = first_position {
if let Some(second_pos) = second_position {
match (category(first_pos), category(second_pos)) {
// Don't allow two vertical keywords or two horizontal keywords.
// also don't allow length/percentage values in the wrong position
(PositionCategory::HorizontalKeyword, PositionCategory::HorizontalKeyword) |
(PositionCategory::VerticalKeyword, PositionCategory::VerticalKeyword) |
(PositionCategory::LengthOrPercentage, PositionCategory::HorizontalKeyword) |
(PositionCategory::VerticalKeyword, PositionCategory::LengthOrPercentage) => return Err(()),

// Swap if both are keywords and vertical precedes horizontal.
(PositionCategory::VerticalKeyword, PositionCategory::HorizontalKeyword) |
(PositionCategory::VerticalKeyword, PositionCategory::OtherKeyword) |
(PositionCategory::OtherKeyword, PositionCategory::HorizontalKeyword) =>
(second_position, first_position),
// By default, horizontal is first.
_ => (first_position, second_position),
}
} else {
(first_position, second_position)
}
} else {
(first_position, second_position)
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be too hard to implement now. You can convert each position to a pair of Options for length and percentage, and then construct a calc value out of them. In case the keyword is right or bottom, you make the len/percentage negative and add 100%. For center, you add 50% but don't make it negative.


// Unwrap for checking if values are at right place.
let first_key = first_keyword.unwrap_or(PositionComponent::Keyword(Keyword::Left));
let second_key = second_keyword.unwrap_or(PositionComponent::Keyword(Keyword::Top));

// Check first and second keywords. This is for 4 value swapping.
let (horiz_keyword, vert_keyword) = match (category(first_key), category(second_key)) {
// Don't allow two vertical keywords or two horizontal keywords.
// also don't allow length/percentage values in the wrong position
(PositionCategory::HorizontalKeyword, PositionCategory::HorizontalKeyword) |
Expand All @@ -60,21 +125,80 @@ impl Position {
// Swap if both are keywords and vertical precedes horizontal.
(PositionCategory::VerticalKeyword, PositionCategory::HorizontalKeyword) |
(PositionCategory::VerticalKeyword, PositionCategory::OtherKeyword) |
(PositionCategory::OtherKeyword, PositionCategory::HorizontalKeyword) => (second, first),
(PositionCategory::OtherKeyword, PositionCategory::HorizontalKeyword) => {
let tmp = horiz;
horiz = vert;
vert = tmp;

(second_keyword, first_keyword)
},
// By default, horizontal is first.
_ => (first, second),
_ => (first_keyword, second_keyword),
};

// Unwrap keywords from PositionComponent and wrap with Option.
let (wrapped_horiz_key, wrapped_vert_key) = if let Some(PositionComponent::Keyword(horiz_key)) = horiz_keyword {
if let Some(PositionComponent::Keyword(vert_key)) = vert_keyword {
(Some(horiz_key), Some(vert_key))
} else {
(Some(horiz_key), None)
}
} else {
(None, None)
};

Ok(Position {
horizontal: horiz.to_length_or_percentage(),
vertical: vert.to_length_or_percentage(),
horiz_keyword: wrapped_horiz_key,
horiz_position: Some(horiz.unwrap().to_length_or_percentage()),
vert_keyword: wrapped_vert_key,
vert_position: Some(vert.unwrap().to_length_or_percentage()),
})
}

pub fn parse(input: &mut Parser) -> Result<Position, ()> {
let first = try!(PositionComponent::parse(input));
let second = input.try(PositionComponent::parse)
.unwrap_or(PositionComponent::Center);
Position::new(first, second)
.unwrap_or(PositionComponent::Keyword(Keyword::Center));

// Try to parse third and fourth values
if let Ok(third) = input.try(PositionComponent::parse) {
if let Ok(fourth) = input.try(PositionComponent::parse) {
// Handle 4 value background position
Position::new(Some(second), Some(fourth), Some(first), Some(third))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4-value can also be specified with the horizontal ones first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nvm, Position::new solves that.

} else {
// Handle 3 value background position
if let PositionCategory::LengthOrPercentage = category(first) {
// "20px bottom 20%"
Position::new(Some(first), Some(third), None, Some(second))
} else {
if let PositionCategory::LengthOrPercentage = category(second) {
if let PositionCategory::HorizontalKeyword = category(third) {
// "bottom 10% right"
Position::new(Some(second), None, Some(first), Some(third))
} else {
// "right 10px 50%"
Position::new(Some(second), Some(third), Some(first), None)
}
} else {
// "right bottom 10px"
Position::new(None, Some(third), Some(first), Some(second))
}
}
}
} else {
// Handle 2 value background position
Position::new(Some(first), Some(second), None, None)
}
}
}

impl ToCss for Keyword {
fn to_css<W>(&self, dest: &mut W) -> fmt::Result where W: fmt::Write {
match *self {
Keyword::Right => try!(dest.write_str("right ")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left, top, and center also serialize.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't write them because of comment above. Just explicit keywords are storing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also wrong, my bad: we want to store explicit keywords for background-positions and general positions. basic-shape needs special treatment whilst serializing.

Keyword::Bottom => try!(dest.write_str("bottom ")),
_ => (),
};
}
}

Expand All @@ -87,17 +211,19 @@ enum PositionCategory {
}

fn category(p: PositionComponent) -> PositionCategory {
match p {
PositionComponent::Left |
PositionComponent::Right =>
PositionCategory::HorizontalKeyword,
PositionComponent::Top |
PositionComponent::Bottom =>
PositionCategory::VerticalKeyword,
PositionComponent::Center =>
PositionCategory::OtherKeyword,
PositionComponent::LengthOrPercentage(_) =>
PositionCategory::LengthOrPercentage,
if let PositionComponent::Keyword(keyword) = p {
match keyword {
Keyword::Left |
Keyword::Right =>
PositionCategory::HorizontalKeyword,
Keyword::Top |
Keyword::Bottom =>
PositionCategory::VerticalKeyword,
Keyword::Center =>
PositionCategory::OtherKeyword,
}
} else {
PositionCategory::LengthOrPercentage
}
}

Expand All @@ -107,16 +233,16 @@ impl ToComputedValue for Position {
#[inline]
fn to_computed_value(&self, context: &Context) -> computed_position::Position {
computed_position::Position {
horizontal: self.horizontal.to_computed_value(context),
vertical: self.vertical.to_computed_value(context),
horizontal: self.horiz_position.to_computed_value(context),
vertical: self.vert_position.to_computed_value(context),
}
}
}

impl HasViewportPercentage for PositionComponent {
fn has_viewport_percentage(&self) -> bool {
match *self {
PositionComponent::LengthOrPercentage(length) => length.has_viewport_percentage(),
PositionComponent::Length(length) => length.has_viewport_percentage(),
_ => false
}
}
Expand All @@ -125,16 +251,16 @@ impl HasViewportPercentage for PositionComponent {
impl PositionComponent {
pub fn parse(input: &mut Parser) -> Result<PositionComponent, ()> {
input.try(LengthOrPercentage::parse)
.map(PositionComponent::LengthOrPercentage)
.map(PositionComponent::Length)
.or_else(|()| {
match try!(input.next()) {
Token::Ident(value) => {
match_ignore_ascii_case! { value,
"center" => Ok(PositionComponent::Center),
"left" => Ok(PositionComponent::Left),
"right" => Ok(PositionComponent::Right),
"top" => Ok(PositionComponent::Top),
"bottom" => Ok(PositionComponent::Bottom),
"center" => Ok(PositionComponent::Keyword(Keyword::Center)),
"left" => Ok(PositionComponent::Keyword(Keyword::Left)),
"right" => Ok(PositionComponent::Keyword(Keyword::Right)),
"top" => Ok(PositionComponent::Keyword(Keyword::Top)),
"bottom" => Ok(PositionComponent::Keyword(Keyword::Bottom)),
_ => Err(())
}
},
Expand All @@ -145,12 +271,14 @@ impl PositionComponent {
#[inline]
pub fn to_length_or_percentage(self) -> LengthOrPercentage {
match self {
PositionComponent::LengthOrPercentage(value) => value,
PositionComponent::Center => LengthOrPercentage::Percentage(Percentage(0.5)),
PositionComponent::Left |
PositionComponent::Top => LengthOrPercentage::Percentage(Percentage(0.0)),
PositionComponent::Right |
PositionComponent::Bottom => LengthOrPercentage::Percentage(Percentage(1.0)),
PositionComponent::Length(value) => value,
PositionComponent::Keyword(keyword) if keyword == Keyword::Center =>
LengthOrPercentage::Percentage(Percentage(0.5)),
PositionComponent::Keyword(keyword) if keyword == Keyword::Left ||
keyword == Keyword::Top => LengthOrPercentage::Percentage(Percentage(0.0)),
PositionComponent::Keyword(keyword) if keyword == Keyword::Right ||
keyword == Keyword::Bottom => LengthOrPercentage::Percentage(Percentage(1.0)),
PositionComponent::Keyword(_) => unimplemented!(), // TODO: All keywords are covered but rust forcing me to add this too?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe match further on keyword looks better and eliminate the compiler's complain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Probably this is better solution. I'll update soon, thanks!

}
}
}