🇺🇸United States @bronzehedwick

Here's a list of expected screen readers keyboard shortcuts, which has a section on autocomplete results.

I did some initial investigation of the code, and it looks like it's using jQuery Autocomplete under the hood. jQuery Autocomplete is keyboard navigable with the arrow keys out of the box, so seems likely that this module is breaking that somehow. Maybe something in the uiAutocomplete.options.select override?

bronzehedwick → created an issue.

volkswagenchick → credited bronzehedwick → .

Ah ha, yep, can confirm this is fixed in 1.22. Thanks @johnv and @geekgnr!

bronzehedwick → created an issue.

I can confirm the provided Tugboat link is loading Tabled correctly, and seems all in order. I think this is a easy win, so happy to mark RBTC, and we can create a new issue if data makes sense to add.

ckrina → credited bronzehedwick → .

@penyaskito I pushed a fix for the issue you raised. Looks like it just needed a small width tweak.

Looks good, thanks @javi-er!

Thanks @javi-er! Have this in my queue to review tomorrow.

bronzehedwick → created an issue.

penyaskito → credited bronzehedwick → .

Tested locally and Dashboard theme looks good in Gin and Claro.

Sounds good, thanks @penyaskito!

Tested for regressions locally, and everything is looking good. Thanks @javi-er!

Gotcha, thanks @javi-er! I re-tested following your steps, and things are working as expected! I had to clear cache, but I think that's normal.

Re-tested and it's working great. Thanks @javi-er!

Thanks @javi-er!

I tested this functionally and am still seeing the issue.

My Steps

1. Check out this branch
2. Copy the module into a fresh Drupal install, inside themes/contrib
3. Enable the module: ddev drush en -y ckeditor_responsive_table
4. Add the "Responsive Table" and "Table" buttons to the Basic text filter
5. Create a basic page
6. Insert a responsive table
7. Insert a regular table
8. Observe both are getting the responsive table enhancements

Is there something I'm missing?

Hey @javi-er, thank you! After this update, I am seeing the caption be visually hidden if toggled off, and shown if not.

However, if I try to change the setting from visible to hidden or vice versa after I've saved the node, the option doesn't change. I think this is a big improvement, though, so fine if we want to handle that in a separate issue.

Tested a confirmed the solution works as expected. once() is implemented correctly as well.

I'm seeing almost the opposite.

1. When I set the caption to be visible, it stays visible
2. When I set the caption to be hidden, is also stays visible (no visually-hidden class is applied)
3. After setting the caption to be hidden, and editing the node again, the editor says the caption is visible

Since the caption is always visible, and the editor always reflects that, my guess is that the caption visible state isn't being saved properly, and that could account for the display issue. Interesting that this is essentially the reverse of what you're seeing @delmarr. Are you still seeing the behavior you're describing in this ticket?

bronzehedwick → created an issue.

bronzehedwick → created an issue.

Looks good to me! Tested locally and MR behaves as expected.

Awesome, thanks @javi-er!

Is there any harm in just merging this?

@javi-er Finished addressing the feedback you provided. Let me know if there are other changes needed!

Looks good to me @javier! I tested locally and this behaves as expected. Good to merge IMO.

Thanks @javier! I pushed fixes for the issues you raised, aside from the ones needed in follow ups.

bronzehedwick → created an issue.

This issue is fixed by https://git.drupalcode.org/project/ckeditor_responsive_table/-/merge_req...

@javier I pushed a two-fold fix:

1. Remove yarn dependency, tracking the tabled library inside this repository. This removes the need for module users to install yarn.
2. Add a configuration page for end users to change the CSS selector which applies tabled, along with a sensible default.

After some research here, I actually think the best way to include the tabled dependency is through package.json, instead of through composer. The reason is two fold.

1. There are already dependencies managed in package.json, so it will keep everything centrally managed.
2. The composer installation pattern requires a different composer command via the composer merge plugin, which is built for optional dependencies. While the dependency could be optional, I'd argue that this would create more confusion and work for 99% of the use cases, where you just want a fully responsive and accessible table element out of the box.

bronzehedwick → created an issue.

bronzehedwick → created an issue.

bronzehedwick → created an issue.

Thanks for all the work on this!

I applied the patch, and I'm not getting the chosen environment color in the side strip or the environment menu. I dumped the variables and it looks like they're still set to the defaults. Can confirm caches are rebuilt.

+1

Also, it would be great if this MR didn't change any SVG file that doesn't really need to change to make it easier to isolate the changes.

Sorry about that @ckrina, those changes snuck in due to IDE automatically inserted newlines at the end of the file. I've pushed a commit to revert that, so those changes should no longer be in the diff.

@SKAUGHT aria-hidden="true" is applied to the .toolbar-button__abbreviation element. That should handle the use case you're mentioning.

@smustgrave I added an abbreviation test. It's my first time writing a Drupal test, so open to any feedback. Thanks!

bronzehedwick → changed the visibility of the branch 3424744-on-collapsed-sidebars to hidden.

bronzehedwick → changed the visibility of the branch 3424744-on-collapsed-sidebars to hidden.

lauriii → credited bronzehedwick → .

Fixed by https://www.drupal.org/project/navigation/issues/3436130 📌 Ensure keyboard navigation works with the drawer Active

