1338158 - Use the 'unset' value more in UA sheets

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

Follow-up from bug 605985 comment 65.

dholbert noted that we have UA style sheet rules that first sets values like so:

input {
...
  padding: 1px;
...
}

and later "unsets" them for some elements, like for example:

input[type="radio"],
input[type="checkbox"] {
...
  padding: 0;
...
}

(this example from layout/style/res/forms.css)

The intent would be clearer (and the parsing slightly faster?) if we use
the 'unset' value instead:

input[type="radio"],
input[type="checkbox"] {
...
  padding: unset;
...
}


Watch out for inherited properties though.  We probably want to leave those
as is or possibly use 'initial'.  E.g. "cursor:text" for 'input' is later reset
to "cursor:default" in the radio/checkbox rule.  Changing that to "cursor:unset"
would be a functional change.  This bug does not intend to do any functional
changes just make the intent clearer were possible.

We should audit all style sheets in layout/style/res/

Comment 1

[Daniel Holbert [:dholbert]]

Yeah, the idea here is:
 A) For all decls that assign not-inherited-by-default properties to "initial" or to their longform initial value like "padding:0":  replace the value with "unset"
 B) For all decls that assign *inherited-by-default* properties to "inherit": replace "inherit" with "unset"
 C) (And in all of these cases, we should perhaps verify that the property is *actually being set* somewhere earlier in the stylesheet -- i.e. that there's actually something that we need to unset)

This makes the intent more clear -- it makes it less mysterious why we are bothering with these styles to the values that they'd normally have anyway.

Comment 2

[Preeti[:preeti]]

Hello everyone! This is Preeti and I have just  recently started contributing to open source. Could someone please guide me on how to proceed on this bug? Thanks in advance!
From what I could comprehend from the above is that I need to reassign the variables in all the  files in the folder layout/style/res/  which were previously assigned as inherit with unset?

Comment 3

[Ananya]

Hi,
This is Ananya, I am new here.
Can someone guide me on how to start contributing towards this bug? 
Thanks!

Comment 4

[Kamalesh Palanisamy]

Hi , 
My name is kamalesh. I am trying to contribute to open source can you help me with working on this bug.

Comment 5

[abhieshekumar]

Hello,
I would like to contribute to this bug. Can someone guide on where to begin ?
Thank you

Comment 6

[Mats Palmgren (inactive)]

I think the first two comments in this bug describes it clearly.

Comment 7

[Yatri]

Hello,

I would like to contribute by taking this as my first issue for my contribution in Outreachy.

Thanks.

Comment 8

[Yatri]

@svoisen @mats Can you assign this issue to me?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Yatri: FWIW, you can submit a patch without being assigned to the bug, you shouldn't wait to get assigned to submit the patch :)

Comment 10

[Yatri]

Attachment: Bug 1338158: use 'unset' value more in UA stylesheets, in cases where we're already just resetting the property

Fixing Bug: 1338158 by using 'unset' value in css sheets

Comment 11

[Yatri]

Attachment: Bug 1338158: use 'unset' value more in UA stylesheets, in cases where we're already just resetting the property

Fixing Bug: 1338158 by using 'unset' value in css sheets

Comment 12

[Daniel Holbert [:dholbert]]

It looks like you've created a second commit (or at least, a separate phabricator revision) to address one of the review comments.

You should make those changes in the original phabricator revision, though (in https://phabricator.services.mozilla.com/D6516 ), like you did when you made fixes/updates before.

Comment 13

[Yatri]

@dholbert I followed the same procedure but I don't know what happened that it made a second revision.

Comment 14

[Yatri]

Attachment: Bug 1338158: use 'unset' value more in UA stylesheets, in cases where we're already just resetting the property

Uses 'unset' wherever the property is inherited ny itself

Comment 15

[Daniel Holbert [:dholbert]]

Let me know when you'd like me to take another look. Right now it looks like it's in a weird two-part state, which still needs to be fixed before this can be reviewed/landed.

(I don't think the real patch is in either part, actually -- both [1][2] of the non-obsolete attachments here seem to only be a semicolon-before-!important removal as the only change in the patch.)

BTW, you can get back the original version of the patch by using the "History" button on D6516 - that shows the various versions of that revision, through time.


[1] https://phabricator.services.mozilla.com/D6516
[2] https://phabricator.services.mozilla.com/D6535

Comment 16

[Yatri]

@dholbert I'm not able to push my diff file in the phabricator as it shows an exception error while I use `arc diff --update D6516`

Comment 17

[Yatri]

Can someone help me with this issue of having 2 revisions and not being able to submit a new diff file

Comment 18

[Yatri]

Linting...
No lint engine configured for this project.
Running unit tests...
 Exception 
Command failed with error #255!
COMMAND
HGPLAIN=1 hg diff --git -U32767 --rev '' -- ''

STDOUT
(empty)

STDERR
hg: parse error: empty query


I'm getting this while trysing to send a new diff file

Comment 19

[Daniel Holbert [:dholbert]]

Sorry -- I'm not familiar with that error, and I'm not sure it'd be possible (or at least time-effective) for me to remotely diagnose what went wrong.  (I do empathize - I've definitely gotten my hg repo into a confusing/broken state at times in the past, and the "arc" overhead adds an extra layer of complexity/possible-system-failure.)

The good news, though, is that I can reconstruct a complete patch based on what you've posted (by applying the two parts that we had here earlier).

I think that's the most time-effective way forward at this point, so I'll just do that and push it to mozilla-inbound (our mercurial server for manually landing patches by hand).

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: patch, reconstructed from pieces in phabricator

Comment 21

[Daniel Holbert [:dholbert]]

Pushed to our Try Server to get a sanity-check test run before this lands:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=482b8c796508cb84b35020c5446dac4f81fe97f0

--> Setting needinfo to remind myself to land this assuming it passes tests on Try.

Comment 22

[Yatri]

@dholbert Sure thanks for this! Can you help me with cleaning my whole hg repo so that this doesn't affect the next patches I submit?

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Yatri from comment #22)
> @dholbert Sure thanks for this! Can you help me with cleaning my whole hg
> repo so that this doesn't affect the next patches I submit?

If all else fails, the most complete option is just to delete and re-clone. :)

But you should be able to use "hg out" to see your own "outgoing" commits, and then you can "hg prune" each one.  (For "hg prune", you have to enable the "evolve" extension, see https://www.mercurial-scm.org/wiki/EvolveExtension#Setup and documentation at https://www.mercurial-scm.org/wiki/ChangesetEvolution )

(You could also remove these commits with "hg strip", but that's a bit more destructive, and you may have to run it several times if you've used "amend" to amend the commits.)

These commands all have documentation available if you run them with "--help".  (Though again, "prune" isn't available until you enable the evolve extension.)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04091914475
use 'unset' value more in UA stylesheets, in cases where we're already just resetting the property. r=dholbert

Comment 25

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/e04091914475