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

Conversation

@dashed
Copy link
Contributor

commented Feb 2, 2017

Fixes #15207

Refactored as per #15207 (comment) .

  • Correct refactor? I'd appreciate any feedback on this.
  • ~~~proper borderstyle value for outline-style: auto;?~~~ ~~~(EDIT: deferred to a FIXME)~~~ (EDIT2: it is now solid for behaviour parity with firefox)
  • squash pending PR review
  • mako code review

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #15207 (github issue number if applicable).
  • There are tests for these changes

This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Feb 2, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/longhand/outline.mako.rs, components/style/properties/shorthand/outline.mako.rs
  • @emilio: components/style/properties/longhand/outline.mako.rs, components/style/properties/shorthand/outline.mako.rs
@dashed

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2017

As per spec: https://drafts.csswg.org/css-ui-3/#outline-style

In addition, in CSS3, outline-style accepts the value auto. The auto value permits the user agent to render a custom outline style, typically a style which is either a user interface default for the platform, or perhaps a style that is richer than can be described in detail in CSS, e.g. a rounded edge outline with semi-translucent outer pixels that appears to glow. As such, this specification does not define how the outline-color is incorporated or used (if at all) when rendering auto style outlines. User agents may treat auto as solid.

I am unsure what default border-style that outline-style should be for auto value. For now, it is BorderStyle::none. Let me know if this should be changed.

@dashed

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2017

Added test case for which outline-style: hidden; is not allowed.


#[derive(PartialEq, Eq, Clone, Debug)]
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
pub enum SpecifiedValue {

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 3, 2017

Member

Can you try whether we can make use of Either<Auto, BorderStyle> here? I think that would reduce a lot of code. The generic type is in components/style/values/mod.rs. You can also git grep for similar examples :)

This comment has been minimized.

Copy link
@dashed

dashed Feb 3, 2017

Author Contributor

Thanks for the hint 👍

