-
Notifications
You must be signed in to change notification settings - Fork 334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Style details in older browsers #3758
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, code wise!
I do find it a bit weird visually, however, as on non-<details>
browsers the summary now looks like regular prose but reads like it's meant to be akin to a section heading.
I know it isn't a section heading (or, at least, we don't want it to be one semantically), but the lack of any visual difference is a bit jarring. Should we get a designer to give this a look over?
|
||
// Absolutely position the marker against this element | ||
position: relative; | ||
|
||
margin-bottom: govuk-spacing(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This margin is being ignored in EdgeHTML Edge. It seems like you need to maintain the display: inline-block
for this to be applied correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style appears in the inspector's style and computed tabs, but if you hover over the element so that the box model is highlighted on the page you can see that the margin-bottom
value hasn't actually been used, even though it's not crossed out.
The difference can also be seen if you do a side-by-side comparison with the existing review app—the border of the <details>
contents has moved up by 5 pixels.
I'm not sure why EdgeHTML devtools says the style is being used when it actually isn't, but I'm gonna chalk that up to EdgeHTML devtools maybe not being amazing. 😝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aah, nice spot! My basic trust in devtools is forever tainted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi thanks for sharing this! I'm wondering if it might look a bit more together if the summary text was made visually part of the same block: That makes it appear basically like an inset text with a heading (I used It does look slightly different to an inset text, because we use different border widths. But maybe that's a problem for another day! What do you think? |
Moving back to draft while I work on these notes. |
857bc20
to
a5f2bf6
Compare
6209787
to
43cf555
Compare
a5f2bf6
to
484aed0
Compare
EdgeHTML is the only browser engine that supports `-ms-ime-align`, so we can check Edge 12 - 18 by applying this property. https://browserstrangeness.github.io/css_hacks.html We can target Edge 16 - 18 more specifically, but keeping it simple for now. This approach just wraps almost everything in a check to see the browser DOESN'T support `-ms-ime-align`, but separates some spacing and border styles to make things look tolerable.
I've had a look at our supported browsers and Opera Mini, and the components behaves as it should. In older browsers, the "inset-text" fallback looks good and matches the inset text component. Beyond Opera Mini, the gaps only come in when we start looking at 0.01% coverage and below, with Firefox 44, 43 and 36 displaying the "linkified" details summary, but not supporting details. I looked into using the new supported class via |
Another set of browsers I hadn't considered before simply because they're all from 2014 and earlier and I hadn't tested that far back. Browsers that support the Currently not exactly sure how to target these using only CSS, or whether it's worth the effort (given that these browsers represent less than 0.1% of GOV.UK visits). |
I've pushed a change to default to browser native styles. This is to cover browsers which support IE 8 - 11 get styled like inset text via a hacky selector. Here's a table summarising what happens:
|
@@ -1,5 +1,49 @@ | |||
## Implementation notes | |||
|
|||
### Styling in older browsers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
Adding to this PR a conversation we had elsewhere: we're comfortable with this impacting 0.18% of browsers as the interaction will break in a way that won't prevent users completing their task. The link won't work on clicking it, but text is still visible. @domoscargin also mentioned that 0.18% is estimated use across the UK, which translates to 0.004% on GOV.UK. |
Adding another conversation from elsewhere: We've kept the IE hack in because of the inertia of support for IE11, but we don't believe it'd be breaking to remove those styles to trim down our CSS in the future when IE11 support is even lower - it'd look a bit worse, but still be fully functional. |
Part of #3464
What
We're removing the JavaScript polyfill for the
<details>
element.We need to ensure that the component is not styled to suggest it can be expanded in browsers that do not natively support the
<details>
element.Browser support for the
<details>
component: https://caniuse.com/?search=detailsWrap "link-like" styles in a CSS Feature Query
We can wrap most of the Details styles in a feature query which excludes EdgeHTML:
This covers the vast majority of browsers (about 97%), which behave as expected.
Hack for Internet Explorer
IE doesn't support the
<details>
element, so we can style it to look like inset text.Internet Explorer 11
Default to browser-native styles
For browsers which support
<details>
, but not CSS feature queries, we simply default to their styles for the<details>
element, plus some added font and spacing.Chrome 27
Android 4.1 (emulated)
Gaps
Browsers which support feature queries, but not
<details>
or es6 module will still have "linkified" styling, but not be interactable. This largely applies to Opera Mini and early Firefox.https://browsersl.ist/#q=supports+css-featurequeries+and+not+supports+details+and+not+supports+es6-module®ion=GB
This represents around 0.18% of browsers in the UK, and is even more minimal on the latest stats I have for GOV.UK (where the affected Firefox versions account for about 0.001% of traffic).
Firefox 48