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

Fixes #3432: Use alt-with-value from <oppia-noninteractive-image> in RichText #4669

Conversation

vrajdesai78
Copy link
Contributor

@vrajdesai78 vrajdesai78 commented Oct 20, 2022

Explanation

Fixes #3432: Created a handleContentDescription function in CustomHtmlContentHandler which is further override in ImageTagHandler. With current approach, I am setting up text (content description text from alt-with-value) to image.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

Demo Video

rich-text.mp4

@BenHenning
Copy link
Sponsor Member

As discussed, this requires further debugging by you, correct @vrajdesai78? Sending it back to you until the solution is fully working.

Also, should this be marked as a draft since it's not yet finished?

@BenHenning BenHenning assigned vrajdesai78 and unassigned BenHenning Oct 21, 2022
@vrajdesai78
Copy link
Contributor Author

As discussed, this requires further debugging by you, correct @vrajdesai78? Sending it back to you until the solution is fully working.

Also, should this be marked as a draft since it's not yet finished?

Yes we can mark it as draft PR

@rt4914
Copy link
Contributor

rt4914 commented Oct 22, 2022

Not sure why its working when the app is coming from background but maybe log all the important information which is responsible for displaying the item-selection-interaction and from that first try to understand what is different.

For example,
when we are changing the state, ie. going from one-question to another, activity/fragment lifecycle is not shifting to other methods like onResume, onCreate, onPause, onStop, onStart`, etc. but when app is in background and is again in foreground these methods are called and maybe there is some code in these methods which is fixing the issue.

@rt4914 rt4914 removed their assignment Oct 22, 2022
@vrajdesai78
Copy link
Contributor Author

Not sure why its working when the app is coming from background but maybe log all the important information which is responsible for displaying the item-selection-interaction and from that first try to understand what is different.

For example, when we are changing the state, ie. going from one-question to another, activity/fragment lifecycle is not shifting to other methods like onResume, onCreate, onPause, onStop, onStart`, etc. but when app is in background and is again in foreground these methods are called and maybe there is some code in these methods which is fixing the issue.

@rt4914 I have tried to investigate lifecycle methods like onResume and onStop which are getting called when we keep our app in background and again in foreground. Suprisingly, I haven't found any such method's overriding (onResume, onStop and onPause) in our current codebase. Can you help suggest me some next steps how can I investigate this issue.

@vrajdesai78
Copy link
Contributor Author

@BenHenning can you PTAL. I have solve the issue of loading images in SelectionInteractionView. Actually, there we were calling View.width { ... } which is creating the problem, so now I am calculating the width directly by calling View.width. Also, update the video in the description with the latest code changes.

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrajdesai78 LGTM, Just check ben's comment, in that PR I have mentioned how to test that bug so please follow those steps and create a video too displaying that the current change does not re-surface that issue.

@rt4914 rt4914 removed their assignment Nov 2, 2022
@vrajdesai78
Copy link
Contributor Author

@BenHenning can you PTAL. I have replied to all of your comments and also mentioned why this solution works. Thanks

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vrajdesai78. Just a bit of work needed left on the UrlImageParser comment, but otherwise the PR LGTM.

@BenHenning BenHenning removed their assignment Nov 7, 2022
@vrajdesai78 vrajdesai78 assigned BenHenning and unassigned vrajdesai78 Nov 7, 2022
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vrajdesai78 just had a few nits--PTAL.

@BenHenning BenHenning assigned vrajdesai78 and unassigned BenHenning Nov 7, 2022
vrajdesai78 and others added 2 commits November 7, 2022 14:02
…mageParser.kt

Co-authored-by: Ben Henning <henning.benmax@gmail.com>
…mageParser.kt

Co-authored-by: Ben Henning <henning.benmax@gmail.com>
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vrajdesai78--the PR LGTM. Also, please make sure not to resolve revewiers' comments (since it makes it easier to then lose these comments).

@BenHenning BenHenning enabled auto-merge (squash) November 7, 2022 08:35
@vrajdesai78
Copy link
Contributor Author

Thanks @vrajdesai78 just had a few nits--PTAL.

@BenHenning fixed nit changes

@oppiabot
Copy link

oppiabot bot commented Nov 7, 2022

Unassigning @BenHenning since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Nov 7, 2022
@oppiabot oppiabot bot assigned BenHenning and unassigned vrajdesai78 Nov 7, 2022
@oppiabot
Copy link

oppiabot bot commented Nov 7, 2022

Unassigning @vrajdesai78 since a re-review was requested. @vrajdesai78, please make sure you have addressed all review comments. Thanks!

@vrajdesai78
Copy link
Contributor Author

@BenHenning @rt4914 can you re-run CI checks, it failed due to connection timed out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[A11y] Use alt-with-value from <oppia-noninteractive-image> in RichText
3 participants