Use the 'unset' value more in UA sheets

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: mats, Assigned: yatripatel.169)

Tracking

({good-first-bug})

Trunk
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

(Whiteboard: [lang=css])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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/
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.
Priority: -- → P3

Comment 2

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

7 months ago
Hi,
This is Ananya, I am new here.
Can someone guide me on how to start contributing towards this bug? 
Thanks!
Hi , 
My name is kamalesh. I am trying to contribute to open source can you help me with working on this bug.
Flags: needinfo?(mats)

Comment 5

7 months ago
Hello,
I would like to contribute to this bug. Can someone guide on where to begin ?
Thank you
(Reporter)

Comment 6

7 months ago
I think the first two comments in this bug describes it clearly.
Flags: needinfo?(mats)
(Assignee)

Comment 7

7 months ago
Hello,

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

Thanks.
(Assignee)

Comment 8

7 months ago
@svoisen @mats Can you assign this issue to me?
Yatri: FWIW, you can submit a patch without being assigned to the bug, you shouldn't wait to get assigned to submit the patch :)
Assignee: nobody → yatripatel.169
(Assignee)

Updated

7 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 10

7 months ago
Fixing Bug: 1338158 by using 'unset' value in css sheets
Attachment #9011012 - Attachment description: Fixes Bug: 1338158 by using 'unset' value in UA sheets → Bug 1338158: use 'unset' value more in UA stylesheets, in cases where we're already just resetting the property
(Assignee)

Comment 11

7 months ago
Fixing Bug: 1338158 by using 'unset' value in css sheets
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.
Flags: needinfo?(yatripatel.169)
(Assignee)

Comment 13

7 months ago
@dholbert I followed the same procedure but I don't know what happened that it made a second revision.
Flags: needinfo?(yatripatel.169)
Attachment #9011012 - Attachment is obsolete: true
Attachment #9011111 - Attachment is obsolete: true
(Assignee)

Comment 14

7 months ago
Uses 'unset' wherever the property is inherited ny itself
Attachment #9011012 - Attachment is obsolete: false
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
Flags: needinfo?(yatripatel.169)
(Assignee)

Comment 16

7 months ago
@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`
Flags: needinfo?(yatripatel.169)
(Assignee)

Comment 17

7 months ago
Can someone help me with this issue of having 2 revisions and not being able to submit a new diff file
Attachment #9011119 - Attachment is obsolete: true
Attachment #9011012 - Attachment is obsolete: true
(Assignee)

Comment 18

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

Updated

7 months ago
Flags: needinfo?(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).
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.
Flags: needinfo?(dholbert)
(Assignee)

Comment 22

7 months ago
@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?
(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

7 months ago
Pushed by dholbert@mozilla.com:
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
Flags: needinfo?(dholbert)

Comment 25

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e04091914475
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.