1229129 - Text is highlighted on tap for non-links

Status: RESOLVED FIXED

(Firefox for iOS :: Browser, defect)

Description

[Brian Nicholson (:bnicholson)]

Go to an article on nytimes.com, and tap the text in any paragraph. The entire paragraph is briefly highlighted even though there's no link. Not reproducible in Safari.

Comment 1

[Brian Nicholson (:bnicholson)]

Looks like the highlight is created by WKWebView automatically when we add a mouseup handler to an element. We use a mouseup handler as a workaround for context menu interactions with VoiceOver, so one fix could be to add that handler only if VoiceOver is enabled.

Comment 2

[EM]

Hi, I'd like to work on this bug. Can I have more guidance and info as to how to proceed please?

Comment 3

[Brian Nicholson (:bnicholson)]

Hi, EM -- thanks for looking into this!

Due to bug 1191687, we need a mouseup listener at [1] to prevent the context menu from interfering with voiceover. Unfortunately, it seems like the presence of a mouseup listener is what causes *this* bug. The solution I'm proposing is to add this mouseup listener only when VoiceOver is enabled (that is, when it's needed).

In order to do that, we'll need to listen for VoiceOver events to get their status. It looks like we can detect VoiceOver's state using UIAccessibilityVoiceOverStatusChanged [2]. If you add a listener for that event in ContextMenuHelper [3], you can send a message to the content script (ContextMenu.js) to update a boolean flag that you create to hold this state. Finally, you can wrap the call at [1] with an if condition using this boolean, so that we only add the mouseup listener if VoiceOver is enabled.

Note that this won't be the entire solution; we'll also need a way to keep this boolean in sync with the existing VoiceOver state every time a new page is loaded. For this part, you can send a message from ContextMenu.js simply to signal that the page has loaded; ContextMenuHelper can then respond with a message of its own with the VoiceOver state.

To verify that your fix works, see the steps to reproduce in comment 0; with the bug fixed, the tap highlight should be gone with VoiceOver disabled.

Hopefully that's enough to get you started. Let me know if anything is unclear or if you have other questions!

[1] https://github.com/mozilla/firefox-ios/blob/3e46ce28a820cc5eb031bbf7010de67fabf94b3d/Client/Assets/ContextMenu.js#L105
[2] https://developer.apple.com/library/ios/documentation/UIKit/Reference/UIAccessibility_Protocol/#//apple_ref/c/data/UIAccessibilityVoiceOverStatusChanged
[3] https://github.com/mozilla/firefox-ios/blob/3e46ce28a820cc5eb031bbf7010de67fabf94b3d/Client/Frontend/Browser/ContextMenuHelper.swift

Comment 4

[Aaron Train [:aaronmt]]

Tried to reproduce this, I don't see a full paragraph highlight, however it is difficult to unselect a highlighted word. Tested on NYTimes on one of the front-page articles using latest TestFlight builds.

Comment 5

[Brian Nicholson (:bnicholson)]

Another option we have is to simply disable the custom context menu altogether for VoiceOver users like we did in bug 1191687. This whole mouseup mess is a big hack, and the fix mentioned in comment 3 would be a bigger hack on top of that, so I think removing it would be the safer option.

If we want to go that route, we should remove the OS version check on this line:

https://github.com/mozilla/firefox-ios/blob/f8f4788ba64e310ac225ed36f22ec660dc833d71/Client/Frontend/Browser/ContextMenuHelper.swift#L30

We can then remove the mouseup listener in ContextMenu.js, which is the source of this bug.

Comment 6

[Bryan Munar (:bkmunar)]

Attachment: PR

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Lets do this. But also please file a followup bug to fix this properly or at least to make sure we remember this change?

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8703911 [details] [review]
PR

Looks good!