<Swing Dev> [13] Review Request: 8225372 accessibility errors in tables in java.desktop files

Tue Sep 3 17:55:04 UTC 2019

Hi Sergey,

Sorry for my belated reply.

Please see my comments inline:

On 03/08/2019 00:56, Sergey Bylokhov wrote:
> Hello.
>
> The accessibility checker found similar issues in the private methods in java.awt.geom.Path2D class. The spec for these methods is exposed via serialization form.

Path2D.java looks fine to me.

>
> So we need to fix it as well:
> http://cr.openjdk.java.net/~serb/8225372/webrev.02
>
> ----- Sergey.Bylokhov at oracle.com wrote:
>
>> Hi, Alexey.
>>
>> Thank you for review, the new version is here:
>> Fix: http://cr.openjdk.java.net/~serb/8225372/webrev.01
>> Doc:
>> http://cr.openjdk.java.net/~serb/8225372/docs.01/api/java.desktop/module-summary.html
>>
>> See comments inline:
>>
>> On 14/06/2019 09:19, Alexey Ivanov wrote:
>>> *GridBagLayout.java*
>>> Does it make sense to use nested lists for valid values> It could
>> make the presentation clearer rather than a paragraph followed by a
>> list.
>>
>> We can:
>> http://cr.openjdk.java.net/~serb/8225372/docs.01/api/java.desktop/java/awt/GridBagLayout.html
>> But I am not sure that it looks better due to the current default
>> styles for nested <li>

It looks fine to me. And it has more semantic markup.
For more visual distinction, we can add style="list-style-type: circle" 
to all the nested <ul> elements, or use ‘square’ type. Alternatively, 
it's possible to modify the style of the outer <ul> element, the nested 
<ul>'s are still rendered with ‘disc’ type.

There are several text passages that aren't part of paragraph. I'm not 
sure if “orphaned” text should be put into a <p>.

The markup for Figure 2 and 3 can be simplified, especially if the image 
headers are dropped.

There's *an error* in ‘alt’ attribute for Figure 2 and 3: the images are 
referred to as Figures 1 and 2 correspondingly.

>>
>>> The value ‘center’ is invalid for ‘float’ property in CSS, so it
>> must be removed, including style attribute of <img> element.
>>
>> I dropped all incorrect "float" attributes whenever possible.

These changes look good. Thanks for cleaning this up across the entire 
module.

>>
>>> *DesktopProperties.html*
>>> I'm still unsure we should suppress rendering <th> in bold, should
>> we not?
>>
>> I changed the code to use default style as-is.
>>
>>> *Modality.html*
>>> The table in examples could be replaced by a series of
>>> <ol style="float: left"> element with , followed by
>>> <p style="float: left"><img ...></p>
>>> Then goes <div style="clear: left"></div>.
>>> I would probably add titles to the examples to make
>>> the distinction between the examples clearer. If each
>>> example is preceded by <h3>Example #</h3>, then
>>> style="clear: left" can be applied to it.
>>> If this sounds reasonable, I can file a new bug to
>>> handle this. This way we'll get rid of presentation table.
>> I think that the final result will be quite similar to the existing
>> table?
>> We can change the order of columns, so the a11y tool will read the
>> text of the image(Example 1/Example 2/etc) and then the current
>> description of the example.
>> http://cr.openjdk.java.net/~serb/8225372/docs.01/api/java.desktop/java/awt/doc-files/Modality.html

Yes, it will look similar to how it looks at the moment. Yet the markup 
will be more semantic without presentation table. In fact, I went ahead 
and re-done the markup of examples:
http://cr.openjdk.java.net/~aivanov/8225372/webrev.00/
http://cr.openjdk.java.net/~aivanov/8225372/webrev.00/raw_files/new/src/java.desktop/share/classes/java/awt/doc-files/Modality.html#Examples

I find it clearer even when viewing visually. Each example is separated 
from others. If anchors are added, it will be possible to link to an 
individual example.

I used Narrator in Windows 10 to read the examples. I admit it's hard 
for me to judge objectively, however, the headers make the page easier 
to navigate and to understand where the next example starts.

