@kirankonduru, please remove code in this PR that's related to the other one. The file diff should only reflect changes that are being made to the specific issue that's being fixed. Remember to base your branch off of develop at the outset, and not your other feature branch.

When you've done that, please assign this issue to @anthkris for review. Thanks!

Codecov Report

Merging #2972 into develop will increase coverage by 0.08%.

@@             Coverage Diff             @@
##           develop    #2972      +/-   ##
===========================================
+ Coverage    46.24%   46.33%   +0.08%     
===========================================
  Files          227      228       +1     
  Lines        17762    17800      +38     
  Branches      2875     2871       -4     
===========================================
+ Hits          8214     8247      +33     
- Misses        9548     9553       +5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f16ebd7...eea71b8. Read the comment docs.

Hey @kirankonduru Please take a look at the failing checks. Take a look at this resource: https://github.com/oppia/oppia/wiki/If-your-build-fails...

@anthkris the test fails because a default value is needed in the description field (alt field) if we want to make it a required field. What do you suggest as the default value for this field?

@kirankonduru I'd suggest making the UI similar to the publish window.

The default text should offer an exemplar. Perhaps something like "George Frideric Handel, 18th century baroque composer" or something relatively universal.

It may also be helpful to add a bit to the label for the field, the "Alternative text (for screen readers)" part to say "Alternative text (for screen readers): Describe the contents of this image"

@seanlip Any thoughts?

Hi @anthkris, no thoughts on my end -- I defer to you and @jaredsilver on this.

While we may understand what alt text and screen readers are, I have a feeling that most teachers and other exploration creators may not. Therefore, it may make sense to tell the user why instead of what. For example, the label might say "Briefly explain this image to a visually impaired learner." And then, the placeholder could offer an exemplar as @anthkris mentioned. As an exploration creator, this may lead me to write a better and more descriptive alternative text because now I know the importance of why I'm doing it, and I also have a good example of what it should look like. What do you think about that approach, @anthkris?

Thanks @jaredsilver That's exactly what I was thinking! I think people have a vague notion of what the alt attribute is but not how to write a good one or why it's really important. I like this approach!

@kirankonduru Please let me know if you have any other questions or concerns.

Here is the modified image upload section

Now the alt field must have a string with length greater than zero if we want to make that field required. In the current schemas all the fields that are required (for example in the insert tabs section) the input fields have some descriptive text by default.

I think that it may be better to remove the first part of the label, "Alternative text (for screen readers)" and just stick with the why explanation (the second part). As for the example, it's considered bad practice to write "image of" or "picture of" in the alt text. It's redundant.

So any other suggestions for default text in the alt field?
If we add this "George Frideric Handel, 18th century baroque composer" and the user does not edit it then the alt for image becomes this string.

I can understand the problem; it's difficult to come up with something universal, but we do want it to fit the guidelines for a good alt description. So we don't want them to think that they can simply complete the phrase "Image of..."
Also, I would say that the desired behavior is that a user should be forced to edit this field before the window will close and save their changes. In other words, the default text should not remain. Is that not what is happening now?

In the current schemas whenever we make a field required we need to provide a default value. The forms are generic and are used in insert tabs and others too. We need to modify the current schemas if we want to allow the default value to be an empty string and in that case we can force the user to fill in the data.

Note that, if there's confirmation that the right thing to do here (from a user perspective) is to have a default value that is the empty string, then it should be possible to remove the general requirement that the default value must be nonempty for required fields.

@kirankonduru -- what other fields make use of this requirement? Just want to make sure that this infrastructural change doesn't break anything.

1. extensions/gadgets/AdviceBar/AdviceBar.py
2. extensions/gadgets/TestGadget/TestGadget.py
3. extensions/gadgets/base_test.py
4. schema_utils_test.py

All these files have the same condition that is needed to make a field required
i.e. ,
'validators': [{
'id': 'is_nonempty',
}],

and whenever we have an empty string as a defualt value the tests fail.

OK. If that's a comprehensive list of the affected fields, I am fine with removing that restriction.

1. Modified the unicode schema which adds a placeholder to this schema.
2. Now the required fields can be empty but they must be filled before the form can be submitted.

This is the traceback

Traceback (most recent call last):
File "/home/kiran/Desktop/oppia/oppia/core/domain/rte_component_registry_test.py", line 171, in test_default_rte_components_are_valid
component._customization_arg_specs) # pylint: disable=protected-access
File "/home/kiran/Desktop/oppia/oppia/core/domain/rte_component_registry_test.py", line 66, in _validate_customization_arg_specs
ca_spec['default_value'], ca_spec['schema']))
File "/home/kiran/Desktop/oppia/oppia/schema_utils.py", line 145, in normalize_against_schema
validator['id'], kwargs, normalized_obj))
AssertionError: Validation failed: is_nonempty ({}) for object

Sorry the mistake was on my part. In a hurry I modified the wrong part and hence was getting the wrong traceback.
Here this _Validators.get(validator['id'])(normalized_obj, **kwargs) becomes null when we have empty value and hence the assertion error.
I guess we need to add some condition which will accept the is_nonempty as an empty string in this test. What do you suggest?

OK, here's what to do:

• Give normalize_against_schema an additional arg apply_custom_validators which defaults to True. If the arg value is True, apply the block of code at the end that checks validation (the one starting with "Validate the normalized object"), otherwise don't do those checks.
• In rte_component_registry_test.py, pass in False for this additional arg, and add a comment explaining that this is because the Image component has a required field whose default value is non-empty.

Note that this preserves the conceptual clarity, which is important when writing code. Things need to do what they say on the tin, otherwise developers will have trouble figuring out what's going on and modifying existing code safely.

@anthkris -- to confirm, we do want the default value for that text box to be empty, right? (I personally don't know the best thing to do here from a UI/UX perspective, so I'd defer to you here.)

@seanlip It would be ideal to have an exemplar there, as we do in the publish modal. However, there may be some difficulty in creating something universal enough that everyone looking at it could understand. So, in this case, empty string may be the best case scenario

Code LGTM, apart from the question I had for @anthkris. Thanks @kirankonduru!

@kirankonduru @seanlip LGTM!