1215878 - implement revert keyword

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Cameron McCormack (:heycam)]

WebKit just implemented the revert keyword.  It's more useful than unset, when used with the all shorthand, so I think we should implement it too.

We should be able to just have an additional array on nsRuleData that stores the level each nsCSSValue came from.  Then when we encounter revert and have a non-eCSSUnit_Null value on the nsRuleData in a MapRuleInfoInto, we can overwrite the value with eCSSUnit_null if the current mLevel is greater than the level we have in the array.  We'll also need to adjust the RuleDetail that gets returned from CheckSpecifiedProperties when we encounter revert.

Comment 1

[Chris Rebert]

Could someone add [parity-safari] to the Whiteboard?
See https://developer.apple.com/library/prerelease/mac/releasenotes/General/WhatsNewInSafari/Articles/Safari_9_1.html#//apple_ref/doc/uid/TP40014305-CH10-SW14

Comment 2

[yisibl]

Update MDN docs: 

https://developer.mozilla.org/en-US/docs/Web/CSS/all
https://developer.mozilla.org/en-US/docs/Web/CSS/revert

Comment 3

[rtm]

This feature turns out to be crucial to a nextgen CSS framework I am working on, but I also think it would be useful for the average developer.

Comment 4

[François REMY]

FWIW, you can use "revert" today in most browsers by doing ```property:var(--\\revert)``` instead.
No that is not a dirty hack ^_^ You are welcome :-)

Comment 5

[Claude Pache]

(In reply to François REMY from comment #4)
> FWIW, you can use "revert" today in most browsers by doing
> ```property:var(--\\revert)``` instead.
> No that is not a dirty hack ^_^ You are welcome :-)

Unfortunately no, an invalid custom property value is equivalent to `unset`, not `revert`.

Comment 6

[Lee Shallis]

The way to implement is simple, add a oldstyle & newstyle type index/pointer paramaters to each identified ruleset, when revert or -moz-restyle (for clarity this is for undoing a revert) are used you simply look up the previously overriden or the next overide via those parameters.

Comment 7

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 8

[Mats Palmgren (inactive)]

Any thoughts on how this could be implemented in Stylo?
(see original comment for some thoughts on how it could be implemented
in the old style system)

Comment 9

[Emilio Cobos Álvarez [:emilio]]

It should be pretty easy to implement, I'd think.

We could have in `apply_declarations` a bitfield from `property` to `cascade levels to skip`.

When you find a declaration with the `revert` value, you just add the current cascade level to the bitfield. When the current cascade level is in the bitfield, you just skip over the declaration.

We'd need to do the `revert` check before inserting into the `seen` bitfield:

  https://searchfox.org/mozilla-central/rev/99cbc0aec3e1c0b65ff9052523fb5c181b248f57/servo/components/style/properties/properties.mako.rs#4056

But that should be about it, since afterwards we'd skip all the declarations in the cascade level. It may be worth storing the `seen` bitfield and this new bitfield in the same struct, since they should be used together.

Should take me very little to write the code if someone writes the WPT tests for the edge-cases, actually... :)

Comment 10

[Emilio Cobos Álvarez [:emilio]]

There's of course a bunch of auditing that must be done with the current users of CSSWideKeyword to see what makes sense to do there, but I think it should be trivial.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Custom properties may be a bit more annoying, because with that approach we'd actually need a HashMap for that, which could be a bit slow.

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Also, I'm not sure whether we _want_ that behavior for custom properties, it may be reasonable to just special-case them and say it behaves as `unset`, and then it'd be fast...

I certainly don't see a use great use-case for that... But we'd need to see WebKit's behavior and carry it to the CSS working group.

Comment 13

[Lee Shallis]

There is a use case for users, xstyle/stylish global style style then when a site miss behaves add a special case to revert style

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Sure, I mean for custom properties.

Comment 15

[techieshark]

I think a great use case for this would be folks creating widgets to embed in other pages. 

As such a user, I'd like to be able to target an element in my embeddable widget and apply the style the browser's originally apply (through the User Agent stylesheet) rather than the styles provided by the CSS spec.  

Doing this 'clears' the styles provided by any style on the embedding webpage, and I could then add the styles I'd normally add when looking at my widget deployed on a blank (browser-styled) page.

(If there's a better idea for covering such a use case I'm all ears, er, eyes. 👀)

Comment 16

[techieshark]

