Add spec links to all CSS properties #14827

Merged
merged 30 commits into from Jan 3, 2017

Projects

None yet

5 participants

@Manishearth
Member
Manishearth commented Jan 2, 2017 edited

This change is Reviewable

@metajack metajack was assigned by highfive Jan 2, 2017
@highfive
highfive commented Jan 2, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/properties/shorthand/background.mako.rs, components/style/properties/shorthand/border.mako.rs, components/style/properties/data.py, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/color.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/shorthand/position.mako.rs
  • @emilio: components/style/properties/shorthand/background.mako.rs, components/style/properties/shorthand/border.mako.rs, components/style/properties/data.py, components/style/properties/longhand/background.mako.rs, components/style/properties/longhand/color.mako.rs, components/style/properties/helpers.mako.rs, components/style/properties/longhand/position.mako.rs, components/style/properties/longhand/border.mako.rs, components/style/properties/shorthand/position.mako.rs
@highfive
highfive commented Jan 2, 2017

warning Warning warning

  • These commits modify style code, but no tests are modified. Please consider adding a test!
@Manishearth
Member

r? @emilio

(still WIP)

I also added support for some logical values we seem to have missed (MDN helpfully tells you when there are specs which supplement the existing one), which needs review.

@highfive highfive assigned emilio and unassigned metajack Jan 2, 2017
@emilio
Member
emilio commented Jan 3, 2017

Does Gecko implement float/clear: inline-{start, end}?

The rest looks great! Thanks for doing this :)

@Manishearth
Member

Does Gecko implement float/clear: inline-{start, end}?

Yes. Behind a pref (enabled by default on nightly/aurora), but yes.

@Manishearth
Member

At some point we'll need to figure out our story for experimental gecko props. It's especially complicated because gecko has support for both completely turning off some properties as well as just preffing off some values.

@Manishearth Manishearth changed the title from WIP add spec links to all CSS properties to Add spec links to all CSS properties Jan 3, 2017
@Manishearth
Member

Done. r? @emilio

components/style/properties/data.py
@@ -90,6 +90,8 @@ def __init__(self, style_struct, name, spec="NO SPEC", animatable=None, derived_
allowed_in_keyframe_block=True, complex_color=False, cast_type='u8',
has_uncacheable_values=False, logical=False):
self.name = name
+ if not spec:
+ raise TypeError("Spec should be specified for %s" %name)
@emilio
emilio Jan 3, 2017 Member

nit: I think the common style is with spaces around both sides of %? Here and below.

components/style/properties/data.py
@@ -134,6 +136,8 @@ class Shorthand(object):
def __init__(self, name, sub_properties, spec="NO SPEC", experimental=False, internal=False,
@emilio
emilio Jan 3, 2017 Member

Also, this patch is useless until you change both spec="NO SPEC" to spec=None, right?

<%helpers:longhand name="-servo-display-for-hypothetical-box"
animatable="False"
derived_from="display"
- products="servo">
+ products="servo"
+ spec="Nonstandard (for user-agent stylesheets only)">
@emilio
emilio Jan 3, 2017 Member

Instead of this, I'd probably write "Internal (not web-exposed)", what do you think?

${helpers.single_keyword("-servo-overflow-clip-box", "padding-box content-box",
- products="servo", animatable=False, internal=True)}
+ products="servo", animatable=False, internal=True,
+ spec="Nonstandard, may be standardized (https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-clip-box)")}
@emilio
emilio Jan 3, 2017 edited Member

Similarly, this should probably say that this is not web-exposed?

${helpers.single_keyword("overflow-clip-box", "padding-box content-box",
- products="gecko", animatable=False, internal=True)}
+ products="gecko", animatable=False, internal=True,
@emilio
emilio Jan 3, 2017 Member

Is this really an internal property in Gecko? (from the MDN docs it seems web exposed).

If it is exposed, please address this or file an issue for a followup.

If it isn't, please note it in the spec=.

@Manishearth
Manishearth Jan 3, 2017 Member

Internal but may be exposed later.

@@ -7,7 +7,12 @@
<% data.new_style_struct("Margin", inherited=False) %>
% for side in ALL_SIDES:
+ <%
+ spec = "https://drafts.csswg.org/css-box/#propdef-%s" % side[0]
@emilio
emilio Jan 3, 2017 Member

is there a margin- missing here? This will generate links like propdef-right, right?

@@ -9,12 +9,16 @@
// https://drafts.csswg.org/css-ui-3/
<% data.new_style_struct("UI", inherited=False, gecko_name="UIReset") %>
+// TODO spec says that UAs should not support this
@emilio
emilio Jan 3, 2017 Member

File a bug and reference it here please.

@bors-servo
Contributor

☔️ The latest upstream changes (presumably #14830) made this pull request unmergeable. Please resolve the merge conflicts.

@Manishearth
Member

Updated

@emilio
emilio approved these changes Jan 3, 2017 View changes
@@ -185,6 +185,7 @@
if property is None:
return ""
%>
+ /// ${property.spec}
@emilio
emilio Jan 3, 2017 Member

Maybe worth to be a bit more descriptive (using also ${property.name}.

Ideally we could come up with pretty good messages, but that's not required to land this.

@emilio
Member
emilio commented Jan 3, 2017

@bors-servo r+

Thanks for doing this! :)

@bors-servo
Contributor

📌 Commit 645ebd3 has been approved by emilio

@bors-servo
Contributor

⌛️ Testing commit 645ebd3 with merge b8ebf21...

@bors-servo bors-servo added a commit that referenced this pull request Jan 3, 2017
@bors-servo bors-servo Auto merge of #14827 - Manishearth:spec-doc, r=emilio
Add spec links to all CSS properties

<!-- 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/14827)
<!-- Reviewable:end -->
b8ebf21
@bors-servo
Contributor

