report: tweak summary marker styles #12267
Conversation
| } | ||
|
|
||
| /* Older webkit browsers might have -webkit-details-marker, | ||
| and require setting list style type to none. */ | ||
| .lh-audit-group > summary, |
There was a problem hiding this comment.
would have used supports not selector(...) but safari doesn't support that.
| @@ -541,8 +541,19 @@ | |||
| background-color: var(--color-hover); | |||
| } | |||
|
|
|||
| /* Hide the expandable arrow icon, three ways: via the CSS Counter Styles spec, for webkit/blink browsers, hiding the polyfilled icon */ | |||
There was a problem hiding this comment.
link is dead, i found the old sass code but it didn't seem helpful to have here
comment confused me so I just removed. maybe someone could help me reword? we aren't use @counter-styles (whatever that is), and I'm don't know what "polyfilled icon" is referring to.
There was a problem hiding this comment.
counter styles spec was .... incorrect.
"polyfilled icon" means the CSS added by deatils-element-polyfill (which is used in PSI for... old edge users and IE11 users?)
we can remove that polyfill now, tbh.
| @@ -541,8 +541,19 @@ | |||
| background-color: var(--color-hover); | |||
| } | |||
|
|
|||
| /* Hide the expandable arrow icon, three ways: via the CSS Counter Styles spec, for webkit/blink browsers, hiding the polyfilled icon */ | |||
| /* https://github.com/javan/details-element-polyfill/blob/master/src/details-element-polyfill/polyfill.sass */ | |||
| /* We want to hide the browser's default arrow marker on summary elements. */ | |||
There was a problem hiding this comment.
i rewrote this to be a little simpler. (and document why we do each thing)
/* We want to hide the browser's default arrow marker on summary elements. Admittedly, it's complicated. */
.lh-audit-group > summary,
.lh-expandable-details > summary {
/* Blink 89+ and Firefox will hide the arrow when display is changed from (new) default of `list-item` to block. https://chromestatus.com/feature/6730096436051968*/
display: block;
}
/* Safari and Blink <=88 require using the -webkit-details-marker selector */
.lh-audit-group > summary::-webkit-details-marker,
.lh-expandable-details > summary::-webkit-details-marker {
display: none;
}
/* When the details-element-polyfill is used, it adds an ugly arrow. Hide it. https://github.com/javan/details-element-polyfill/blob/main/src/styles.js */
.lh-audit-group > summary:before,
.lh-expandable-details > summary:before {
display: none;
}There was a problem hiding this comment.
I tweaked my comments a bit, but IMO it was already specifying why things were being done at an appropriate level of detail (new/old browsers, tale as old as time). The provided links provides more detail (like what version of blink the newer stuff is supported).
got rid of the @supports block too, the idea there was to eventually have chrome not emit a deprecation warning if it was smart and checked for these rules in a better way; but the deprecation is now removed, so whatever.
There was a problem hiding this comment.
I'm unclear why this has to be contentious..
I tweaked my comments a bit, but IMO it was already specifying why things were being done at an appropriate level of detail
if that were the case, then you would have deleted the list-style-type: none; rule. it's totally unnecessary.
| @@ -541,8 +541,19 @@ | |||
| background-color: var(--color-hover); | |||
| } | |||
|
|
|||
| /* Hide the expandable arrow icon, three ways: via the CSS Counter Styles spec, for webkit/blink browsers, hiding the polyfilled icon */ | |||
There was a problem hiding this comment.
counter styles spec was .... incorrect.
"polyfilled icon" means the CSS added by deatils-element-polyfill (which is used in PSI for... old edge users and IE11 users?)
we can remove that polyfill now, tbh.
connorjclark
changed the title
paulirish
changed the title
related: #12089
Latest Chrome decides to emit a deprecation warning for
::-webkit-details-marker(it probably shouldn't...). this doesn't actually avoid that, but I thought we should use the new::markerjust cuz.still works in FF/Safari/Chrome. actually, nothing really changes :) this will be eventually necessary, I guess Chrome will remove old support one day?