Remove webkit-inner-spin-button UA styles for temporal inputs and guard its usage. [chromium/src : main]
Luke (Gerrit)
Message from Luke![]( "Open in Gerrit")
Set Ready For Review
Related details
- Luke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luke (Gerrit)
Luke added 1 comment![]( "Open in Gerrit")
LukeWaiting for https://chromium-review.googlesource.com/c/chromium/src/+/7738961
Done
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 3 comments![]( "Open in Gerrit")
This pseudo hasn't been used for temporal inputs since the formBut these are web-exposed, right? So there are compat implications of doing this. I think this needs at least a PSA, and likely some use counters (or other intuition) about why this isn't a compat problem.
CHECK(!RuntimeEnabledFeatures::
HTMLInputElementTemporalDropWebkitSpinButtonEnabled());I don't think you can do this right? I.e. this patch just removes those pseudo elements from the UA stylesheet, but if a developer stylesheet uses them, you'll hit many of these `CHECK`s, right? I.e. this will show the spin buttons always:
```
input::-webkit-inner-spin-button {
opacity: 1;
}
```
I'm realizing now that the same comments might also apply to https://chromium-review.git.corp.google.com/c/chromium/src/+/7738961, right? Though there wasn't an actual populated clear button in that case, so maybe not.
name: "HTMLInputElementTemporalDropWebkitSpinButton",Generally, it's an anti-pattern to have "negative" features. It's too hard to follow what `DropEnabled` means - is it dropped or enabled?
So ideally you make this `HTMLInputElementTemporalWebkitSpinButton` and make the default case (if you're trying to ship this now) have no `status: ""` line at all. Then the killswitch is to enabled it, if things go bad.
Related details
- Luke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luke (Gerrit)
Luke added 3 comments![]( "Open in Gerrit")
This pseudo hasn't been used for temporal inputs since the formBut these are web-exposed, right? So there are compat implications of doing this. I think this needs at least a PSA, and likely some use counters (or other intuition) about why this isn't a compat problem.
This pseudo does still exist and get used but *specifically* for number inputs. NOT temporal inputs.
CHECK(!RuntimeEnabledFeatures::
HTMLInputElementTemporalDropWebkitSpinButtonEnabled());I don't think you can do this right? I.e. this patch just removes those pseudo elements from the UA stylesheet, but if a developer stylesheet uses them, you'll hit many of these `CHECK`s, right? I.e. this will show the spin buttons always:
```
input::-webkit-inner-spin-button {
opacity: 1;
}
```I'm realizing now that the same comments might also apply to https://chromium-review.git.corp.google.com/c/chromium/src/+/7738961, right? Though there wasn't an actual populated clear button in that case, so maybe not.
Like mentioned above this won't show the spin button for temporal inputs, only number inputs which I haven't touched in this change.
The other change also doesn't make use of the clear button so it's not an issue either.
name: "HTMLInputElementTemporalDropWebkitSpinButton",Generally, it's an anti-pattern to have "negative" features. It's too hard to follow what `DropEnabled` means - is it dropped or enabled?
So ideally you make this `HTMLInputElementTemporalWebkitSpinButton` and make the default case (if you're trying to ship this now) have no `status: ""` line at all. Then the killswitch is to enabled it, if things go bad.
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 3 comments![]( "Open in Gerrit")
Ok, just the feature inversion needed here, I think.
This pseudo hasn't been used for temporal inputs since the formLukeBut these are web-exposed, right? So there are compat implications of doing this. I think this needs at least a PSA, and likely some use counters (or other intuition) about why this isn't a compat problem.
This pseudo does still exist and get used but *specifically* for number inputs. NOT temporal inputs.
Ok, right, not for temporal, just for number. Thanks, and sorry.
CHECK(!RuntimeEnabledFeatures::
HTMLInputElementTemporalDropWebkitSpinButtonEnabled());LukeI don't think you can do this right? I.e. this patch just removes those pseudo elements from the UA stylesheet, but if a developer stylesheet uses them, you'll hit many of these `CHECK`s, right? I.e. this will show the spin buttons always:
```
input::-webkit-inner-spin-button {
opacity: 1;
}
```I'm realizing now that the same comments might also apply to https://chromium-review.git.corp.google.com/c/chromium/src/+/7738961, right? Though there wasn't an actual populated clear button in that case, so maybe not.
Like mentioned above this won't show the spin button for temporal inputs, only number inputs which I haven't touched in this change.
The other change also doesn't make use of the clear button so it's not an issue either.
- Luke
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |