-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
JDK-8314731 : Add support for the alt attribute in the image type input HTML tag #15319
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back scientificware! A progress list of the required criteria for merging this PR into |
@scientificware The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
src/java.desktop/share/classes/javax/swing/text/html/FormView.java
Outdated
Show resolved
Hide resolved
…java FormView.java : Remove a redundant space. Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>
@scientificware This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Waiting for a review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you write a regression test for it?
JButton button; | ||
try { | ||
URL base = ((HTMLDocument)getElement().getDocument()).getBase(); | ||
@SuppressWarnings("deprecation") | ||
URL srcURL = new URL(base, srcAtt); | ||
Icon icon = new ImageIcon(srcURL); | ||
button = new JButton(icon); | ||
ImageIcon icon = new ImageIcon(srcURL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably pass altAtt
as the description
parameter, yet I'm unsure if it's ever used in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably pass
altAtt
as thedescription
parameter, yet I'm unsure if it's ever used in this context.
As I say, I'm unsure if it's used… The description could be exposed to Accessibility API. By default, ImageIcon
uses the URL as the description if it isn't provided:
jdk/src/java.desktop/share/classes/javax/swing/ImageIcon.java
Lines 231 to 233 in 99de9bb
public ImageIcon (URL location) { | |
this(location, location.toExternalForm()); | |
} |
Now that you handle the alt
attribute, you can pass the description if it's provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have this in mind ?
<html>
<body>
<input type=image name=point src="file:oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
<p>
<input type=image name=point src="file:oracle_logo_50x50.jpg">
<p>
<input type=image name=point src="file:none_oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
<p>
<input type=image name=point src="file:none_oracle_logo_50x50.jpg">
<p>
<input type=image name=point src="files:none_oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
<p>
<input type=image name=point src="files:none_oracle_logo_50x50.jpg">
<p>
</body>
</html>
String srcAtt = (String) attr.getAttribute(HTML.Attribute.SRC);
String altAtt = (String) attr.getAttribute(HTML.Attribute.ALT);
if (altAtt == null) {
altAtt = srcAtt;
}
JButton button;
try {
URL base = ((HTMLDocument)getElement().getDocument()).getBase();
@SuppressWarnings("deprecation")
URL srcURL = new URL(base, srcAtt);
ImageIcon icon = new ImageIcon(srcURL, altAtt);
button = new JButton(icon);
} catch (MalformedURLException e) {
button = new JButton(altAtt == null ? srcAtt : altAtt);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, something similar:
String srcAtt = (String) attr.getAttribute(HTML.Attribute.SRC);
String altAtt = (String) attr.getAttribute(HTML.Attribute.ALT);
JButton button;
try {
URL base = ((HTMLDocument)getElement().getDocument()).getBase();
@SuppressWarnings("deprecation")
URL srcURL = new URL(base, srcAtt);
ImageIcon icon = altAtt != null
? new ImageIcon(srcURL, altAtt)
: new ImageIcon(srcURL);
button = icon.getImageLoadStatus() == MediaTracker.COMPLETE
? new JButton(icon)
: new JButton(altAtt != null ? altAtt : srcAtt);
} catch (MalformedURLException e) {
button = new JButton(altAtt != null ? altAtt : srcAtt);
}
Icon icon = new ImageIcon(srcURL); | ||
button = new JButton(icon); | ||
ImageIcon icon = new ImageIcon(srcURL); | ||
button = icon.getImageLoadStatus() == MediaTracker.COMPLETE ? new JButton(icon) : new JButton(altAtt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I thought the status could change, it can't.
Looks good to me.
Icon icon = new ImageIcon(srcURL); | ||
button = new JButton(icon); | ||
ImageIcon icon = new ImageIcon(srcURL); | ||
button = icon.getImageLoadStatus() == MediaTracker.COMPLETE ? new JButton(icon) : new JButton(altAtt); | ||
} catch (MalformedURLException e) { | ||
button = new JButton(srcAtt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
button = new JButton(srcAtt); | |
button = new JButton(altAtt); |
If altAtt
is provided, it should be used to handle invalid srcAtt
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aivanov-jdk My apologies, I forgot to reply. I wanted to distinguish the two cases : (Missing resource or bad resource name) and malformed URL. Like in the previous example I gave.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aivanov-jdk My apologies, I forgot to reply. I wanted to distinguish the two cases : (Missing resource or bad resource name) and malformed URL. Like in the previous example I gave.
What purpose does this distinction serve?
It could give an additional insight to the app developer. At the same time, the app developer should ensure the image loads.
Does the user of the app care about it [the distinction]? Unlikely. The end result is the image isn't loaded. As HTML specifies, the alt
attribute contains the image description, which is more useful to the user in all the cases where the image failed to load.
Therefore I think you should fallback to altAtt
if it's provided.
This actually raises another question: what if altAtt
isn't provided? Should it fallback to srcURL
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What purpose does this distinction serve?
Definitely an application developer and because it's the context of this HTML implementation, may I preserve this distinction ?
This actually raises another question: what if altAtt isn't provided? Should it fallback to srcURL?
Yes, it's a logical error on my part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What purpose does this distinction serve?
Definitely an application developer and because it's the context of this HTML implementation, may I preserve this distinction ?
Okay, let it be. However, I still think it's not the right thing.
There was nothing else but the src
attribute, now there is.
This actually raises another question: what if altAtt isn't provided? Should it fallback to srcURL?
Yes, it's a logical error on my part.
Are you going to address it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<html>
<body>
<input type=image name=point src="file:oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
<p>
<input type=image name=point src="file:oracle_logo_50x50.jpg">
<p>
<input type=image name=point src="file:none_oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
<p>
<input type=image name=point src="file:none_oracle_logo_50x50.jpg">
<p>
<input type=image name=point src="files:none_oracle_logo_50x50.jpg" alt="Logo Oracle JPG">
<p>
<input type=image name=point src="files:none_oracle_logo_50x50.jpg">
<p>
</body>
</html>
- Left before the Patch
- Right with your suggestion and if the distinction is preserved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what I meant.
A button with the plain source is better than a button without a title at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to address it?
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a problem with this change, but there's no goal to bring Swing HTML up to any later HTML spec version.
@scientificware This change is no longer ready for integration - check the PR body for details. |
/integrate |
@scientificware |
JButton button; | ||
try { | ||
URL base = ((HTMLDocument)getElement().getDocument()).getBase(); | ||
@SuppressWarnings("deprecation") | ||
URL srcURL = new URL(base, srcAtt); | ||
Icon icon = new ImageIcon(srcURL); | ||
button = new JButton(icon); | ||
ImageIcon icon = new ImageIcon(srcURL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should probably pass
altAtt
as thedescription
parameter, yet I'm unsure if it's ever used in this context.
As I say, I'm unsure if it's used… The description could be exposed to Accessibility API. By default, ImageIcon
uses the URL as the description if it isn't provided:
jdk/src/java.desktop/share/classes/javax/swing/ImageIcon.java
Lines 231 to 233 in 99de9bb
public ImageIcon (URL location) { | |
this(location, location.toExternalForm()); | |
} |
Now that you handle the alt
attribute, you can pass the description if it's provided.
Icon icon = new ImageIcon(srcURL); | ||
button = new JButton(icon); | ||
ImageIcon icon = new ImageIcon(srcURL); | ||
button = icon.getImageLoadStatus() == MediaTracker.COMPLETE ? new JButton(icon) : new JButton(altAtt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it fallback to src
attribute if the alt
attribute is missing?
/reviewers 2 |
@aivanov-jdk |
Is it possible to write a jtreg test for this feature? Do we want to test it? |
- Fall back to the src attribute if the alt attribute is missing.
…_alt_attribute_support' of github.com:scientificware/jdk into scientificware-patch-006-HTML-adds_input_tag_image_type_alt_attribute_support
- Pass altAtt as the description parameter in the ImageIcon constructor.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I wonder if it's possible to write a (unit) test which verifies the new behaviour… and whether it's worth doing. |
|
@scientificware This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@scientificware - are you still working on this ? |
@prrace Yes but I have to postpone this work. |
@scientificware This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@scientificware please say when you will be getting back to this |
@prrace Sorry for the delay about all my contributions but I won a racing bib for the "Marathon Pour Tous" of Paris 2024 Olympic Games. So I have suspended all projects becauseof my training plan until september. My intent is to deliver the unit test in december. |
Congratulations! Good luck!
@scientificware Could you please convert this PR to draft then? Click Convert to draft on the right side below the list of reviewers. |
@scientificware This pull request has been inactive for more than 8 weeks and will be automatically closed if another 8 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
This is referenced in Java Bug Database as
This is tracked in JBS as
According HTML 3.2 specification
alt
is not an attribute of theinput
element.According HTML 4.01 specifications :
This feature is not implemented in
FormView.java
.This also affects the HTML 32 DTDLeft before the patch and right after the patch.
The image to use with the code above, save it with the name :
![oracle_logo_50x50.jpg](https://private-user-images.githubusercontent.com/19194678/261181159-66eac51a-00a4-42f5-b330-e1d41af37988.jpg?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjAxMjQ4NzIsIm5iZiI6MTcyMDEyNDU3MiwicGF0aCI6Ii8xOTE5NDY3OC8yNjExODExNTktNjZlYWM1MWEtMDBhNC00MmY1LWIzMzAtZTFkNDFhZjM3OTg4LmpwZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MDQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzA0VDIwMjI1MlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWViMGZhZWFjM2JiMjk1NzEyNjMzZTM2YjFiZTYxN2FmZWUyOTkwZTJmYTFhNWIxYmY4OTYwYzUzMGUxMjdkOTQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.vPR9I1PSStxJlfvpIwydDrEIctwcjw8DnRKkxJXxBaM)
oracle_logo_50x50.jpg
Designed from : ScientificWare : JDK-8314731 : Add support for the alt attribute in the image type input HTML tag
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15319/head:pull/15319
$ git checkout pull/15319
Update a local copy of the PR:
$ git checkout pull/15319
$ git pull https://git.openjdk.org/jdk.git pull/15319/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15319
View PR using the GUI difftool:
$ git pr show -t 15319
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15319.diff
Webrev
Link to Webrev Comment