Fixed by https://www.drupal.org/project/navigation/issues/3436130 📌 Ensure keyboard navigation works with the drawer Active

Fixed by https://www.drupal.org/project/navigation/issues/3436130 📌 Ensure keyboard navigation works with the drawer Active

Got it @SKAUGHT, I pushed a commit that hard codes 40 as the width and height.

moving width/height to var seems not needed [as this doesn't recenter, re-size the actual paths]

This will re-size/re-center the entire graphic, as intended.

All width/height sizing and any drawing code inside an SVG is relative to the viewBox. So the width and height on <rect fill="#347efe" width="32" height="32" rx="8"/> being set to 32 is the same as saying 100% of the width and height, since the viewBox is 32/32.

Increasing the width/height on the <svg> element scales everything inside it based on the ratio of the viewBox, aka the S in SVG ;)

also noticed viewbox is "0 0 32 32" -- i think should be 40

As per above, the SVG is displayed at 40/40, but it's scaling baseline is 32/32 (aka, the viewBox). Changing the viewBox from 32/32 will make the internal scaling math off, in this case, adding extra white space around the SVG, optically shrinking it.

You can test all this by changing the width and height values on the <svg> element.

I also think we need to rework current events system.

They should be better documented and all iterations click/hovers/keys should go in same manner.
Since only events cares globally about multiple things. EG

- Switch expand/collapse text
- Switch classes
- Controls sibling elements open/close

@finnsky would this be appropriate for a follow up ticket?

Ah that makes sense, thanks @SKAUGHT. I pushed a commit that adds back that condition.

I created a new MR with a new approach. Is this more inline with what you're expecting @ckrina?

@finnsky that last commit fixed the issue. Thanks!

This approach also resolves the follow up issues, listed below.

• Hover effect can cause problems with keyboard focus 📌 Hover effect can cause problems with keyboard focus Active
• Improv keyboard navigation focus inside drawer 📌 Improv keyboard navigation inside drawer Active
• Third level menu accordions are not navigable via the arrow keys 📌 Third level menu accordions are not navigable via the arrow keys Active

Moving to RTBC.

Thanks @finnsky! Your updates addressed all my feedback.

There is one small, new regression, I think from the 1.x rebase. There's extra padding and grey bar below each third level accordion.

This seems to be caused by the recently merged additional padding MR 🐛 Bottom padding needs to be increased along gray container box of grandchildren menu items Fixed running up against the new styles.

One solution would be to change line 38 of toolbar-menu.pcss.css from:

.toolbar-menu--level-2 {

to:

.toolbar-menu--level-2:not([inert]) {

I think this is a reasonable approach, removing the extra network request. We don't get the added styling benefits, but those don't have a plan to be taken advantage of right now, and this will remove refactoring. Marking RBTC!

I think this issue has been fixed by #3440228 🐛 Bottom padding needs to be increased along gray container box of grandchildren menu items Fixed . Is that right @jwitkowski79?

Looks good! I tested the MR locally, and found no regressions. I searched the code base for any references to user.jpg, and found none.

Hey @finnsky, thanks for this! Some things I noticed.

• When closing a popover with left arrow, focus moves up to the previous menu item, instead of moving back to the popover's button.
• Pressing right arrow inside an open popover, the the popover is dismissed and focus is moved to the next main menu item below the current item. I would expect nothing to happen.
• When inside a third level menu (accordion), there's no way to move out of it.
  • Pressing either arrow left or right dismiss the entire popover.
  • Pressing Space scrolls the page.

  I would expect pressing arrow left (and maybe arrow right?) or Space to collapse the accordion, moving focus back to the top level of the accordion.

• After initially opening a popover, pressing up arrow dismisses the popover. I would expect the focus to move to the last menu item in the popover.
• When focus is on a collapsed third level accordion, pressing left arrow does nothing. I would expect the popover to be dismissed.

bronzehedwick → created an issue.

Thanks for catching @ckrina! I've updated the MR.

bronzehedwick → made their first commit to this issue’s fork.

bronzehedwick → created an issue.

Moving to NR, given @ckrina's guidance, above. The follow up issues are below.

1. Improv keyboard navigation focus inside drawer 📌 Improv keyboard navigation inside drawer Active
2. Hover effect can cause problems with keyboard focus 📌 Hover effect can cause problems with keyboard focus Active

bronzehedwick → created an issue.

Pushed a fix for the inaccessible Shortcuts menu via arrow keys. Simple oversight, just needed to add tabindex.

Looks like the problem here is the hover. The hover effect is controlled in JavaScript, and has a delay on toggling (by design). The hover off is triggered when you hover over another menu item. That means that if you quickly hover over a few items, then move your mouse outside the menu drawer, the hovered items will stay hovered. This blocks arrow movement, unless you press arrow left to "dismiss" the (hidden) popover.

I haven't been able to reproduce the JS error @rkoller and @finnsky are seeing yet.

This is an architectural issue, and will probably require some larger refactoring to resolve.

I think this and the other remaining issues should be moved into the follow up ticket 📌 Improv keyboard navigation inside drawer Active , to reduce churn while the core MR is being made. Does that make sense with what you are thinking @ckrina?