There's a little inconsistency in the examples: at times quotes are 
used, at other times they aren't. Shall the quotes be dropped?

Another issue is that Modality.html extensively uses <u> element [3]. 
This element was deprecated in HTML 4; it was restored with HTML 5 with 
new semantic. I strongly believe the usage of <u> in this document does 
not correspond to the element semantics [4][5]. I think it should be 
replaced with <mark> “to mark key words or phrases” or <i> “to denote 
technical terms…” which better correspond to the usage on this page.

Definitely, this issue should be addressed separately from this one. The 
entire desktop module should be analysed and cleaned of <u> where it's 
used for presentational purposes only. (The same is true for <b> and <i> 
too.)

[3] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/u
[4] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/u#Usage_notes
[5] 
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/u#Avoiding_%3Cu%3E

>>> *NumericShaper.html*
>>> The table has only one row group, so you have to add row groups:
>>> <tbody> elements create the row group that will be associated with
>> “Arabic” row group header.
>>
>> fixed.
>>> *gif_metadata.html*
>>> As you mention, some tables don't have meaningful row header. And
>> it's not required in this case.
>>> Adding the additional index column which does not make the table
>> clearer does not make any sense to me. We already have column headers
>> (yes, scope="col" should be added), and that resolves the missing
>> header problem, doesn't it?
>>> See “Tables with one header” article in Web Accessibility Tutorial:
>>>
>> https://www.w3.org/WAI/tutorials/tables/one-header/#table-with-ambiguous-data
>>
>> The current document checker requires both row and columns headers. We
>> can fix the checker, but I do not see an issue with "index", these
>> tables look like a grid which contains enumerated data.

Well, I think the additional column adds noise when it's read by 
Narrator. I cannot see any scenario where the index column could be useful.

At the same time, I don't mind adding the Index column if everyone 
agrees it's the way to go. However, fixing the checker still seems like 
a better option. As another option, the first column could be the header 
of the row.

>>> *tiff_metadata.html*
>>> Compression type headers should be in <thead> (lines 516–518) and other table content in <tbody>
>>> In case of Mapping the Standard Metadata Format to TIFF Native Image Metadata, the first column could probably be the row header. I'm not sure.
>> I added index to all these tables, otherwise, the tables looks
>> non-unified.
>>
>>> The extra row in this table, lines 869–872 could be removed completely.
>> Fixed.
>>
>>> *BoxLayout.java*
>>> It would suggest more semantic mark-up for "Example" (lines 39–44):
>>> <p><b>Example:</b>
>>> <p><img ...>
>>> instead of changing only text presentation by ‘font-weight: bold’
>> property on <div> element.
>>> This is applicable to other similar cases.
>>> <b> [1] has new semantics since HTML5; <strong> [2] can also be
>> used, however, in this case this snipped does not represent anything
>> of certain importance.>
>>> style attribute on <img> element has invalid value of ‘bottom’ for
>> property ‘float’, so it must be removed.
>>
>> Fixed.
>>
>>> [1] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/b
>>> [2] https://developer.mozilla.org/en-US/docs/Web/HTML/Element/strong

>>> *componentProperties.html*
>>> The same as DesktopProperties.html: should we suppress
>>> rendering <th> in bold?
>>>
>>> JFileChooser, JInternalFrame, JInternalFrameTitlePane,
>>> JProgressBar, JSlider, JTabbedPane, Text Properties
>>> tables lack <thead> element around the header row.
>>> Because of the style applied, the header row is not
>>> rendered in bold. So suppressing bold rendering proved
>>> to be useful.

The tables of properties have no <thead> element. Their header row 
should be inside <thead> element; the start of <tbody> should be moved 
below the closing of </thead>.

>>> On 12/06/2019 06:29, Sergey Bylokhov wrote:
>>>> Hello.
>>>> Please review the fix for JDK 13.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225372
>>>> Fix: http://cr.openjdk.java.net/~serb/8225372/webrev.00
>>>> Doc: http://cr.openjdk.java.net/~serb/8225372/docs/api/java.desktop/module-summary.html
>>>>
>>>> <SNIP>
-- 
Regards,
Alexey