-
Notifications
You must be signed in to change notification settings - Fork 329
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
Fix details expanded state not announced on iOS #5089
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 00371fa |
Stylesheets changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
index 0d72bd2ab..1b097f1c9 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
@@ -2973,7 +2973,7 @@ screen and (forced-colors:active) {
}
.govuk-details__summary {
- display: inline-block
+ display: block
}
.govuk-details[open] .govuk-details__summary {
@@ -3029,6 +3029,8 @@ screen and (forced-colors:active) {
@supports not (-ms-ime-align:auto) {
.govuk-details__summary {
position: relative;
+ width: -webkit-fit-content;
+ width: fit-content;
padding-left: 25px;
color: #1d70b8;
cursor: pointer
Action run for 00371fa |
Other changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/components/details/_index.scss b/packages/govuk-frontend/dist/govuk/components/details/_index.scss
index 0a500c62f..f349edc0c 100644
--- a/packages/govuk-frontend/dist/govuk/components/details/_index.scss
+++ b/packages/govuk-frontend/dist/govuk/components/details/_index.scss
@@ -8,8 +8,7 @@
}
.govuk-details__summary {
- // Make the focus outline shrink-wrap the text content of the summary
- display: inline-block;
+ display: block;
}
.govuk-details[open] .govuk-details__summary {
@@ -74,6 +73,10 @@
// Absolutely position the marker against this element
position: relative;
+ // Make the focus outline shrink-wrap the text content of the summary
+ width: -webkit-fit-content;
+ width: fit-content;
+
// Allow for absolutely positioned marker and align with disclosed text
padding-left: govuk-spacing(4) + $govuk-border-width;
Action run for 00371fa |
Spreadsheet for testing: So far I've not observed any changes other than in Safari on iOS 17.5.1 (21F90) where we see the desired improvement, with the expanded state announced on toggle and when returning to the summary:
I don't have access to test with TalkBack on Android – it'd be fab if someone else was able to help with that. |
9a75836
to
8cdeef4
Compare
I've had a quick look at the margin issue mentioned in the description and I'm going to make the bold case that this isn't worth worrying about. As Ollie mentions, Safari <13, so 12 and lower, for both mac and iOS will functionally have 5px less margin. You can see this illustrated in the below screenshot of Safari mac 12.1 where the margin collapses into the focus state bottom border: Safari 11 and 12 fall under Grade C in our browser support model which states the following about bugs in those browsers:
I'm using this policy and the low impact to visual rhythm for Safari 12 and below users to justify not implementing a fix for this. |
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.
Code changes look good to me. If I understood correctly, the margin issue is that we now have 30px of margin in browsers that don't support flow-root
rather than 35px?
Given the large margin of 30px and the limited browsers it applies to, I think it'd be fine, but it would be worth confirming with a designer.
Another thought on the matter is whether the margins not collapsing was intentional or accidental, in the first place. Looks like the summary requiring 5px of margin is really for when the detail is open, so there's some space between it and the grey bar, is it 'leaking' when the detail is closed? If that's the case we may want to condition that to [open] .govuk-details__summary
or add extra margin-top
/padding-top
to the govuk-details__text
, which would both clarify the reason for that extra space. We could look into that in a separate small story, though 😊
I definitely think its worth fixing for all versions of the browser, but to answer to main question yes the answer sounds right to be, scrapping the extra 5px sounds like it should work. Without looking at the reasoning for it, I can’t really see why it would be there in the first place. |
Added a PR to tackle the spacing of the details element separately #5352 |
741546e
to
fd0e290
Compare
For some reason, setting `display: inline-block` on the summary means that VoiceOver on iOS no longer announces the expanded state of the `<details>` element [1]. We were using `display: inline-block` so that the interactive area and the focus state (when visible) were constrained to the text within the `<summary>`. We can achieve the same thing by using `display: block` with `width: fit-content`. [1]: https://bugs.webkit.org/show_bug.cgi?id=230408
fd0e290
to
00371fa
Compare
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.
Romaric had planned to approve this but he ran out of time yesterday and it's now his non-working day. Outside of github he's commented that he's happy with this change and he's given me permission to approve and merge this 🎉
Just to give a bit more context that I had shared with the team internally last week... I have tested this in other iOS versions, and I've also tested the native version. Results from that are in the spreadsheet. VoiceOver iOS 17.5.1Both the native version and the version from this PR work as expected. VoiceOver iOS 15.8None of the versions work. VoiceOver iOS 12.5.7Interestingly, the native version as well as the version from this PR worked okay. Other iOS versionsI don't have access to other iOS versions. |
For some reason, setting
display: inline-block
on the summary means that VoiceOver on iOS no longer announces the expanded state of the<details>
element.We were using
display: inline-block
so that the interactive area and the focus state (when visible) were constrained to the text within the<summary>
.We can achieve the same thing by using
display: block
withwidth: fit-content
.However, now that we’re no longer using
display: inline-block
1, the the 5px bottom margin on the summary and the 30px bottom margin on the details now collapse when the<details>
is closed except in Chrome which has been updated to usecontent-visibility
.Preserve the existing margin on the component and make the behaviour across browsers consistent by establishing a new block formatting context using
display: flow-root
, preventing the margins from collapsing.Safari < 13 does not support
display: flow-root
and so the margins will collapse, which means these older versions of Safari will have 5px less margin than when we were still usingdisplay: inline-block
.EDIT: We've dropped the commit that added
display: flow-root
to the rootdetails
element as we removed the extra spacing in #5352, bypassing this issue. Therefore the previous 3 paragraphs are out of date. I'm preserving Ollie's description if we ever wanted to bring the spacing back in.Fixes #2349
Fixes #4972 (whilst this PR doesn't address 4972 directly, we did explore this option and it unfortuantely doesn't make a difference. This solution has been our most successful so far)
Footnotes
‘margins of inline-block boxes do not collapse (not even with their in-flow children)’ – https://www.w3.org/TR/CSS21/box.html#collapsing-margins ↩