💔 Test failed - linux-dev

@emilio
Member
emilio commented Jan 3, 2017

Well, it works:

--- stderr


Traceback (most recent call last):
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/build.py", line 60, in render
    return template.render(**context).encode("utf8")
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/Mako-0.9.1.zip/mako/template.py", line 443, in render
    return runtime._render(self, self.callable_, args, data)
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/Mako-0.9.1.zip/mako/runtime.py", line 807, in _render
    **_kwargs_for_callable(callable_, data))
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/Mako-0.9.1.zip/mako/runtime.py", line 839, in _render_context
    _exec_template(inherit, lclcontext, args=args, kwargs=kwargs)
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/Mako-0.9.1.zip/mako/runtime.py", line 865, in _exec_template
    callable_(context, *args, **kwargs)
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/properties.mako.rs", line 71, in render_body
    <%include file="/longhand/effects.mako.rs" />
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/Mako-0.9.1.zip/mako/runtime.py", line 734, in _include_file
    callable_(ctx, **_kwargs_for_include(callable_, context._data, **kwargs))
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/longhand/effects.mako.rs", line 10, in render_body
    ${helpers.predefined_type("opacity",
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/helpers.mako.rs", line 20, in render_predefined_type
    <%call expr="longhand(name, predefined_type=type, **kwargs)">
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/helpers.mako.rs", line 588, in longhand
    </%call>
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/helpers.mako.rs", line 8, in render_longhand
    <%call expr="raw_longhand(name, **kwargs)">
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/helpers.mako.rs", line 179, in raw_longhand
    </%call>
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/helpers.mako.rs", line 183, in render_raw_longhand
    <%
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/data.py", line 226, in declare_longhand
    longhand = Longhand(self.current_style_struct, name, **kwargs)
  File "/home/servo/buildbot/slave/linux-dev/build/components/style/properties/data.py", line 94, in __init__
    raise TypeError("Spec should be specified for %s" % name)
TypeError: Spec should be specified for opacity
@Manishearth
Member

@bors-servo r=emilio

@bors-servo
Contributor

📌 Commit ac6fb96 has been approved by emilio

@bors-servo
Contributor

⌛️ Testing commit ac6fb96 with merge 9696403...

@bors-servo bors-servo added a commit that referenced this pull request Jan 3, 2017
@bors-servo bors-servo Auto merge of #14827 - Manishearth:spec-doc, r=emilio
Add spec links to all CSS properties

<!-- 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/14827)
<!-- Reviewable:end -->
9696403
@bors-servo
Contributor

💔 Test failed - linux-dev

Manishearth added some commits Jan 2, 2017
@Manishearth Manishearth Support logical values for float/clear 6a88723
@Manishearth Manishearth Add spec links for column properties dedebc5
@Manishearth Manishearth Add spec links for counter properties be8f9ad
@Manishearth Manishearth Add spec links for effects properties b567622
@Manishearth Manishearth Add spec links for font properties 82835c8
@Manishearth Manishearth Add spec links for inherited_box properties 71026c5
@Manishearth Manishearth Add spec links for inherited_svg properties ff4893e
@Manishearth Manishearth Add spec links for inherited_table properties 096dca9
@Manishearth Manishearth Add spec links for inherited_text properties 26a9b45
@Manishearth Manishearth Add spec links for list properties 6138b6a
@Manishearth Manishearth Add spec links for margin properties, support spec links on four_side…
…s_shorthand
3128694
@Manishearth Manishearth Add spec links for mask properties fdd6a40
@Manishearth Manishearth Add spec links for outline properties e7464b5
@Manishearth Manishearth Add spec links for padding properties 6cf15de
@Manishearth Manishearth Add spec links for pointing properties baaddf6
@Manishearth Manishearth Add spec links for svg properties 7238cfb
@Manishearth Manishearth Add spec links for table properties 24f4388
@Manishearth Manishearth Add spec links for text properties 450a5d1
@Manishearth Manishearth Add spec links for ui properties 0c6829a
@Manishearth Manishearth Add spec links for xul properties 9a2945b
@Manishearth Manishearth Make spec links mandatory ca6ada8
@Manishearth Manishearth Add spec links for opacity c89368d
@Manishearth Manishearth Add spec links for border shorthands d873bb8
@Manishearth Manishearth Add spec link for text-emphasis
1094c68
@Manishearth
Member

@bors-servo r=emilio

@bors-servo
Contributor

📌 Commit 1094c68 has been approved by emilio

@bors-servo
Contributor

⌛️ Testing commit 1094c68 with merge bfb1de9...

@bors-servo bors-servo added a commit that referenced this pull request Jan 3, 2017
@bors-servo bors-servo Auto merge of #14827 - Manishearth:spec-doc, r=emilio
Add spec links to all CSS properties

<!-- 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/14827)
<!-- Reviewable:end -->
bfb1de9
@bors-servo
Contributor

💔 Test failed - linux-dev

@Manishearth Manishearth Fix image-orientation line-wrap
94412ab
@Manishearth
Member

@bors-servo r=emilio

@bors-servo
Contributor

📌 Commit 94412ab has been approved by emilio

@bors-servo
Contributor

⌛️ Testing commit 94412ab with merge 61f6454...

@bors-servo bors-servo added a commit that referenced this pull request Jan 3, 2017
@bors-servo bors-servo Auto merge of #14827 - Manishearth:spec-doc, r=emilio
Add spec links to all CSS properties

<!-- 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/14827)
<!-- Reviewable:end -->
61f6454
@bors-servo bors-servo merged commit 94412ab into servo:master Jan 3, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
homu Test successful
Details
@emilio
Member
emilio commented Jan 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment