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

browser: accessibility: fix incorrect semantic use of a <table> tag #9457

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

hcvcastro
Copy link
Member

" the element, only elements (table rows) and within
those, or elements (table cells), should be used. "

Change-Id: Iafff80c01158a1cb08d3a6f8cc29cbc00b9aaeb5
Signed-off-by: Henry Castro [email protected]

@eszkadev
Copy link
Contributor

eszkadev commented Jul 8, 2024

This PR causes regression, some weird spacing inside some list views:
treeview

Would be good to check also in:

  • macro dialog
  • zotero dialog
    because they have additional complex structures inside treeviews

@hcvcastro hcvcastro force-pushed the pr/master/74 branch 5 times, most recently from 38d3a14 to 9f6d884 Compare July 15, 2024 12:57
@hcvcastro hcvcastro force-pushed the pr/master/74 branch 5 times, most recently from c69ceb1 to 45008cf Compare July 17, 2024 14:38
@hcvcastro hcvcastro force-pushed the pr/master/74 branch 5 times, most recently from 13c31be to bb9b24b Compare July 24, 2024 13:38
@eszkadev
Copy link
Contributor

Hi I have proposal:

  • we have at least 3 types of treeviews
  • can we make separate classes for them? so they all inherit from TreeViewControl, have the same api, but implement differences inside

It will be much easier to modify in the future if we know what we are changing...
Also could you comment above each of the classes in which dialogs that specific implementation can be tested?

so I would expect that main "entry point", treeview handler will do something like:

if (some condition)
    treeControl = SimpleTreeControl()
else if (something else)
    treeControl = MoreAdvancedTreeControl()
else if (...)
     treeControl = SomeOtherType()
else
     error('unkonwn type');

@eszkadev
Copy link
Contributor

Also it would be nice to merge this step by step: so please implement one kind of treeview -> then we review and merge this one implementation (other types use old handler).
Then we do second and so on

@hcvcastro
Copy link
Member Author

Hi I have proposal:

we have at least 3 types of treeviews
can we make separate classes for them? so they all inherit from TreeViewControl, have the same api, but implement differences inside

Thanks, I analyzed too, but first I wanted to convert to class and reuse almost all possible code, then we can separate in
3 sub-classes, inspecting the shared code, but the problem here is the iterations of data.entries, the initial idea is to auto-detect the treeviews type in one loop iteration

@hcvcastro
Copy link
Member Author

It will be much easier to modify in the future if we know what we are changing...
Also could you comment above each of the classes in which dialogs that specific implementation can be tested?

I agree

@hcvcastro
Copy link
Member Author

so I would expect that main "entry point", treeview handler will do something like:

if (some condition)
treeControl = SimpleTreeControl()
else if (something else)
treeControl = MoreAdvancedTreeControl()
else if (...)
treeControl = SomeOtherType()
else
error('unkonwn type');

The problem I have found how to auto-detect which type of treeview, I did not analyze the Core side, it could
send data.entries, with children or not, with headers or not, or 1 column, all it must be done
in one loop iterations, we cannot iterate for every type =)

@hcvcastro hcvcastro force-pushed the pr/master/74 branch 3 times, most recently from 56728eb to 12582a0 Compare July 26, 2024 14:27
// ul -> li container, no headers, no columns or columns == 1
class UnorderedListControl extends TreeViewControl {
constructor(data, builder) {
super(data, builder);

Check warning

Code scanning / CodeQL

Superfluous trailing arguments Warning

Superfluous arguments passed to
default constructor of class TreeViewControl
.
// table -> tr -> td, simple list (no children), with or no headers, columns > 1
class SimpleTableControl extends TreeViewControl {
constructor(data, builder) {
super(data, builder);

Check warning

Code scanning / CodeQL

Superfluous trailing arguments Warning

Superfluous arguments passed to
default constructor of class TreeViewControl
.
// complex table treegrid, with children, with or no headers, columns > 1
class ComplexTableControl extends TreeViewControl {
constructor(data, builder) {
super(data, builder);

Check warning

Code scanning / CodeQL

Superfluous trailing arguments Warning

Superfluous arguments passed to
default constructor of class TreeViewControl
.
Initial tag ul -> li container with childs.

Change-Id: Iaa44489f199951fd3f5611460f14c0cda08c1382
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I2c0fd45e14a7f220181eec371d01adab05f66e5c
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I8fee01148de260b211c118a14f0a457033585565
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I3f2e31ce25115380e3f9f18be57a55920b10cfa6
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I032ffbed9becde0eb80d9f8f71ba6b4d5b117913
Signed-off-by: Henry Castro <[email protected]>
Change-Id: Ib96cb53bc1ae5bd98df9e5888802b6f1bec6cbe8
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I1f9522c11900f1e2643348dad8b1a940da232f21
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I179c0c0e707ff41a27483eaa5462b53776b49a1b
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I00b2b3eb00c46532b69f28fcc0c3a16b2383f688
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I46a576fac13111ec9691af7531676510ba1e643d
Signed-off-by: Henry Castro <[email protected]>
Set arser options to support es6 static variables.

Change-Id: Id84f50f0bcf530992d819f6b41faf5e6bf02feea
Signed-off-by: Henry Castro <[email protected]>
Change-Id: Ic3b6773a3471efe7f936ef802403e1517824ff4f
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I6c1de9c9474fde921b033eb84636f230b26eb77f
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I386a819f4380ae60c1480ca4507f372584643555
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I9daf84388977bfa7e47b7d9cef46d5a1503151de
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I38e2a164dcdbb5c1f48fd24c11a28858839c981d
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I0f2a968b14ff09f38c19c171f8c8440f24ff66b9
Signed-off-by: Henry Castro <[email protected]>
Class inheritance of TreeViewControl

Change-Id: Iaf3367825b4f73de686351bc2bd0fe5d9cbfdef0
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I6d05389855c77698b1e442ce8bb0932e9d12e361
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I8e4b7b22c564b10cf8bb81619cc08e410e8a7f40
Signed-off-by: Henry Castro <[email protected]>
Change-Id: Ifa1f9856b6b02f6ef6c30b93deaee603b964046a
Signed-off-by: Henry Castro <[email protected]>
Change-Id: I4a231f9d9d3a556e848522f59df28dd78c861b97
Signed-off-by: Henry Castro <[email protected]>
Change-Id: Ie494b24115e431c55a6edf9d12dab3b9b80651fb
Signed-off-by: Henry Castro <[email protected]>

if (entry.state !== undefined) {
td = L.DomUtil.create('td', '', tr);
selectionElement = this.createSelectionElement(td, data, entry, builder);

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

The value assigned to selectionElement here is unused.

if (entry.state !== undefined) {
td = L.DomUtil.create('td', '', tr);
selectionElement = this.createSelectionElement(td, data, entry, builder);

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning

The value assigned to selectionElement here is unused.
}

fillRow(data, entry, builder, level, parent) {
let td, state, selectionElement;

Check notice

Code scanning / CodeQL

Unused variable, import, function or class Note

Unused variable state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: To Review
Development

Successfully merging this pull request may close these issues.

3 participants