@@ -0,0 +1,37 @@
/* This Source Code Form is subject to the terms of the Mozilla Public

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 3, 2017

Member

This file should be outline.rs. We usually group tests into one module :)

@wafflespeanut

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

I am unsure what default border-style that outline-style should be for auto value. For now, it is BorderStyle::none. Let me know if this should be changed.

As the spec says, this is specific to outline-style and it's up to the UA to determine how it should be rendered. I think we can safely ignore worrying about it while parsing :)

@dashed dashed force-pushed the dashed:gh-15207 branch from b0bc671 to fbefaf9 Feb 3, 2017
Copy link
Member

left a comment

Looks like Either is useful here. Thanks for trying it out. Just a few more comments :)

}
let outline_style = match style.get_outline().outline_style {
Either::First(_auto) => border_style::T::none,
Either::Second(border_style) => border_style,

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 5, 2017

Member

Note that we should be returning in case of border-style: none. Also, since we don't handle auto right now, let's drop a FIXME comment saying that outline-style: auto should be handled.

This comment has been minimized.

Copy link
@dashed

dashed Feb 5, 2017

Author Contributor

Doh. Fixing now. 👍

@@ -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,
needs_context=True, need_clone=True,

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 5, 2017

Member

needs_context is True by default. It can be ignored :)

result => result

pub fn parse(context: &ParserContext, input: &mut Parser) -> Result<SpecifiedValue, ()> {
match SpecifiedValue::parse(context, input) {

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 5, 2017

Member

and_then would suit better here.

@@ -10,7 +10,7 @@ ${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,
needs_context=True,

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 5, 2017

Member

ditto on needs_context

if outline_style == border_style::T::none {
return
}
let outline_style = match style.get_outline().outline_style {

This comment has been minimized.

Copy link
@dashed

dashed Feb 5, 2017

Author Contributor

@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.

This comment has been minimized.

Copy link
@dashed

dashed Feb 5, 2017

Author Contributor

Ignore above comment. It's addressed now.


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

This comment has been minimized.

Copy link
@dashed

dashed Feb 5, 2017

Author Contributor

@wafflespeanut is this fine?

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 5, 2017

Member

Yep

}
let outline_style = match style.get_outline().outline_style {
Either::First(_auto) => border_style::T::none,
Either::Second(border_style) => border_style,

This comment has been minimized.

Copy link
@dashed

dashed Feb 5, 2017

Author Contributor

Doh. Fixing now. 👍

@dashed

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2017

@wafflespeanut Hmm.. Apparently I did a "github review" without knowing how it works... so I wasn't totally sure if you saw those comments 🤣

EDIT: i thought "pending" comments was visible to everybody.

use style_traits::ToCss;

#[test]
fn test_outline_style() {

This comment has been minimized.

Copy link
@dashed

dashed Feb 5, 2017

Author Contributor

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

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 5, 2017

Member

Cool with me.

use values::NoViewportPercentage;
use values::computed::ComputedValueAsSpecified;

pub type SpecifiedValue = Either<Auto, BorderStyle>;

This comment has been minimized.

Copy link
@emilio

emilio Feb 5, 2017

Member

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

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 5, 2017

Member

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

This comment has been minimized.

Copy link
@emilio

emilio Feb 5, 2017

Member

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

let outline_style = match style.get_outline().outline_style {
// FIXME: handle case when "outline-style: auto". For now, assume
// auto is akin to border_style::T::none.
Either::First(_) | Either::Second(border_style::T::none) => return,

This comment has been minimized.

Copy link
@emilio

emilio Feb 5, 2017

Member

could you use Either::First(_auto) here, so it's clear we're not ignoring the style.

Also, let's use border_style::T::solid instead of not drawing anything, since it's what Firefox does (which has outline-style: auto handling disabled by default).

@wafflespeanut

This comment has been minimized.

Copy link
Member

commented Feb 5, 2017

Once you've addressed the comments, please squash your commits appropriately.

@dashed

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2017

Addressed the comments and squashed the commits.

@wafflespeanut

This comment has been minimized.

Copy link
Member

commented Feb 5, 2017

Thanks for working on this! :)

@bors-servo r=Wafflespeanut,emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2017

📌 Commit ad5280f has been approved by Wafflespeanut,emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2017

⌛️ Testing commit ad5280f with merge 7538858...

@dashed

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2017

hmm this macro seems to be the culprit:

macro_rules! define_css_keyword_enum {

@dashed

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2017


or somewhere: style/properties/gecko.mako.rs 
@dashed dashed force-pushed the dashed:gh-15207 branch from 077e4f0 to 25450a3 Feb 5, 2017
@dashed

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2017

I made some changes with regards to the mako scaffolding code to make ./mach build-geckolib happy. I'm not familiar with mako templates, since it's my first time using it, and I appreciate any critical feedback to make it better 👍

% endfor
};
${set_gecko_property(gecko_ffi_name, "result")}
}
</%def>

<%def name="impl_keyword_clone(ident, gecko_ffi_name, keyword)">
<%def name="impl_keyword_clone(ident, gecko_ffi_name, keyword, resolver=None)">

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 6, 2017

Member

While it's probably a clever idea to wrap the keyword-constant conversion, I think this adds a bit of complication to what's there already (I don't especially like having a check for outline-style). Also, since this is only needed for outline-style, let's simply write the autogenerated code for now.

This comment has been minimized.

Copy link
@dashed

dashed Feb 6, 2017

Author Contributor

Agreed, the manual conditions outline-style felt hacky. I wasn't entirely sure if mako templates should still be used.

I'll look into the approach for handwriting out the code for outline-style :)

@emilio

This comment has been minimized.

Copy link
Member

commented Feb 6, 2017

Yes, we should just implement clone_outline_style manually in Gecko.

@dashed

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2017

I unrolled the mako code for outline-style in components/style/properties/gecko.mako.rs. Let me know if there are improvements that need to be made. 👍

@wafflespeanut

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

The changes look good to me. Can you please squash the commits? Thanks.

#[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")} {

This comment has been minimized.

Copy link
@emilio

emilio Feb 8, 2017

Member

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),

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 8, 2017

Member

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

@@ -924,7 +924,7 @@ fn static_assert() {
structs::${border_style_keyword.gecko_constant(value)} ${border_style_keyword.maybe_cast("u8")},
% endfor
Either::First(Auto) =>
structs::${border_style_keyword.gecko_constant('solid')} ${border_style_keyword.maybe_cast("u8")},
structs::${border_style_keyword.gecko_constant('auto')} ${border_style_keyword.maybe_cast("u8")},

This comment has been minimized.

Copy link
@dashed

dashed Feb 9, 2017

Author Contributor

@wafflespeanut @emilio Can you let me know if this is the right approach for usage of gecko based prefs?

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Feb 9, 2017

Member

You're right! Squash please 😄

@@ -941,6 +941,7 @@ fn static_assert() {
% 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),

This comment has been minimized.

Copy link
@dashed

dashed Feb 9, 2017

Author Contributor

@wafflespeanut @emilio same thing here as above comment?

@dashed dashed force-pushed the dashed:gh-15207 branch from e122fd9 to 09d4751 Feb 9, 2017
@dashed

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

@wafflespeanut squashed :)

@wafflespeanut

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

Thanks!

@bors-servo r=Wafflespeanut,emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

📌 Commit 09d4751 has been approved by Wafflespeanut,emilio

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

⌛️ Testing commit 09d4751 with merge 368af6f...

bors-servo added a commit that referenced this pull request Feb 9, 2017
Refactor outline-style to accept "auto" value in addition to border-style values.

Fixes #15207

Refactored as per #15207 (comment) .

<!-- Please describe your changes on the following line: -->

- [x] Correct refactor? I'd appreciate any feedback on this.
- [x] ~~~proper borderstyle value for `outline-style: auto;`?~~~ ~~~(EDIT: deferred to a `FIXME`)~~~ (EDIT2: it is now solid for behaviour parity with firefox)
- [x] squash pending PR review
- [x] mako code review

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #15207 (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15357)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2017

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: Wafflespeanut,emilio
Pushing 368af6f to master...

@bors-servo bors-servo merged commit 09d4751 into servo:master Feb 9, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo referenced this pull request Feb 9, 2017
3 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.