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

Refactor outline-style to accept "auto" value in addition to border-style values. #15357

Merged
merged 1 commit into from Feb 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
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
11 changes: 7 additions & 4 deletions components/layout/display_list_builder.rs
Expand Up @@ -1072,15 +1072,18 @@ impl FragmentDisplayListBuilding for Fragment {
style: &ServoComputedValues,
bounds: &Rect<Au>,
clip: &ClippingRegion) {
use style::values::Either;

let width = style.get_outline().outline_width;
if width == Au(0) {
return
}

let outline_style = style.get_outline().outline_style;
if outline_style == border_style::T::none {
return
}
let outline_style = match style.get_outline().outline_style {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wafflespeanut I stopped around here when refactoring towards Either. I am unsure if I should continue to refactor gfx::display_list::BorderDisplayItem.

Temporarily, the outline is transformed to border_style::T::none.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore above comment. It's addressed now.

Either::First(_auto) => border_style::T::solid,
Either::Second(border_style::T::none) => return,
Either::Second(border_style) => border_style
};

// Outlines are not accounted for in the dimensions of the border box, so adjust the
// absolute bounds.
Expand Down
36 changes: 34 additions & 2 deletions components/style/properties/gecko.mako.rs
Expand Up @@ -59,6 +59,8 @@ use std::ptr;
use std::sync::Arc;
use std::cmp;
use values::computed::ToComputedValue;
use values::{Either, Auto};
use computed_values::border_style;

pub mod style_structs {
% for style_struct in data.style_structs:
Expand Down Expand Up @@ -913,7 +915,38 @@ fn static_assert() {
skip_longhands="${skip_outline_longhands}"
skip_additionals="*">

<% impl_keyword("outline_style", "mOutlineStyle", border_style_keyword, need_clone=True) %>
#[allow(non_snake_case)]
pub fn set_outline_style(&mut self, v: longhands::outline_style::computed_value::T) {
// FIXME(bholley): Align binary representations and ditch |match| for cast + static_asserts
let result = match v {
% for value in border_style_keyword.values_for('gecko'):
Either::Second(border_style::T::${to_rust_ident(value)}) =>
structs::${border_style_keyword.gecko_constant(value)} ${border_style_keyword.maybe_cast("u8")},
% endfor
Either::First(Auto) =>
structs::${border_style_keyword.gecko_constant('auto')} ${border_style_keyword.maybe_cast("u8")},
};
${set_gecko_property("mOutlineStyle", "result")}
}

#[allow(non_snake_case)]
pub fn copy_outline_style_from(&mut self, other: &Self) {
self.gecko.mOutlineStyle = other.gecko.mOutlineStyle;
}

#[allow(non_snake_case)]
pub fn clone_outline_style(&self) -> longhands::outline_style::computed_value::T {
// FIXME(bholley): Align binary representations and ditch |match| for cast + static_asserts
match ${get_gecko_property("mOutlineStyle")} ${border_style_keyword.maybe_cast("u32")} {
Copy link
Member

Choose a reason for hiding this comment

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

We should check auto here, which we don't right now. You can add a branch here like: structs::NS_STYLE_BORDER_STYLE_AUTO => Either::First(Auto),

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I assumed that we're setting auto as solid (forgot about gecko's pref-based setting). If that's the case, then we should also set Either::First(Auto) as NS_STYLE_BORDER_STYLE_AUTO

% for value in border_style_keyword.values_for('gecko'):
structs::${border_style_keyword.gecko_constant(value)} => Either::Second(border_style::T::${value}),
% endfor
structs::${border_style_keyword.gecko_constant('auto')} => Either::First(Auto),
% if border_style_keyword.gecko_inexhaustive:
x => panic!("Found unexpected value in style struct for outline_style property: {:?}", x),
% endif
}
}

<% impl_app_units("outline_width", "mActualOutlineWidth", need_clone=True,
round_to_pixels=True) %>
Expand Down Expand Up @@ -2821,4 +2854,3 @@ pub unsafe extern "C" fn Servo_GetStyleVariables(_cv: ServoComputedValuesBorrowe
-> *const nsStyleVariables {
&*EMPTY_VARIABLES_STRUCT
}

2 changes: 1 addition & 1 deletion components/style/properties/longhand/border.mako.rs
Expand Up @@ -26,7 +26,7 @@
% for side in ALL_SIDES:
${helpers.predefined_type("border-%s-style" % side[0], "BorderStyle",
"specified::BorderStyle::none",
needs_context=False, need_clone=True,
need_clone=True,
alias=maybe_moz_logical_alias(product, side, "-moz-border-%s-style"),
spec=maybe_logical_spec(side, "style"),
animatable=False, logical = side[1])}
Expand Down
45 changes: 37 additions & 8 deletions components/style/properties/longhand/outline.mako.rs
Expand Up @@ -16,16 +16,45 @@ ${helpers.predefined_type("outline-color", "CSSColor", "::cssparser::Color::Curr

<%helpers:longhand name="outline-style" need_clone="True" animatable="False"
spec="https://drafts.csswg.org/css-ui/#propdef-outline-style">
pub use values::specified::BorderStyle as SpecifiedValue;
pub fn get_initial_value() -> SpecifiedValue { SpecifiedValue::none }

use std::fmt;
use style_traits::ToCss;
use values::specified::BorderStyle;
use values::NoViewportPercentage;
use values::computed::ComputedValueAsSpecified;

pub type SpecifiedValue = Either<Auto, BorderStyle>;
Copy link
Member

Choose a reason for hiding this comment

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

@wafflespeanut why using Either here instead of adding an auto variant to BorderStyle?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because border-style doesn't have auto and outline-style demands auto | <border-style>?

Copy link
Member

@emilio emilio Feb 5, 2017

Choose a reason for hiding this comment

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

right, thought the exception was the other way around, sorry for the noise.


impl SpecifiedValue {
#[inline]
pub fn none_or_hidden(&self) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wafflespeanut is this fine?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep

match *self {
Either::First(ref _auto) => false,
Either::Second(ref border_style) => border_style.none_or_hidden()
}
}
}

#[inline]
pub fn get_initial_value() -> computed_value::T {
Either::Second(BorderStyle::none)
}

pub mod computed_value {
pub use values::specified::BorderStyle as T;
pub type T = super::SpecifiedValue;
}
pub fn parse(_: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
match SpecifiedValue::parse(input) {
Ok(SpecifiedValue::hidden) => Err(()),
result => result
}

pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
SpecifiedValue::parse(context, input)
.and_then(|result| {
if let Either::Second(BorderStyle::hidden) = result {
// The outline-style property accepts the same values as border-style,
// except that 'hidden' is not a legal outline style.
Err(())
} else {
Ok(result)
}
})
}
</%helpers:longhand>

Expand Down
3 changes: 1 addition & 2 deletions components/style/properties/shorthand/border.mako.rs
Expand Up @@ -10,7 +10,6 @@ ${helpers.four_sides_shorthand("border-color", "border-%s-color", "specified::CS

${helpers.four_sides_shorthand("border-style", "border-%s-style",
"specified::BorderStyle::parse",
needs_context=False,
spec="https://drafts.csswg.org/css-backgrounds/#border-style")}

<%helpers:shorthand name="border-width" sub_properties="${
Expand Down Expand Up @@ -61,7 +60,7 @@ pub fn parse_border(context: &ParserContext, input: &mut Parser)
}
}
if style.is_none() {
if let Ok(value) = input.try(specified::BorderStyle::parse) {
if let Ok(value) = input.try(|i| specified::BorderStyle::parse(context, i)) {
style = Some(value);
any = true;
continue
Expand Down
4 changes: 2 additions & 2 deletions components/style/properties/shorthand/outline.mako.rs
Expand Up @@ -6,7 +6,7 @@

<%helpers:shorthand name="outline" sub_properties="outline-color outline-style outline-width"
spec="https://drafts.csswg.org/css-ui/#propdef-outline">
use properties::longhands::outline_width;
use properties::longhands::{outline_width, outline_style};
use values::specified;
use parser::Parse;

Expand All @@ -25,7 +25,7 @@
}
}
if style.is_none() {
if let Ok(value) = input.try(specified::BorderStyle::parse) {
if let Ok(value) = input.try(|input| outline_style::parse(context, input)) {
style = Some(value);
any = true;
continue
Expand Down
4 changes: 2 additions & 2 deletions components/style/values/mod.rs
Expand Up @@ -25,9 +25,9 @@ macro_rules! define_numbered_css_keyword_enum {
$( $variant = $value ),+
}

impl $name {
impl Parse for $name {
#[allow(missing_docs)]
pub fn parse(input: &mut ::cssparser::Parser) -> Result<$name, ()> {
fn parse(_context: &ParserContext, input: &mut ::cssparser::Parser) -> Result<$name, ()> {
match_ignore_ascii_case! { try!(input.expect_ident()),
$( $css => Ok($name::$variant), )+
_ => Err(())
Expand Down
19 changes: 18 additions & 1 deletion tests/unit/style/parsing/border.rs
Expand Up @@ -5,11 +5,12 @@
use cssparser::Parser;
use media_queries::CSSErrorReporterTest;
use servo_url::ServoUrl;
use style::parser::ParserContext;
use style::parser::{ParserContext, Parse};
use style::properties::longhands::{border_image_outset, border_image_repeat, border_image_slice};
use style::properties::longhands::{border_image_source, border_image_width};
use style::properties::shorthands::border_image;
use style::stylesheets::Origin;
use style_traits::ToCss;

#[test]
fn border_image_shorthand_should_parse_when_all_properties_specified() {
Expand Down Expand Up @@ -122,3 +123,19 @@ fn border_image_outset_should_return_length_on_length_zero() {
let result = border_image_outset::parse(&context, &mut parser);
assert_eq!(result.unwrap(), parse_longhand!(border_image_outset, "0em"));
}

#[test]
fn test_border_style() {
use style::values::specified::BorderStyle;

assert_roundtrip_with_context!(BorderStyle::parse, r#"none"#);
assert_roundtrip_with_context!(BorderStyle::parse, r#"hidden"#);
assert_roundtrip_with_context!(BorderStyle::parse, r#"solid"#);
assert_roundtrip_with_context!(BorderStyle::parse, r#"double"#);
assert_roundtrip_with_context!(BorderStyle::parse, r#"dotted"#);
assert_roundtrip_with_context!(BorderStyle::parse, r#"dashed"#);
assert_roundtrip_with_context!(BorderStyle::parse, r#"groove"#);
assert_roundtrip_with_context!(BorderStyle::parse, r#"ridge"#);
assert_roundtrip_with_context!(BorderStyle::parse, r#"inset"#);
assert_roundtrip_with_context!(BorderStyle::parse, r#"outset"#);
}
1 change: 1 addition & 0 deletions tests/unit/style/parsing/mod.rs
Expand Up @@ -75,6 +75,7 @@ mod image;
mod inherited_box;
mod inherited_text;
mod mask;
mod outline;
mod position;
mod selectors;
mod supports;
Expand Down
37 changes: 37 additions & 0 deletions tests/unit/style/parsing/outline.rs
@@ -0,0 +1,37 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

use cssparser::Parser;
use media_queries::CSSErrorReporterTest;
use style::parser::ParserContext;
use style::stylesheets::Origin;
use style_traits::ToCss;

#[test]
fn test_outline_style() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wafflespeanut there doesn't seem to be similar a test case for border-style parsing. Shall I add one within this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool with me.

use style::properties::longhands::outline_style;

assert_roundtrip_with_context!(outline_style::parse, r#"auto"#);
assert_roundtrip_with_context!(outline_style::parse, r#"none"#);
assert_roundtrip_with_context!(outline_style::parse, r#"solid"#);
assert_roundtrip_with_context!(outline_style::parse, r#"double"#);
assert_roundtrip_with_context!(outline_style::parse, r#"dotted"#);
assert_roundtrip_with_context!(outline_style::parse, r#"dashed"#);
assert_roundtrip_with_context!(outline_style::parse, r#"groove"#);
assert_roundtrip_with_context!(outline_style::parse, r#"ridge"#);
assert_roundtrip_with_context!(outline_style::parse, r#"inset"#);
assert_roundtrip_with_context!(outline_style::parse, r#"outset"#);

{
// The outline-style property accepts the same values as border-style,
// except that 'hidden' is not a legal outline style.

let url = ::servo_url::ServoUrl::parse("http://localhost").unwrap();
let context = ParserContext::new(Origin::Author, &url, Box::new(CSSErrorReporterTest));
let mut parser = Parser::new(r#"hidden"#);
let parsed = outline_style::parse(&context, &mut parser);
assert!(parsed.is_err());
};

}
26 changes: 23 additions & 3 deletions tests/unit/style/properties/serialization.rs
Expand Up @@ -8,7 +8,8 @@ pub use style::properties::{DeclaredValue, PropertyDeclaration, PropertyDeclarat
pub use style::values::specified::{BorderStyle, BorderWidth, CSSColor, Length, NoCalcLength};
pub use style::values::specified::{LengthOrPercentage, LengthOrPercentageOrAuto, LengthOrPercentageOrAutoOrContent};
pub use style::properties::longhands::outline_color::computed_value::T as ComputedColor;
pub use style::values::RGBA;
pub use style::properties::longhands::outline_style::SpecifiedValue as OutlineStyle;
pub use style::values::{RGBA, Auto};
pub use style::values::specified::url::{UrlExtraData, SpecifiedUrl};
pub use style_traits::ToCss;

Expand Down Expand Up @@ -459,14 +460,15 @@ mod shorthand_serialization {

mod outline {
use style::properties::longhands::outline_width::SpecifiedValue as WidthContainer;
use style::values::Either;
use super::*;

#[test]
fn outline_should_show_all_properties_when_set() {
let mut properties = Vec::new();

let width = DeclaredValue::Value(WidthContainer(Length::from_px(4f32)));
let style = DeclaredValue::Value(BorderStyle::solid);
let style = DeclaredValue::Value(Either::Second(BorderStyle::solid));
let color = DeclaredValue::Value(CSSColor {
parsed: ComputedColor::RGBA(RGBA { red: 1f32, green: 0f32, blue: 0f32, alpha: 1f32 }),
authored: None
Expand All @@ -485,7 +487,7 @@ mod shorthand_serialization {
let mut properties = Vec::new();

let width = DeclaredValue::Value(WidthContainer(Length::from_px(4f32)));
let style = DeclaredValue::Value(BorderStyle::solid);
let style = DeclaredValue::Value(Either::Second(BorderStyle::solid));
let color = DeclaredValue::Initial;

properties.push(PropertyDeclaration::OutlineWidth(width));
Expand Down Expand Up @@ -513,6 +515,24 @@ mod shorthand_serialization {
let serialization = shorthand_properties_to_string(properties);
assert_eq!(serialization, "outline: 4px none rgb(255, 0, 0);");
}

#[test]
fn outline_should_serialize_correctly_when_style_is_auto() {
let mut properties = Vec::new();

let width = DeclaredValue::Value(WidthContainer(Length::from_px(4f32)));
let style = DeclaredValue::Value(Either::First(Auto));
let color = DeclaredValue::Value(CSSColor {
parsed: ComputedColor::RGBA(RGBA { red: 1f32, green: 0f32, blue: 0f32, alpha: 1f32 }),
authored: None
});
properties.push(PropertyDeclaration::OutlineWidth(width));
properties.push(PropertyDeclaration::OutlineStyle(style));
properties.push(PropertyDeclaration::OutlineColor(color));

let serialization = shorthand_properties_to_string(properties);
assert_eq!(serialization, "outline: 4px auto rgb(255, 0, 0);");
}
}

#[test]
Expand Down