Fix annotation input focus trap regression in Safari #13229
Conversation
This causes issues on Safari (see mozilla#12592 and mozilla#13191). Just removing the line fixes the issues and no difference / regression in behavior was observed without the `event.target.setSelection(0, 0);` bit in other browsers (including Firefox). The blur (switching focus out of the control) does effectively achieve the same thing in all browsers I have tested on. Tested specifically in Firefox with 1. inspect a `TextWidgetAnnotation` `input` element 2. `$0.setSelectionRange = (a, b) => console.warn(a, b);` 3. type `wow` and select `wow` in the input element 4. click/tab/tap outside the input element 5. <kbd>CTRL</kbd> + <kbd>C</kbd> ➡ no `wow` ✔ copied 6. <kbd>CTRL</kbd> + <kbd>V</kbd> ➡ confirmed
setSelectionRange(0, 0); // required only by Firefox, causes issues in Safari (see mozilla#12592 and mozilla#13191). scrollLeft = 0; // is a fix that breaks the focus trap in Safari while keeping Firefox behavior same.
setSelectionRange(0, 0); // required only by Firefox, causes issues in Safari (see mozilla#12592 and mozilla#13191). scrollLeft = 0; // is a fix that breaks the focus trap in Safari while keeping Firefox behavior same for mozilla#12359
|
Sorry for PR #13224 yesterday, I honestly failed to see the link to #12359 which now has no Firefox regression with this change. @Snuffleupagus - please see files changed: 1 LoC.
Safari bug (?) report for |
CetinSert
changed the title
CetinSert
changed the title
CetinSert
changed the title
CetinSert
changed the title
CetinSert
changed the title
| @@ -668,7 +668,7 @@ class TextWidgetAnnotationElement extends WidgetAnnotationElement { | |||
| if (elementData.formattedValue) { | |||
| event.target.value = elementData.formattedValue; | |||
| } | |||
| event.target.setSelectionRange(0, 0); | |||
| event.target.scrollLeft = 0; | |||
There was a problem hiding this comment.
This really needs a good comment, referencing the original issue, since it's less clear exactly what this does than in the previous code; I'd suggest:
// Reset the cursor position to the start of the field (issue 12359).
event.target.scrollLeft = 0;
When fixing this, please remember https://github.com/mozilla/pdf.js/wiki/Squashing-Commits and make sure that the patch has a proper commit message (not just PR description).
`setSelectionRange(0, 0)` added in mozilla@44b24fc for mozilla#12359, required only by Firefox ([bug](https://bugzilla.mozilla.org/show_bug.cgi?id=860329)), causes issues mozilla#13191, mozilla#12592 in Safari. `scrollLeft = 0` is a fix that breaks the focus trap in Safari while **keeping Firefox behavior same for mozilla#12359**.
There was a problem hiding this comment.
As mentioned previously, you need to follow https://github.com/mozilla/pdf.js/wiki/Squashing-Commits since we cannot consider landing this in its current state.
Note also that it's not advisable to do development directly against the master branch, please see https://github.com/mozilla/pdf.js/wiki/Contributing
|
@Snuffleupagus see PR #13232. Thank you for your prompt and patient guidance! |
setSelectionRange(0, 0)added in 44b24fc for #12359, required only by Firefox (bug), causes issues #13191, #12592 in Safari.scrollLeft = 0is a fix that breaks the focus trap in Safari while keeping Firefox behavior same for #12359.@calixteman