Status

()

enhancement
P3
normal
RESOLVED FIXED
4 years ago
18 days ago

People

(Reporter: heycam, Assigned: emilio)

Tracking

(Blocks 2 bugs, {dev-doc-needed, parity-safari})

unspecified
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

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]

Comment 3

3 years ago
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

3 years ago
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

2 years ago
(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

2 years ago
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]

Updated

7 months ago
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)
(Assignee)

Comment 9

7 months ago
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)
(Assignee)

Comment 10

7 months ago
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.
(Assignee)

Comment 11

7 months ago
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.
(Assignee)

Comment 12

7 months ago
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.
(Assignee)

Updated

7 months ago
Depends on: 1491620

Comment 13

7 months ago
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
(Assignee)

Comment 14

7 months ago
Sure, I mean for custom properties.
Flags: needinfo?(cam)

Comment 15

4 months ago
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

4 months ago
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

4 months ago
(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

2 months ago

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.

(Assignee)

Comment 19

2 months ago
Posted 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)

Updated

2 months ago
Assignee: nobody → emilio
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Updated

2 months ago
Attachment #9048032 - Attachment is obsolete: true
(Assignee)

Comment 20

2 months ago

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.

(Assignee)

Comment 21

2 months ago

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.

(Assignee)

Comment 22

2 months ago

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.

(Assignee)

Updated

a month ago
Blocks: 1533327

Comment 23

a month ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ae26ce1cf09
Implement CSS revert keyword. r=heycam,birtles

Comment 24

a month ago
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

Comment 25

a month ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/536782d2357f
Temporarily skip font-family subtest because of bug 1533392.

Comment 26

a month ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
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
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.