Also, I notice the Status of this just changed 4 days ago in Chromium to "Available". I assume that means the feature is coming to Chrome but I could be wrong (I'm not familiar with their issue tracker).  

https://bugs.chromium.org/p/chromium/issues/detail?id=579788

Comment 17

[Rob Wu [:robwu]]

(In reply to techieshark from comment #16)
> Also, I notice the Status of this just changed 4 days ago in Chromium to
> "Available". I assume that means the feature is coming to Chrome but I could
> be wrong (I'm not familiar with their issue tracker).  

On the contrary; "Available" means that nobody is working on it.
Their statuses are "Untriaged" -> "Available" -> "Assigned" -> "Started" -> "Fixed"

Comment 18

[Jeremiah Lee]

revert would be particularly useful for WebExtensions that use browser.tabs.insertCSS and need to be defensive of element styling. I do hope revert is added.

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Attachment: Draft patch.

I just realized I did a bunch of refactoring over bug 1491620 and co. to implement this and I never looked it again.

This is a draft of how it should look. I don't know why this approach wouldn't work. Probably just a matter of writing tests and filling it the patch, but gotta have lunch first.

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1215878 - Implement CSS revert keyword. r=#style!,heycam!,birtles!

The only fishy bit is the animation stuff. In particular, there are two places
where we just mint the revert behavior:

• When serializing web-animations keyframes (the custom properties stuff in
  declaration_block.rs). That codepath is already not sound and I wanted to
  get rid of it in bug 1501530, but what do I know.

• When getting an animation value from a property declaration. At that point
  we no longer have the CSS rules that apply to the element to compute the
  right revert value handy. It'd also use the wrong style anyway, I think,
  given the way StyleBuilder::for_animation works.

  We could probably get them out of somewhere, but it seems like a whole lot
  of code reinventing the wheel which is probably not useful, and that Blink
  and WebKit just cannot implement either since they don't have a rule tree,
  so it just doesn't seem worth the churn.

The custom properties code looks a bit different in order to minimize hash
lookups in the common case. FWIW, revert for custom properties doesn't seem
very useful either, but oh well.

Comment 21

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1215878 - Optimize cascading of other wide keywords if possible. r=xidorn,heycam

The way the copy-on-write stuff works, and the way that we have to apply
properties from most specific to less specific guarantees that always that we're
going to inherit an inherited property, or reset a reset property, we have
already the right value on the style.

Revert relies on that, so there doesn't seem to be a reason to not use that fact
more often and skip useless work earlier.

Font-size is still special of course... I think I have a way to move the
specialness outside of the style, but piece by piece.

Comment 22

[Emilio Cobos Álvarez [:emilio]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18aad3aca9bdb07765a2de8c40463869dd01e171 for the first patch (the failing devtools test is updated already).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4a268597f9dbc14e8c930c7aa0382352f8a0c1d for the second, which I just pushed but that should be green, if I didn't mess up the font-size specialness.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ae26ce1cf09
Implement CSS revert keyword. r=heycam,birtles

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d33ffb859fa
Optimize cascading of other wide keywords if possible. r=xidorn

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/536782d2357f
Temporarily skip font-family subtest because of bug 1533392.

Comment 26

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1ae26ce1cf09
https://hg.mozilla.org/mozilla-central/rev/0d33ffb859fa

Comment 27

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/536782d2357f

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15759 for changes under testing/web-platform/tests

Comment 29

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 30

[ExE Boss]

Attachment: MDN: Browser compatibility table update

Comment 31

[Chris Mills [:cmills]]

Note to MDN writers:

revert keyword has been give its own page, added to the all page, and mentioned in the Fx 67 rel notes:
https://developer.mozilla.org/en-US/docs/Web/CSS/revert
https://developer.mozilla.org/en-US/docs/Web/CSS/all
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/67#CSS

On the all page, the BCD says it isn't supported, so that needs to be updated.

I also have a sinking feeling that we ought to add this to the list of global property values that appears on EVERY CSS property page ;-|

Comment 32

[Eric Shepherd [:sheppy]]

BCD has been updated by someone in the last few days. I went in and made some changes to the revert article to be more detailed and to ensure that terms were being defined usefully, and I added a glossary entry for the term "style origin"

And yes, this keyword needs to be added to every page that mentions things like unset and such. So yeah. Ugh.

Chris, how do we proceed with that? It should be done relatively soon probably, but it's not realistic to expect it for the 67 sprint we're doing now.

Comment 33

[Mats Palmgren (inactive)]

(In reply to Eric Shepherd [:sheppy] from comment #32)

And yes, this keyword needs to be added to every page that mentions things like unset and such. So yeah. Ugh.

I don't think enumerating these CSS-wide values for every
property adds much value. We could at least shorten
it down to something like:
font-size:<defaulting-keyword>
(which will save time the next time we add a new value ;-))

Comment 34

[Eric Shepherd [:sheppy]]

:mats - Well, I think they should be listed, but we need to set things up so it happens automatically where appropriate instead of having to manually update the content. Heck, it might already do that. I'm not sure. :)

Comment 35

[Chris Mills [:cmills]]

(In reply to Eric Shepherd [:sheppy] from comment #34)

:mats - Well, I think they should be listed, but we need to set things up so it happens automatically where appropriate instead of having to manually update the content. Heck, it might already do that. I'm not sure. :)

I don't think it does. This is one of those "why oh why didn't we use a macro" situations.

Maybe for now you could create a macro that adds the global property values in, then in the next sprint we could add it to the top 100 properties according to the docs maintenance sheet? This gives us about 75-80% of property page views, I'd estimate.

then we can fix the others as we get to them.

Comment 36

[Maciej Jaros]

I'm confused. You say this was implemented but the example on MDN page doesn't work (tested on FF dev and Nightly). I mean I assume the text in the .widget should be black and thin.
https://developer.mozilla.org/en-US/docs/Web/CSS/revert

From devtools I can see that revert does seem to do something, but it seems that the effects are not inherited...

Comment 37

[Maciej Jaros]

Attachment: devtools inheritance problem

The inheritance problem mentioned above. Devtools shows that all: revert overridden properties from section but only on .widget and not on .widget p.

The screen also shows that text in .widget is still blue and bold. Note that using .widget p {all:revert} doesn't help (styles are still applied).

Comment 38

[Emilio Cobos Álvarez [:emilio]]

I think the example (or your expectation of how the example should render) is just wrong. WebKit renders it the same, and it also supports revert.

revert is working as intended in that example (reverting to the previous level of the cascade, preventing other declarations from applying).

Turns out that there's no other declaration for color, and color is an inherited property, so it inherits as usual. I think that example is pretty poor, because it doesn't use the feature at all in any meaningful way. I think what you expected is what you'd get with all: initial.

Comment 39

[Maciej Jaros]

Quote from: https://www.w3.org/TR/css-cascade-4/#valdef-all-revert
"author origin
Rolls back the cascaded value to the user level, so that the specified value is calculated as if no author-level rules were specified for this property on this element."

When adding any CSS to a webpage I'm in the author origin. Hence revert should revert all declarations from author origin (and revert them to user origin).

In user origin color is black. In author origin color is blue. Reverting to user origin should make the color black again.

So I think the MDN example is correct. The implementation is wrong.

Comment 40

[Emilio Cobos Álvarez [:emilio]]

That's not how it works?

Rolls back the cascaded value to the user level, so that the specified value is calculated as if no author-level rules were specified for this property on this element.

This is exactly right, so it's 100% as if no author rules existed.

In user origin color is black. In author origin color is blue. Reverting to user origin should make the color black again.

This is wrong however, there's no user origin declaration (unless you have a user stylesheet), so the value is inherited.

Comment 41

[Chris Mills [:cmills]]

This is wrong however, there's no user origin declaration (unless you have a user stylesheet), so the value is inherited.

I have just tried playing around with revert, including applying a user stylesheet to Firefox and then trying to rollback the styling it applies using revert. But even that doesn't seem to work in the way I'd expect.

Can you give me an idea of an example that would show an effective use of revert, so I can try to correct the reference page to show something more useful? Thanks!

Comment 42

[Emilio Cobos Álvarez [:emilio]]

Revert is pretty similar to unset, except it applies previous origins of the cascade. You can see it in action everywhere there's UA rules.

For example, the following should apply the default margins for an <h1> element:

<!doctype html>
<style>
  h1 {
    margin: 0;
  }
</style>
<h1 style="margin: revert">Should have margins</h1>

It's more useful with display or such, since revert makes it go back to the default display value of the element accounting for user / UA sheets, unlike unset / initial. So <div style="display: revert"> is a block, while with initial / unset it'd be an inline.

Not sure about more practical examples in the web-components space. Maybe Mats or Cameron know?

Comment 43

[Chris Mills [:cmills]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #42)

Revert is pretty similar to unset, except it applies previous origins of the cascade. You can see it in action everywhere there's UA rules.

For example, the following should apply the default margins for an <h1> element:

<!doctype html>
<style>
  h1 {
    margin: 0;
  }
</style>
<h1 style="margin: revert">Should have margins</h1>

It's more useful with display or such, since revert makes it go back to the default display value of the element accounting for user / UA sheets, unlike unset / initial. So <div style="display: revert"> is a block, while with initial / unset it'd be an inline.

Not sure about more practical examples in the web-components space. Maybe Mats or Cameron know?

OK, thanks, I get this now.

For the moment, I've just added something based on your example here to the revert page, so that it isn't wrong. I had a bit of a play with using it in web components, but ran out of time.

Comment 44

[Maciej Jaros]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #40)

That's not how it works?

Rolls back the cascaded value to the user level, so that the specified value is calculated as if no author-level rules were specified for this property on this element.

This is exactly right, so it's 100% as if no author rules existed.

No. That is not how it works. color: blue in the example is in the author origin and it is not reverted.

In user origin color is black. In author origin color is blue. Reverting to user origin should make the color black again.

This is wrong however, there's no user origin declaration (unless you have a user stylesheet), so the value is inherited.

Inherited from what? When I revert It can only be inherited from user origin and from browser origin. Neither of them define color: blue.

Another question -- why is color declaration striked out if it still applies in the .widget?

Please have another look the screen from comment #37. I really think there is something wrong.

The standard was changes recently. Maybe the implementation reflect previous version of the upcoming standard? See:
https://www.w3.org/TR/css-cascade-4/#valdef-all-revert

(In reply to Emilio Cobos Álvarez (:emilio) from comment #42)

Revert is pretty similar to unset, except it applies previous origins of the cascade. You can see it in action everywhere there's UA rules.

For example, the following should apply the default margins for an <h1> element:

<!doctype html>
<style>
  h1 {
    margin: 0;
  }
</style>
<h1 style="margin: revert">Should have margins</h1>

Isin't <style> and style="..." both in the same origin? Seems to me that they are both in the author origin. Same as two declarations in <style> would be. So in terms of origin there is no difference in the example from MDN and to your example.

Note that the updated spec says:

Rolls back the cascaded value to the user level, so that the specified value is calculated as if no author-level rules were specified for this property on this element.

Comment 45

[Emilio Cobos Álvarez [:emilio]]

No. That is not how it works. color: blue in the example is in the author origin and it is not reverted.

Because color: blue isn't a declaration applying to the .widget, it's inherited from the parent (section). If you change the selector in that test-case to be .widget instead of section, then it wouldn't become blue.

Another question -- why is color declaration striked out if it still applies in the .widget?

That's probably a devtools issue of not understanding the revert keyword well enough. Probably happens with unset as well.

Isin't <style> and style="..." both in the same origin? Seems to me that they are both in the author origin. Same as two declarations in <style> would be. So in terms of origin there is no difference in the example from MDN and to your example.

Right, the difference is that margin is not inherited, it applies to the h1 directly, and thus gets reversed. In the old MDN example, the color was inherited from the parent <section>.

Comment 46

[Emilio Cobos Álvarez [:emilio]]

I filed bug 1552949 to consider making devtools understand css-wide-keywords so that declarations don't appear crossed-out.

Comment 47

[Maciej Jaros]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #45)

No. That is not how it works. color: blue in the example is in the author origin and it is not reverted.

Because color: blue isn't a declaration applying to the .widget, it's inherited from the parent (section). If you change the selector in that test-case to be .widget instead of section, then it wouldn't become blue.

Another question -- why is color declaration striked out if it still applies in the .widget?

That's probably a devtools issue of not understanding the revert keyword well enough. Probably happens with unset as well.

Isin't <style> and style="..." both in the same origin? Seems to me that they are both in the author origin. Same as two declarations in <style> would be. So in terms of origin there is no difference in the example from MDN and to your example.

Right, the difference is that margin is not inherited, it applies to the h1 directly, and thus gets reversed. In the old MDN example, the color was inherited from the parent <section>.

I think I understand what you are trying to say... But aren't you describing unset? I can't think of an example were unset and revert would behave differently. Cold you provide some example? From what I understand on author level unset and revert should behave differently. Or am I reading it wrong?

Here are some things I tried:
https://codepen.io/eccenux/pen/joGaMp

Comment 48

[Emilio Cobos Álvarez [:emilio]]

<div style="display: revert"> computes to block. <div style="display: unset"> computes to inline.

Comment 49

[Maciej Jaros]

OK. Sorry about that. It does seem fine.

Added more examples on MDN and I hope I clarified the description as well.
https://developer.mozilla.org/en-US/docs/Web/CSS/revert

Originally I just didn't got the part about revert being applied only to the currently selected element. So it's just a value and not some magic directive that resets everything. So not really for widgets (as some seemed to hope here).

Comment 50

[Emilio Cobos Álvarez [:emilio]]

Just went through the examples on MDN and they look great, thanks! :)