Fix multi-line link accessibility paths#561
Conversation
meherkasam-square
force-pushed
the
meher/text-links-2
branch
2 times, most recently
from
82fcb49 to
4b6728f
Compare
|
Can you base this PR off of |
meherkasam-square
force-pushed
the
meher/text-links-2
branch
from
4b6728f to
7cf350a
Compare
meherkasam-square
force-pushed
the
meher/text-links-2
branch
2 times, most recently
from
3c08978 to
0f68690
Compare
soroushsq
left a comment
soroushsq
left a comment
There was a problem hiding this comment.
Looks ok to me, would love for @RoyalPineapple to take a look as well
| hasher.combine(firstRect.origin.x) | ||
| hasher.combine(firstRect.origin.y) | ||
| hasher.combine(firstRect.size.width) | ||
| hasher.combine(firstRect.size.height) |
There was a problem hiding this comment.
So annoying that CGRect isn't Hashable, what's that about
There was a problem hiding this comment.
Seriously! I was originally thinking of making an extension but I realised that I would be pushing that extension to every downstream dependency and potentially cause build errors.
| let path: UIBezierPath? = { | ||
| // In cases where a link overflows from one line to the next, | ||
| // we will get back more than one rect. If we had used the boundingRect | ||
| // function instead, it will only return us a bounding rect that can | ||
| // fully encompass all the sub-rects - this is undesired behavior as | ||
| // it will suppress focusable items within the label that have an | ||
| // overlap with the larger bounding box. | ||
| guard rects.count > 1 else { return nil } | ||
|
|
||
| let cgPath = CGMutablePath() | ||
| let cornerRadius: CGFloat = 4.0 | ||
|
|
||
| for rect in rects { | ||
| cgPath.addRoundedRect(in: rect, cornerWidth: cornerRadius, cornerHeight: cornerRadius) | ||
| } | ||
| return .init(cgPath: cgPath) | ||
| }() |
There was a problem hiding this comment.
This to me looks it could be expressed well with a reduce
There was a problem hiding this comment.
Thanks! Updated.
| return .init(cgPath: cgPath) | ||
| }() | ||
|
|
||
| return .init(firstRect: rects[0], path: path) |
There was a problem hiding this comment.
Is there any chance rect could be empty?
There was a problem hiding this comment.
Are you referring to the rects array? I am struggling to think of a situation where it would be empty. The only one that comes to mind is a malformed markdown string that has an empty string for a linked portion. Line 698 would address this possible edge case.
There was a problem hiding this comment.
it might feel cleaner if this unchecked rects[0] were instead baked into the above guard statement. I like to do something like this.
guard let first = rects.first else { return .init(firstRect: .zero) }
so that we can later say
return .init(firstRect: first, path: path)
meherkasam-square
force-pushed
the
meher/text-links-2
branch
from
0f68690 to
011fd88
Compare
RoyalPineapple
left a comment
RoyalPineapple
left a comment
There was a problem hiding this comment.
This all makes sense, one minor stylistic suggestion.
I cant wait unitl we can expose these in snapshots tests.
meherkasam-square
force-pushed
the
meher/text-links-2
branch
from
011fd88 to
667a81d
Compare
4 participants
This change is a fast-follow to this PR. The previous PR improved how multi-line links were exposed to VoiceOver and Full Keyboard Access. However, it was flawed in that it would only expose the first fragment. This PR fixes that by encompassing the entire link inside a single bezier path.
Demo
(For the "Before" view, refer to the description in this PR.)
VoiceOver
VoiceOver.MP4
Full keyboard access
Full.Keyboard.Access.mov