Skip to content

fix: update field editing focus outline style #643

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.

Sign up for GitHub

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

Merged
BenHenning merged 6 commits into from
Jul 8, 2025
Merged

fix: update field editing focus outline style #643

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1234c9
Update field editing focus outline style
microbit-grace Jul 7, 2025
a3c9562
Merge branch 'main' into field-edit-focus-style
microbit-matt-hillsdon Jul 8, 2025
478cf40
chore: comment CSS, remove readded duplicate selector
microbit-matt-hillsdon Jul 8, 2025
250494c
fix: correct use of focus-within
microbit-matt-hillsdon Jul 8, 2025
39dbe81
chore: consistently use focus-within
microbit-matt-hillsdon Jul 8, 2025
3ea518a
chore: remove redundant case
microbit-matt-hillsdon Jul 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 29 additions & 3 deletions test/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,15 +115,30 @@
stroke: var(--blockly-active-node-color);
stroke-width: var(--blockly-selection-width);
}
.blocklyKeyboardNavigation
.blocklyKeyboardNavigation:not(
Copy link
Collaborator

@BenHenning BenHenning Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this & the other CSS: I think this is starting to get quite complicated to follow, and would benefit from at least comments for each new section explaining briefly what it's intending to do (even if some of them can just say 'look at the same selector above for an explanation').

Separately, if there's time it may well be worth adding tests for some or all of these cases. Without explicitly verifying CSS, I suspect it has a decent chance of breaking at some point in the future (and probably being really unclear to whoever breaks it what they should be testing).

Copy link
Contributor Author

@microbit-matt-hillsdon microbit-matt-hillsdon Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agreed, Grace and I will do both of these but let's do it as a follow up after sorting out how this change relates to #637

I think if I go through it and add more comments now I'll make that harder to sort out whichever way we decide.

Please make a judgement as to whether it's best to merge this now and rework that PR to include this change or hold off for that to land and rework this one.

Raised #645, please feel free to assign to me (if possible).

Copy link
Collaborator

@maribethb maribethb Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@microbit-matt-hillsdon @rachel-fenichel is going to address aaron's comment on #637 since Christopher has gone home for the day, that way we can get that submitted today and unblock this PR.

Copy link
Contributor Author

@microbit-matt-hillsdon microbit-matt-hillsdon Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
We've merged in main and moved our changes to the index.ts file.
I've also added comments to the CSS. We'll look at testing the styles in a separate PR.

:has(
.blocklyDropDownDiv > .blocklyDropDownContent > *,
.blocklyWidgetDiv > *
)
)
.blocklyPassiveFocus:is(
.blocklyPath:not(.blocklyFlyout .blocklyPath),
.blocklyHighlightedConnectionPath
),
.blocklyKeyboardNavigation
.blocklyKeyboardNavigation:not(
:has(
.blocklyDropDownDiv > .blocklyDropDownContent > *,
.blocklyWidgetDiv > *
)
)
.blocklyPassiveFocus.blocklyField
> .blocklyFieldRect,
.blocklyKeyboardNavigation
.blocklyKeyboardNavigation:not(
:has(
.blocklyDropDownDiv > .blocklyDropDownContent > *,
.blocklyWidgetDiv > *
)
)
.blocklyPassiveFocus.blocklyIconGroup
> .blocklyIconShape:first-child {
stroke: var(--blockly-active-node-color);
Expand Down Expand Up @@ -154,6 +169,17 @@
.blocklyWorkspaceFocusRing,
.blocklyKeyboardNavigation
.blocklyWorkspace.blocklyActiveFocus
.blocklyWorkspaceFocusRing,
.blocklyKeyboardNavigation
.blocklyWorkspace.blocklyActiveFocus
.blocklyWorkspaceFocusRing,
.blocklyKeyboardNavigation:has(
.blocklyDropDownDiv > .blocklyDropDownContent > *
):focus-within
.blocklyWorkspace
.blocklyWorkspaceFocusRing,
.blocklyKeyboardNavigation:has(.blocklyWidgetDiv > *):focus-within
.blocklyWorkspace
.blocklyWorkspaceFocusRing {
stroke: var(--blockly-active-tree-color);
stroke-width: calc(var(--blockly-selection-width) * 2);
Expand Down