Closed Bug 1215878 Opened 4 years ago Closed 1 year ago

implement revert keyword

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: heycam, Assigned: emilio)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: dev-doc-complete, parity-safari)

Attachments

(4 files, 1 obsolete file)

Bug 1215878 - Implement CSS revert keyword. r=#style!,heycam!,birtles!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1215878 - Optimize cascading of other wide keywords if possible. r=xidorn,heycam
47 bytes, text/x-phabricator-request
Details | Review
MDN: Browser compatibility table update
52 bytes, text/x-github-pull-request
Details | Review
devtools inheritance problem
579.84 KB, image/png
Details 
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.
Whiteboard: [parity-safari]
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.
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 :-)
(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`.
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.
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-safari
Whiteboard: [parity-safari]
See Also: → 1488711
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)
Flags: needinfo?(emilio)
Flags: needinfo?(cam)
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... :)
Flags: needinfo?(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.
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.
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.
Depends on: 1491620
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
Sure, I mean for custom properties.
Flags: needinfo?(cam)
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. 👀)
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
(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"

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.

Attached patch Draft patch. (obsolete) — — Splinter Review

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.

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Priority: -- → P3
Attachment #9048032 - Attachment is obsolete: true

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.

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.

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.

Blocks: 1533327
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ae26ce1cf09
Implement CSS revert keyword. r=heycam,birtles
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d33ffb859fa
Optimize cascading of other wide keywords if possible. r=xidorn
Depends on: 1533392
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/536782d2357f
Temporarily skip font-family subtest because of bug 1533392.

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Depends on: 1533654
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15759 for changes under testing/web-platform/tests

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 ;-|

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.

Flags: needinfo?(cmills)
Keywords: dev-doc-needed → dev-doc-complete

(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 ;-))

: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. :)

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

Flags: needinfo?(cmills)

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

Flags: needinfo?(svoisen)

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

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.

Flags: needinfo?(svoisen)

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.

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.

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!

Flags: needinfo?(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?

Flags: needinfo?(emilio)

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

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

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 filed bug 1552949 to consider making devtools understand css-wide-keywords so that declarations don't appear crossed-out.

(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

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

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

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

You need to log in before you can comment on or make changes to this bug.