Broken mobile toolbar on pages with non-responsive tables. [#3392922]

Comment	File	Size	Author
	Screenshot_2023-10-10-08-17-31-446_com.android.chrome.jpg	87.71 KB	finnsky

Comment #1

10 October 2023 at 12:28

finnsky created an issue. See original summary.

Log in or register to post comments

Comment #2

20 October 2023 at 11:02

vensires made their first commit to this issue’s fork.

Log in or register to post comments

Comment #3

20 October 2023 at 12:10

vensires opened merge request !100

Log in or register to post comments

Comment #4

finnsky commented 20 October 2023 at 12:16

From my point of view this kind of change should be at least scoped in:

1. Only mobile breakpoint
2. Only tables in content area.

Also probably we need to use
https://www.npmjs.com/package/tua-body-scroll-lock

It is part of core libraries. So can be useful here.

Log in or register to post comments

Comment #5

vensires

Greece

commented 20 October 2023 at 12:26

Status:

Active

» Needs review

@finnsky, I have just pushed what could be a solution.
Can you please review it in https://git.drupalcode.org/project/navigation/-/merge_requests/100.diff?

In general, when the user has zoomed out in ex. /admin/content and the navigation menu gets expanded, the tables in the body get limited to the viewport's width which actually causes something like a zoom-in, though not exactly.

Log in or register to post comments

Comment #6

20 October 2023 at 14:41

rfmarcelino made their first commit to this issue’s fork.

Log in or register to post comments

Comment #7

20 October 2023 at 15:26

vensires closed merge request !100

Log in or register to post comments

Comment #8

rfmarcelino commented 20 October 2023 at 15:28

Status:

Needs review

» Reviewed & tested by the community

Tested and working as intended

Log in or register to post comments

Comment #9

20 October 2023 at 15:33

rfmarcelino opened merge request !103

Log in or register to post comments

Comment #10

finnsky commented 20 October 2023 at 15:38

Status:

Reviewed & tested by the community

» Needs work

Re: #5

I think it is not really secure change.

1. `table` may appear in lot of place inside body. Old school frontenders still use them in layouts :)
2. We need it only for mobile. (scope needed)
3. Maybe we can find some selector to identify that tables.
4. Scroll lock still an option.

Log in or register to post comments

Comment #11

vensires

Greece

commented 20 October 2023 at 22:37

Regarding point (3) we can't really find a more specific selector.

The default /admin/content provides the following HTML:

<table class="views-table views-view-table cols-7 responsive-enabled sticky-enabled" data-once="tableresponsive table-select">

and we also have its sticky header:

<table class="sticky-header" style="visibility: hidden; position: fixed; top: 0px; left: 312px; width: 1425px;">

We could target the "views-table" and "sticky-header" classes specifically, but this would beat the purpose of what we decided today that the module should provide robust code even if the frontend theme does crazy things and, of course, not all tables are views tables and not all tables are responsive tables.

I will fix the mobile scope issue in a future commit.