Skip to content
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

[$500] BUG: Mac/Firefox - Opening the protect PDF modal and pressing TAB doesn’t focus on enter the password reported by @dhairyasenjaliya #12058

Closed
kavimuru opened this issue Oct 21, 2022 · 76 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kavimuru
Copy link

kavimuru commented Oct 21, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Go to any conversation
  2. Send Protected PDF (if required)
  3. Open Protected PDF
  4. Press TAB key simultaneously try to navigate to enter the password with TAB
  5. Notice it will focus only on Close Modal /Download PDF

Expected Result:

On pressing TAB it should be able to focus on enter the password

Actual Result:

On pressing TAB it does not focus on enter the password

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web - Mac Firefox only

Version Number: 1.2.18-4
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Screen.Recording.2022-10-20.at.9.00.17.AM.mov
Recording.36.mp4

PDF used :
password-protected.pdf

Expensify/Expensify Issue URL:
Issue reported by: @dhairyasenjaliya
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1666236736080999

View all open jobs on GitHub

@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Oct 21, 2022
@mallenexpensify mallenexpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Oct 21, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 21, 2022

Triggered auto assignment to @zanyrenney (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@melvin-bot melvin-bot bot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Oct 21, 2022
@mallenexpensify
Copy link
Contributor

@zanyrenney new BZ chore SO is here, we want to ensure BZ stays assigned to all issues so we can get to BugZero,

@melvin-bot melvin-bot bot added the Overdue label Oct 24, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 25, 2022

@zanyrenney Whoops! This issue is 2 days overdue. Let's get this updated quick!

@zanyrenney
Copy link
Contributor

Hey @kavimuru - Are you created the password protected PDF in Adobe? That's the only way I see how to recreate this. If you can attach an example of the PDF to this GH issue, i can test this properly.

@melvin-bot melvin-bot bot removed the Overdue label Oct 25, 2022
@kavimuru
Copy link
Author

@zanyrenney Added the pdf used for this test in the GH.

@zanyrenney
Copy link
Contributor

zanyrenney commented Oct 26, 2022

I need the password to be able to test too though, please? @kavimuru

@kavimuru
Copy link
Author

@zanyrenney Password is 1234 and attached another PDF

@zanyrenney
Copy link
Contributor

Thanks!

@zanyrenney
Copy link
Contributor

zanyrenney commented Oct 27, 2022

I can't reproduce this on Mac/Chrome - is it only happening on FireFox @kavimuru ?

2022-10-27_16-00-21 (1)

@dhairyasenjaliya
Copy link
Contributor

@zanyrenney this bug is only related to firefox

@zanyrenney
Copy link
Contributor

Thanks. Asking in slack if we should be testing and fixing these super edge cases: https://expensify.slack.com/archives/C01SKUP7QR0/p1666963361304399

@zanyrenney zanyrenney added the External Added to denote the issue can be worked on by a contributor label Oct 28, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 28, 2022

Current assignee @zanyrenney is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Oct 28, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 28, 2022
@melvin-bot
Copy link

melvin-bot bot commented Oct 28, 2022

Triggered auto assignment to @roryabraham (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title BUG: Mac/Firefox - Opening the protect PDF modal and pressing TAB doesn’t focus on enter the password reported by @dhairyasenjaliya [$250] BUG: Mac/Firefox - Opening the protect PDF modal and pressing TAB doesn’t focus on enter the password reported by @dhairyasenjaliya Oct 28, 2022
@parasharrajat
Copy link
Member

No proposals yet.

@melvin-bot melvin-bot bot added the Overdue label Oct 31, 2022
@s77rt
Copy link
Contributor

s77rt commented Nov 11, 2022

I meant something like this

diff --git a/src/components/PDFView/PDFInfoMessage.js b/src/components/PDFView/PDFInfoMessage.js
index 5d583db40..898983f13 100644
--- a/src/components/PDFView/PDFInfoMessage.js
+++ b/src/components/PDFView/PDFInfoMessage.js
@@ -1,6 +1,6 @@
 import React from 'react';
 import PropTypes from 'prop-types';
-import {View} from 'react-native';
+import {View, Pressable} from 'react-native';
 import Text from '../Text';
 import TextLink from '../TextLink';
 import Icon from '../Icon';
@@ -29,9 +29,11 @@ const PDFInfoMessage = props => (
         <Text>{props.translate('attachmentView.pdfPasswordForm.infoText')}</Text>
         <Text>
             {props.translate('attachmentView.pdfPasswordForm.beforeLinkText')}
-            <TextLink onPress={props.onShowForm}>
-                {` ${props.translate('attachmentView.pdfPasswordForm.linkText')} `}
-            </TextLink>
+            <Pressable onPress={props.onShowForm}>
+                <Text style={[styles.link]}>
+                    {` ${props.translate('attachmentView.pdfPasswordForm.linkText')} `}
+                </Text>
+            </Pressable>
             {props.translate('attachmentView.pdfPasswordForm.afterLinkText')}
         </Text>
     </View>

@huzaifa-99
Copy link
Contributor

huzaifa-99 commented Nov 11, 2022

@s77rt something similar already proposed here but this would not work as suggested by @parasharrajat here

@melvin-bot melvin-bot bot added the Overdue label Nov 14, 2022
@zanyrenney
Copy link
Contributor

Hi everyone - this issue is a duplicate of #12010, please can you post your proposals on the original issue to avoid duplication and track this correctly. @huzaifa-99 @s77rt cc. @parasharrajat @roryabraham

@melvin-bot melvin-bot bot removed the Overdue label Nov 14, 2022
@zanyrenney
Copy link
Contributor

Closing this issue in favour of #12010

@huzaifa-99
Copy link
Contributor

huzaifa-99 commented Nov 14, 2022

@zanyrenney #12010 is related to the focus issues on Download/Close buttons on Mac/Safari only. This issue is related to focus issues on a label "Enter the password" in Mac/FireFox only.

Should we still consider these as one?

@dhairyasenjaliya
Copy link
Contributor

dhairyasenjaliya commented Nov 14, 2022

hello @zanyrenney both issues are separate

#12058 this issue is about "enter the password" not focusing on the tab

Screenshot 2022-11-14 at 5 08 59 PM

#12010 this issue is about protect the PDF modal and pressing TAB doesn’t focus on download/close
Screenshot 2022-11-14 at 5 10 07 PM

  • as this is not a duplicate issue so I do think this would be eligible for bug reporting

@s77rt
Copy link
Contributor

s77rt commented Nov 14, 2022

@zanyrenney @huzaifa-99 @dhairyasenjaliya
not the same issue.
but #12010 covers both #12058 and #12554

@huzaifa-99
Copy link
Contributor

Bump @zanyrenney

@zanyrenney
Copy link
Contributor

zanyrenney commented Nov 29, 2022

Sorry @huzaifa-99 but from this comment it looks like it is correct this is closed in favour of that issue that covers this off? #12058 (comment) @roryabraham please also let me know your thoughts here.

@dhairyasenjaliya
Copy link
Contributor

@zanyrenney also bug reporting bonus remaining since it was valid at reporting time then later on it was decided not to focus on keyboard navigation issues

@melvin-bot
Copy link

melvin-bot bot commented Nov 29, 2022

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

@huzaifa-99
Copy link
Contributor

huzaifa-99 commented Nov 29, 2022

@zanyrenney related to the comment you mentioned, I am not sure if the other issue covers this one, please see this comment on the other issue. Maybe @s77rt can share more context.

At this time, I am not sure if my proposal is valid as from discussion here. The discussion does point to solve issues with accepted proposals before that thread was created (16 Nov), mine was accepted on 11 Nov.

Please feel free to close if otherwise. Thanks

@s77rt
Copy link
Contributor

s77rt commented Nov 29, 2022

The solution to this issue #12010
is the solution of #12058 and #12554

I don't know how to handle such case but it's logical to close the new issues in favour of the old one but this didn't happen.
If it was about implementing a solution, I'd say @huzaifa-99 should be credited for solving a part of the issue (original issue), but I don't know how the reporting bonus work in such case so I can't help in that.

@JmillsExpensify
Copy link

@roryabraham @parasharrajat Is this issue still aimed at solving focus for TAB navigation? If so, I think we should close it. We've announced that these are new accessibility features and not bugs.

@roryabraham
Copy link
Contributor

I agree, we should close this.

also bug reporting bonus remaining since it was valid at reporting time then later on it was decided not to focus on keyboard navigation issues

Not sure if this is correct or not, but I think we need to sort this out when we close it. Normally reporting bonus is only paid for bugs and new features that are fixed/implemented.

@dhairyasenjaliya
Copy link
Contributor

Alright, got it, thank you @JmillsExpensify @roryabraham I think we are good to close then since this issue is about TAB navigation

@parasharrajat
Copy link
Member

parasharrajat commented Nov 30, 2022

#12058 (comment)

No payment is due here.

@zanyrenney
Copy link
Contributor

Taken to slack for discussion: https://expensify.slack.com/archives/C01GTK53T8Q/p1669809248604369

@zanyrenney
Copy link
Contributor

hi @huzaifa-99 we decided that the proposal itself, which we approved is the only segment for payment, please can you apply for the job on upwork so we can pay you: https://www.upwork.com/jobs/~010f0af8b1df252e1d

@huzaifa-99
Copy link
Contributor

@zanyrenney applied thanks.

we decided that the proposal itself, which we approved is the only segment for payment

Just to be sure, I should raise a PR, right?

@zanyrenney
Copy link
Contributor

Hey @huzaifa-99 - no, you don't need to submit a PR. The reason you are due the payment is that you worked on the proposal already by the time we asked you not to do it, so, we're paying you for the proposal given the work was complete even though we aren't using it, for fairness and appreciation of your time. I hope that makes sense? Let me know if you have any questions.

@zanyrenney
Copy link
Contributor

offer sent @huzaifa-99

@huzaifa-99
Copy link
Contributor

Hey @huzaifa-99 - no, you don't need to submit a PR. The reason you are due the payment is that you worked on the proposal already by the time we asked you not to do it, so, we're paying you for the proposal given the work was complete even though we aren't using it, for fairness and appreciation of your time. I hope that makes sense? Let me know if you have any questions.

@zanyrenney thanks for the consideration and followup :)

@zanyrenney
Copy link
Contributor

Paid via upwork, closing this out. @huzaifa-99 Reopen if you have any problems or follow up questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests