Accessibility: change table layout in toolbar #8587

Darshan-upadhyay1110 · 2024-03-20T16:14:44Z

Change table layout to CSS base table structure

For the editor in the home view, a layout table is used, and it includes the use of IDs. This can
potentially lead to issues with the interpretation of content by assistive technologies
Layout tables are
meant to structure the layout of a page and should not contain structural markup like th , caption ,
summary , headers , or id.

Solution:

replace td and 'table' tag with div
make some changes on css to be consistent with our previous design

Adjust table elements for mobile view

we changed table structure of toolbar-wrapper with div
so we also need to consider mobile view
made some necessary changes for mobile view because of structure change in cool.html.m4 ('toolbar-wrapper'

browser/src/control/Control.MobileTopBar.js

eszkadev

code is ok, waiting for Pedro wrt CSS

eszkadev · 2024-03-22T07:56:12Z

I canceled cypress, I will retrigger build after my fix for cypress will be merged.

eszkadev · 2024-03-25T06:16:30Z

rebased so it will include toolbar reworks

pedropintosilva

Desktop: all good
Mobile:

the back button and hamburger are misaligned -> fixed pushed another commit
Calc on mobile is broken: formula bar is inaccessible: I have pushed (last commit [WIP]) that fixes almost all issues, please feel free to re-use it and fix the last cosmetic issue

pedropintosilva · 2024-03-28T14:31:23Z

Desktop: all good Mobile:

* the back button and hamburger are misaligned -> fixed pushed another commit

* Calc on mobile is broken: formula bar is inaccessible: I have pushed (last commit [WIP]) that fixes almost all issues, please feel free to re-use it and fix the last cosmetic issue

Tested on real android device with nc with chrome and chrome remote debug tools

Darshan-upadhyay1110 · 2024-03-29T14:28:33Z

Desktop: all good Mobile:

the back button and hamburger are misaligned -> fixed pushed another commit

Calc on mobile is broken: formula bar is inaccessible: I have pushed (last commit [WIP]) that fixes almost all issues, please feel free to re-use it and fix the last cosmetic issue

I think we should not change flex to grid, it is risky.
I have tested this it moves the toolbar icons if we type in formula bar
Here is the ref:
Screencast from 2024-03-29 16-19-32.webm

I have made some changes by adding a wrapper with CSS.

I have tested all apps in desktop + mobile. All Good

CC: @pedropintosilva

eszkadev · 2024-03-30T20:29:11Z

Please rebase on top of recent w2ui rework to be sure it still works.

Darshan-upadhyay1110 · 2024-04-01T05:16:21Z

Please rebase on top of recent w2ui rework to be sure it still works.

Things are now broken after w2ui rework :( . I will try to fix it

CC: @pedropintosilva please do not review it now i will post comment after fixes thanks

Darshan-upadhyay1110 · 2024-04-01T05:48:46Z

Please rebase on top of recent w2ui rework to be sure it still works.

Things are now broken after w2ui rework :( . I will try to fix it

CC: @pedropintosilva please do not review it now i will post comment after fixes thanks

It seems a normal fix. There is fix height for toolbar-up, which is not good. Tested with mobile view and desktop all looks OK

CC: @pedropintosilva ready to review

eszkadev · 2024-04-01T07:49:00Z

@Darshan-upadhyay1110 there are incoming mobile toolbar conversions, best to not touch mobile yet
After we will remove w2ui then we can do it on clean structure

eg PR: #8673

Darshan-upadhyay1110 · 2024-04-01T08:15:58Z

@Darshan-upadhyay1110 there are incoming mobile toolbar conversions, best to not touch mobile yet After we will remove w2ui then we can do it on clean structure

eg PR: #8673

Sure, maybe i will wait till this PR merge then will proceed to change table thing

eszkadev · 2024-04-04T16:23:30Z

Please rebase on top of recent w2ui rework to be sure it still works.

Note:

cypress_test/integration_tests/desktop/writer/scrolling_spec.js seems to be a strange test case. as a locally i tried the same document which is used in this document and followed the same steps.

I can see in status bar there is a possibility to get "Page 1 and 2 of 4" it can not be always a specific string like "Page 1 of 4" or "Page 2 of 4" it can "Page 2 and 3 of 4"

This is a real case that I tried.

For me it happened when I changed size of toolbars (height), then view was different and it reported different state fro "currently visible pages"

Darshan-upadhyay1110 · 2024-04-05T13:28:54Z

Please rebase on top of recent w2ui rework to be sure it still works.

Note:
cypress_test/integration_tests/desktop/writer/scrolling_spec.js seems to be a strange test case. as a locally i tried the same document which is used in this document and followed the same steps.
I can see in status bar there is a possibility to get "Page 1 and 2 of 4" it can not be always a specific string like "Page 1 of 4" or "Page 2 of 4" it can "Page 2 and 3 of 4"
This is a real case that I tried.

For me it happened when I changed size of toolbars (height), then view was different and it reported different state fro "currently visible pages"
Rebased
No change, still that case is failed.

eszkadev · 2024-04-05T13:57:28Z

Are you sure you didn't change any toolbar size? even 1 px, please check canvas size used in that test

eszkadev · 2024-04-26T13:28:06Z

Unit Tests and Cypress Tests against core co-24.04 — FAILURE is old entry with all-in-one cypress - ignore

eszkadev

one commit has name: WIP Please Darshan take this: Calc: Mobile: top toolbar and formula could you rename it or squash with other?
So it will not look like mistake.

Darshan-upadhyay1110 · 2024-04-29T17:13:42Z

one commit has name: WIP Please Darshan take this: Calc: Mobile: top toolbar and formula could you rename it or squash with other?
So it will not look like mistake.

Done

eszkadev

ok, then I will leave final decision for @pedropintosilva as it is mainly css change

Darshan-upadhyay1110 · 2024-04-30T07:15:46Z

Tested mobile test case all are passing in local build.

- For the editor in the home view, a layout table is used, and it includes the use of IDs. This can potentially lead to issues with the interpretation of content by assistive technologies - Layout tables are meant to structure the layout of a page and should not contain structural markup like th , caption , summary , headers , or id. Solution: - replace `td` and 'table' tag with `div` - make some changes on css to be consistent with our prev design Signed-off-by: Darshan-upadhyay1110 <darshan.upadhyay@collabora.com> Change-Id: I136cd8d51b2e5035c6ef4292f759f49519e5bd61

- we changed table structure of `toolbar-wrapper` with `div` - so we also need to consider mobile view - made some neccessary changes for mobileview because of structure change in cool.html.m4 ('toolbar-wrapper') Signed-off-by: Darshan-upadhyay1110 <darshan.upadhyay@collabora.com> Change-Id: I91cd2c3ffbb0d625a78852398f6773136e9a7f6b

No need to specifically set width and height for .menuwizard-opened state. Best to set only one time those dimensions and just inherit from #toolbar-hamburger no matter the additional css class Signed-off-by: Pedro Pinto Silva <pedro.silva@collabora.com> Change-Id: I51eafc6c9b05b843d800b40a990aefd912e463f8

With "Change table layout to CSS base table structure" and "Adjust table elements for mobile view." changes we now have more divs instead of table elements but with that the hamburger menu is now mislaigned - Fix height - To do: ideally we would make both hamburger menu and the button at its side rendered with the exact same structure which is not the case. And ultimately remove height and just have it flex stretch etc Signed-off-by: Pedro Pinto Silva <pedro.silva@collabora.com> Change-Id: Id9365df82f1caff132dde41760a06689ec1145d1

Pedro : This changes from flex to grid (so we can have multiple columns and 2 rows) Darshan: - adding one more div and wrap all child of toolbar-wrapper execpt 'formulabr' will work here - i have tested the css grid approch it is breaking the mobile UI in Calc - here i have changed a bit in html and twiked css which covers all cases Signed-off-by: Pedro Pinto Silva <pedro.silva@collabora.com> Change-Id: Ia50fda95e2bef57d707bb4be1dd34e2ec083bfc5

To make consistent all toolbar element height with prev version where we were using w2Ui lib - i have added height to table-row element same as we had in prev version - remove fix height for toolbar-up which is not needed now - this fix height will effect the bg-color in toolbar-up section Signed-off-by: Darshan-upadhyay1110 <darshan.upadhyay@collabora.com> Change-Id: I7b307a6a72968ed81fe2eb8663c430f11e3b3ced

pedropintosilva

thanks, there are still other things that need fix/improvement but lets get this merged and I will send another PR and ask your testing @Darshan-upadhyay1110

Darshan-upadhyay1110 · 2024-05-02T12:52:41Z

still other things that need fix/improvement but lets get this merged and I will send another PR and ask your testing @Darshan-upadhyay1110

Thanks :)

pedropintosilva · 2024-05-02T12:54:31Z

browser/html/cool.html.m4

+          <div id="toolbar-hamburger">
+            <label class="main-menu-btn" for="main-menu-state">
+              <span class="main-menu-btn-icon" id="main-menu-btn-icon"></span>
+            </label>
+          </div>


As another follow up we should probably simplify this to have just a simple button with icon

please prepare only after my PR

Darshan-upadhyay1110 requested review from eszkadev and pedropintosilva March 20, 2024 16:14

Darshan-upadhyay1110 changed the title ~~Accesschange table layout in toolbar~~ Accessibility: change table layout in toolbar Mar 20, 2024

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch 3 times, most recently from 6e3e9bc to 5842fa0 Compare March 21, 2024 06:37

eszkadev reviewed Mar 21, 2024

View reviewed changes

browser/src/control/Control.MobileTopBar.js Outdated Show resolved Hide resolved

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch from 5842fa0 to 317be6a Compare March 21, 2024 07:33

eszkadev reviewed Mar 21, 2024

View reviewed changes

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch from 317be6a to 896d474 Compare March 21, 2024 10:44

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch 2 times, most recently from d7668b7 to d1a6c6f Compare March 23, 2024 06:58

eszkadev force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch from d1a6c6f to 69c6a4b Compare March 25, 2024 06:15

eszkadev approved these changes Mar 25, 2024

View reviewed changes

pedropintosilva requested changes Mar 28, 2024

View reviewed changes

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch from 019a17e to 1bac95b Compare April 1, 2024 05:47

Darshan-upadhyay1110 requested review from eszkadev and pedropintosilva April 1, 2024 05:48

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch 3 times, most recently from bc1064a to 4da6cf6 Compare April 3, 2024 05:36

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch 2 times, most recently from b7c6380 to e875759 Compare April 5, 2024 11:50

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch from e875759 to 105a9c4 Compare April 10, 2024 10:11

eszkadev closed this Apr 26, 2024

eszkadev reopened this Apr 26, 2024

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch 2 times, most recently from 22b9f79 to 9991d09 Compare April 29, 2024 16:42

eszkadev requested changes Apr 29, 2024

View reviewed changes

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch from 9991d09 to 14a82c6 Compare April 29, 2024 17:13

eszkadev approved these changes Apr 29, 2024

View reviewed changes

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch from 14a82c6 to a8e4b49 Compare April 29, 2024 17:37

Darshan-upadhyay1110 and others added 5 commits April 30, 2024 12:46

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch from a8e4b49 to 20ddde4 Compare April 30, 2024 07:16

Darshan-upadhyay1110 force-pushed the accessibility/darshan/change-table-layout-in-toolbar branch from 20ddde4 to e96e22d Compare April 30, 2024 09:49

pedropintosilva approved these changes May 2, 2024

View reviewed changes

pedropintosilva reviewed May 2, 2024

View reviewed changes

pedropintosilva merged commit e88d934 into master May 2, 2024
14 checks passed

pedropintosilva deleted the accessibility/darshan/change-table-layout-in-toolbar branch May 2, 2024 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessibility: change table layout in toolbar #8587

Accessibility: change table layout in toolbar #8587

Darshan-upadhyay1110 commented Mar 20, 2024

eszkadev left a comment

eszkadev commented Mar 22, 2024

eszkadev commented Mar 25, 2024

pedropintosilva left a comment

pedropintosilva commented Mar 28, 2024

Darshan-upadhyay1110 commented Mar 29, 2024

eszkadev commented Mar 30, 2024

Darshan-upadhyay1110 commented Apr 1, 2024 •

edited

Darshan-upadhyay1110 commented Apr 1, 2024

eszkadev commented Apr 1, 2024 •

edited

Darshan-upadhyay1110 commented Apr 1, 2024

eszkadev commented Apr 4, 2024

Darshan-upadhyay1110 commented Apr 5, 2024

eszkadev commented Apr 5, 2024

eszkadev commented Apr 26, 2024

eszkadev left a comment

Darshan-upadhyay1110 commented Apr 29, 2024 •

edited

eszkadev left a comment

Darshan-upadhyay1110 commented Apr 30, 2024

pedropintosilva left a comment

Darshan-upadhyay1110 commented May 2, 2024

pedropintosilva May 2, 2024

pedropintosilva May 2, 2024

Darshan-upadhyay1110 May 2, 2024

Accessibility: change table layout in toolbar #8587

Accessibility: change table layout in toolbar #8587

Conversation

Darshan-upadhyay1110 commented Mar 20, 2024

eszkadev left a comment

Choose a reason for hiding this comment

eszkadev commented Mar 22, 2024

eszkadev commented Mar 25, 2024

pedropintosilva left a comment

Choose a reason for hiding this comment

pedropintosilva commented Mar 28, 2024

Darshan-upadhyay1110 commented Mar 29, 2024

eszkadev commented Mar 30, 2024

Darshan-upadhyay1110 commented Apr 1, 2024 • edited

Darshan-upadhyay1110 commented Apr 1, 2024

eszkadev commented Apr 1, 2024 • edited

Darshan-upadhyay1110 commented Apr 1, 2024

eszkadev commented Apr 4, 2024

Darshan-upadhyay1110 commented Apr 5, 2024

eszkadev commented Apr 5, 2024

eszkadev commented Apr 26, 2024

eszkadev left a comment

Choose a reason for hiding this comment

Darshan-upadhyay1110 commented Apr 29, 2024 • edited

eszkadev left a comment

Choose a reason for hiding this comment

Darshan-upadhyay1110 commented Apr 30, 2024

pedropintosilva left a comment

Choose a reason for hiding this comment

Darshan-upadhyay1110 commented May 2, 2024

pedropintosilva May 2, 2024

Choose a reason for hiding this comment

pedropintosilva May 2, 2024

Choose a reason for hiding this comment

Darshan-upadhyay1110 May 2, 2024

Choose a reason for hiding this comment

Darshan-upadhyay1110 commented Apr 1, 2024 •

edited

eszkadev commented Apr 1, 2024 •

edited

Darshan-upadhyay1110 commented Apr 29, 2024 •

edited