Search code, repositories, users, issues, pull requests...

src/static/js/pad_editbar.ts[329-356]
src/static/js/pad_editbar.ts[231-246]

+    // Escape from inside any open popup: close the popup and let
+    // toggleDropDown('none') restore focus to the trigger.
+    if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) {
+      this.toggleDropDown('none');
+      evt.preventDefault();
+      return;
+    }

Evidence

_bodyKeyEvent() now intercepts Escape when focus is inside any open .popup.popup-show and
invokes toggleDropDown('none'). However, toggleDropDown('none') continues early for
thisModuleName === 'users', so #users is never closed by Escape.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Escape-to-close is implemented by calling `toggleDropDown('none')`, but the `none` branch skips closing the `users` popup. As a result, Escape does nothing for the Users dialog.
### Issue Context
`toggleDropDown('none')` currently contains `if (thisModuleName === 'users') continue;`.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[231-246]
- src/static/js/pad_editbar.ts[329-336]
### Suggested fix
Make Escape close `#users` as well (unless it is intentionally sticky):
- Remove the unconditional skip for `users`, OR
- Only skip `users` if it is in a sticky state (e.g. `#users.hasClass('stickyUsers')`), OR
- Add a dedicated “close all popups” helper that includes users and call that from the Escape handler.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Chat label overridden by l10n~~ ☑ 🐞 Bug ≡ Correctness

Description

#chaticon has an explicit aria-label="Open chat" and title="Chat (Alt C)", but adding
data-l10n-id="pad.chat.title" causes html10n to overwrite both title and aria-label with the
localized string at runtime, making the label inconsistent and potentially breaking tests that
assert the initial aria-label.

Code

src/templates/pad.html[R383-387]

+        <button type="button" id="chaticon" class="visible" title="Chat (Alt C)" aria-label="Open chat" data-l10n-id="pad.chat.title">
       <span id="chatlabel" data-l10n-id="pad.chat"></span>
-            <span class="buttonicon buttonicon-chat"></span>
-            <span id="chatcounter">0</span>
-        </div>
+            <span class="buttonicon buttonicon-chat" aria-hidden="true"></span>
+            <span id="chatcounter" aria-label="Unread messages">0</span>
+        </button>

Evidence

Because the l10n key ends in .title, html10n treats it as a translatable title attribute and
(per its implementation) also sets aria-label to the same translated string whenever it applies a
translation to any non-textContent property. This will overwrite the hard-coded aria-label/title
in the markup.

src/templates/pad.html[383-387]
src/static/js/vendors/html10n.ts[643-667]
src/locales/en.json[160-160]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`#chaticon` sets `aria-label` and `title` in HTML, but also adds `data-l10n-id="pad.chat.title"`. html10n will overwrite the element’s `title` and `aria-label` after localization.
### Issue Context
In html10n, IDs that end with `.title` are treated as a `title` translation, and the translator sets `aria-label` to the same translation string.
### Fix Focus Areas
- src/templates/pad.html[383-387]
- src/static/js/vendors/html10n.ts[643-667]
### Suggested fix
Pick a single source of truth:
- Simplest: remove `data-l10n-id="pad.chat.title"` from `#chaticon` and keep the explicit `title`/`aria-label`.
- Or: remove the hard-coded `title`/`aria-label` and update the l10n string to include the shortcut text (and adjust tests to expect the localized value).
Avoid having both hard-coded attributes and a `data-l10n-id` that can overwrite them.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. ~~Show-more button unstyled~~ ☑ 🐞 Bug ⚙ Maintainability

Description

.show-more-icon-btn was changed from a ` to a `, but the toolbar CSS does not reset native
button styles (background/border/appearance), which can cause visible styling/layout regressions
across browsers.

Code

src/templates/pad.html[74-77]
src/static/css/pad/toolbar.css[100-110]

+      <button type="button" class="show-more-icon-btn" aria-label="Show more toolbar buttons" aria-expanded="false"></button> <!-- use on small screen to display hidden toolbar buttons -->

Evidence

The template now renders a ` for .show-more-icon-btn, but existing CSS for .toolbar
.show-more-icon-btn` only sets sizing/typography and does not clear user-agent button styles
(border, background, padding, appearance).

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A `<span>` was converted to a `<button>`, but the CSS does not reset default button styles, which can cause borders/background/padding to appear.
### Issue Context
The toolbar CSS styles `.show-more-icon-btn` as if it were a neutral inline element.
### Fix Focus Areas
- src/static/css/pad/toolbar.css[100-110]
- src/templates/pad.html[74-77]
### Suggested fix
Add a reset for `.toolbar .show-more-icon-btn` (and optionally focus-visible styling), e.g.:
- `background: transparent; border: 0; padding: 0; color: inherit; font: inherit; appearance: none;`
Keep the existing width/height/line-height so the `:after` icon stays centered.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

5. Tabnabbing via export links 🐞 Bug ⛨ Security

Description

The export links use target="_blank" but still lack rel="noopener noreferrer", which keeps
window.opener accessible to the opened page and enables reverse-tabnabbing if that page executes
script.

Code

src/templates/pad.html[218-235]
src/templates/pad.html[183-186]

+              <a id="exportetherpada" target="_blank" class="exportlink" aria-label="Export as Etherpad">
+                <span class="exporttype buttonicon buttonicon-file-powerpoint" id="exportetherpad" data-l10n-id="pad.importExport.exportetherpad" aria-hidden="true"></span>
         </a>
-              <a id="exporthtmla" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-code" id="exporthtml" data-l10n-id="pad.importExport.exporthtml"></span>
+              <a id="exporthtmla" target="_blank" class="exportlink" aria-label="Export as HTML">

Evidence

All export links open in a new tab but do not set rel="noopener noreferrer". The same file already
demonstrates the safer pattern (rel="noopener") for external links, indicating the intended
convention.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Links with `target="_blank"` should include `rel="noopener noreferrer"` to prevent reverse-tabnabbing.
### Issue Context
The export links (`#export*`) use `target="_blank"` and no `rel`.
### Fix Focus Areas
- src/templates/pad.html[218-235]
### Suggested fix
Add `rel="noopener noreferrer"` to each export `<a ... target="_blank" ...>`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

View more (18)

6. ~~Escape won’t close userlist~~ ☑ 🐞 Bug ≡ Correctness

Description

The new Escape path calls toggleDropDown('none'), but toggleDropDown('none') intentionally skips the
'users' dropdown and #users is not a .toolbar-popup element. Escape pressed while focus is inside
the Users popup therefore won’t close it or restore focus to the trigger.

Code

src/static/js/pad_editbar.ts[329-336]
src/static/js/pad_editbar.ts[219-246]
src/templates/pad.html[348-376]

+    // Escape from inside any open popup: close the popup and let
+    // toggleDropDown('none') restore focus to the trigger.
+    if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) {
+      this.toggleDropDown('none');
+      evt.preventDefault();
+      return;
+    }

Evidence

Escape handling always routes to toggleDropDown('none') when focus is inside an open popup, but
toggleDropDown('none') skips closing the users dropdown. Because the Users popup is rendered as
class="popup" (no toolbar-popup class), the earlier $('.toolbar-popup').removeClass('popup-show')
line does not affect it, leaving it open on Escape.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Escape-to-close calls `toggleDropDown('none')`, but that code path skips closing the `users` dropdown, so Escape won’t dismiss the Users popup.
### Issue Context
- `toggleDropDown('none')` currently has a hard-coded `if (thisModuleName === 'users') continue;`.
- `#users` in the template has `class="popup"` (no `toolbar-popup`), so `$('.toolbar-popup').removeClass('popup-show')` does not close it.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[211-246]
- src/static/js/pad_editbar.ts[329-336]
- src/templates/pad.html[348-376]
### Suggested fix
- In the `moduleName === 'none'` branch, close `users` unless it is sticky:
- Replace the unconditional skip with something like:
- `if (thisModuleName === 'users' && $('#users').hasClass('stickyUsers')) continue;`
- otherwise remove `popup-show` from `#users` the same way as other dropdowns.
- Alternatively, in the Escape handler, detect `#users.popup-show` and explicitly close it (again respecting `stickyUsers`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

7. ~~Show-more button default styling~~ ☑ 🐞 Bug ⚙ Maintainability

Description

The show-more control changed from a span to a button, but there is no CSS reset for native button
border/background/padding. When it becomes visible in cropped-toolbar mode, it can render with
browser-default button chrome and spacing.

Code

+      <button type="button" class="show-more-icon-btn" aria-label="Show more toolbar buttons" aria-expanded="false"></button> <!-- use on small screen to display hidden toolbar buttons -->

Evidence
The template now emits a ` for .show-more-icon-btn`. The existing CSS sets sizing/typography but
does not reset default button styles (border/background/padding/appearance), which were irrelevant
when the element was a ``.
src/templates/pad.html[64-78]
src/static/css/pad/toolbar.css[100-110]
src/static/skins/colibris/src/components/toolbar.css[116-124]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`.show-more-icon-btn` is now a `<button>` and may pick up browser default border/background/padding unless explicitly reset.
### Issue Context
The existing toolbar CSS styles `.show-more-icon-btn` (size, line-height, etc.) but does not neutralize native button UI. Previously this was a `<span>` so it had no native chrome.
### Fix Focus Areas
- src/templates/pad.html[64-78]
- src/static/css/pad/toolbar.css[100-110]
- src/static/skins/colibris/src/components/toolbar.css[116-124]
### Suggested fix
Add a reset for `.toolbar .show-more-icon-btn` (in both the default toolbar CSS and colibris skin if needed), for example:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

8. Blank-target export lacks noopener 🐞 Bug ⛨ Security

Description

The export links still use target="_blank" without rel="noopener noreferrer", allowing the opened
page to access window.opener. This enables tabnabbing/opener navigation if the opened document
executes script.

Code

+              <a id="exportetherpada" target="_blank" class="exportlink" aria-label="Export as Etherpad">
+                <span class="exporttype buttonicon buttonicon-file-powerpoint" id="exportetherpad" data-l10n-id="pad.importExport.exportetherpad" aria-hidden="true"></span>
        </a>
-              <a id="exporthtmla" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-code" id="exporthtml" data-l10n-id="pad.importExport.exporthtml"></span>
+              <a id="exporthtmla" target="_blank" class="exportlink" aria-label="Export as HTML">

Evidence
The PR touched the export link markup and kept target="_blank" but did not add `rel="noopener
noreferrer". The codebase already uses rel="noopener" on other target="_blank"` links,
indicating this protection is expected.
src/templates/pad.html[218-235]
src/node/handler/ExportHandler.ts[58-87]
src/templates/pad.html[183-186]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`target="_blank"` links without `rel="noopener noreferrer"` expose `window.opener` to the new tab.
### Issue Context
The export links in the import/export dialog open in a new tab and were modified in this PR. Other links in the same template already include `rel="noopener"`, so adding it here is consistent.
### Fix Focus Areas
- src/templates/pad.html[218-235]
### Suggested fix
Add `rel="noopener noreferrer"` to each export `<a ... target="_blank" ...>` element (all six). If you need referrer behavior, keep it consistent with existing link policies.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

9. ~~Escape won’t close userlist~~ ☑ 🐞 Bug ≡ Correctness

Description

The new Escape path calls toggleDropDown('none'), but toggleDropDown('none') intentionally skips the
'users' dropdown and #users is not a .toolbar-popup element. Escape pressed while focus is inside
the Users popup therefore won’t close it or restore focus to the trigger.

Code

src/static/js/pad_editbar.ts[329-336]
src/static/js/pad_editbar.ts[219-246]
src/templates/pad.html[348-376]

+    // Escape from inside any open popup: close the popup and let
+    // toggleDropDown('none') restore focus to the trigger.
+    if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) {
+      this.toggleDropDown('none');
+      evt.preventDefault();
+      return;
+    }

Evidence

Escape handling always routes to toggleDropDown('none') when focus is inside an open popup, but
toggleDropDown('none') skips closing the users dropdown. Because the Users popup is rendered as
class="popup" (no toolbar-popup class), the earlier $('.toolbar-popup').removeClass('popup-show')
line does not affect it, leaving it open on Escape.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Escape-to-close calls `toggleDropDown('none')`, but that code path skips closing the `users` dropdown, so Escape won’t dismiss the Users popup.
### Issue Context
- `toggleDropDown('none')` currently has a hard-coded `if (thisModuleName === 'users') continue;`.
- `#users` in the template has `class="popup"` (no `toolbar-popup`), so `$('.toolbar-popup').removeClass('popup-show')` does not close it.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[211-246]
- src/static/js/pad_editbar.ts[329-336]
- src/templates/pad.html[348-376]
### Suggested fix
- In the `moduleName === 'none'` branch, close `users` unless it is sticky:
- Replace the unconditional skip with something like:
- `if (thisModuleName === 'users' && $('#users').hasClass('stickyUsers')) continue;`
- otherwise remove `popup-show` from `#users` the same way as other dropdowns.
- Alternatively, in the Escape handler, detect `#users.popup-show` and explicitly close it (again respecting `stickyUsers`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

10. ~~Show-more button default styling~~ ☑ 🐞 Bug ⚙ Maintainability

Description

The show-more control changed from a span to a button, but there is no CSS reset for native button
border/background/padding. When it becomes visible in cropped-toolbar mode, it can render with
browser-default button chrome and spacing.

Code

+      <button type="button" class="show-more-icon-btn" aria-label="Show more toolbar buttons" aria-expanded="false"></button> <!-- use on small screen to display hidden toolbar buttons -->

Evidence
The template now emits a ` for .show-more-icon-btn`. The existing CSS sets sizing/typography but
does not reset default button styles (border/background/padding/appearance), which were irrelevant
when the element was a ``.
src/templates/pad.html[64-78]
src/static/css/pad/toolbar.css[100-110]
src/static/skins/colibris/src/components/toolbar.css[116-124]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`.show-more-icon-btn` is now a `<button>` and may pick up browser default border/background/padding unless explicitly reset.
### Issue Context
The existing toolbar CSS styles `.show-more-icon-btn` (size, line-height, etc.) but does not neutralize native button UI. Previously this was a `<span>` so it had no native chrome.
### Fix Focus Areas
- src/templates/pad.html[64-78]
- src/static/css/pad/toolbar.css[100-110]
- src/static/skins/colibris/src/components/toolbar.css[116-124]
### Suggested fix
Add a reset for `.toolbar .show-more-icon-btn` (in both the default toolbar CSS and colibris skin if needed), for example:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

11. Blank-target export lacks noopener 🐞 Bug ⛨ Security

Description

The export links still use target="_blank" without rel="noopener noreferrer", allowing the opened
page to access window.opener. This enables tabnabbing/opener navigation if the opened document
executes script.

Code

+              <a id="exportetherpada" target="_blank" class="exportlink" aria-label="Export as Etherpad">
+                <span class="exporttype buttonicon buttonicon-file-powerpoint" id="exportetherpad" data-l10n-id="pad.importExport.exportetherpad" aria-hidden="true"></span>
         </a>
-              <a id="exporthtmla" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-code" id="exporthtml" data-l10n-id="pad.importExport.exporthtml"></span>
+              <a id="exporthtmla" target="_blank" class="exportlink" aria-label="Export as HTML">

Evidence
The PR touched the export link markup and kept target="_blank" but did not add `rel="noopener
noreferrer". The codebase already uses rel="noopener" on other target="_blank"` links,
indicating this protection is expected.
src/templates/pad.html[218-235]
src/node/handler/ExportHandler.ts[58-87]
src/templates/pad.html[183-186]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`target="_blank"` links without `rel="noopener noreferrer"` expose `window.opener` to the new tab.
### Issue Context
The export links in the import/export dialog open in a new tab and were modified in this PR. Other links in the same template already include `rel="noopener"`, so adding it here is consistent.
### Fix Focus Areas
- src/templates/pad.html[218-235]
### Suggested fix
Add `rel="noopener noreferrer"` to each export `<a ... target="_blank" ...>` element (all six). If you need referrer behavior, keep it consistent with existing link policies.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

12. ~~Escape won’t close userlist~~ ☑ 🐞 Bug ≡ Correctness

Description

The new Escape path calls toggleDropDown('none'), but toggleDropDown('none') intentionally skips the
'users' dropdown and #users is not a .toolbar-popup element. Escape pressed while focus is inside
the Users popup therefore won’t close it or restore focus to the trigger.

Code

src/static/js/pad_editbar.ts[329-336]
src/static/js/pad_editbar.ts[219-246]
src/templates/pad.html[348-376]

+    // Escape from inside any open popup: close the popup and let
+    // toggleDropDown('none') restore focus to the trigger.
+    if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) {
+      this.toggleDropDown('none');
+      evt.preventDefault();
+      return;
+    }

Evidence

Escape handling always routes to toggleDropDown('none') when focus is inside an open popup, but
toggleDropDown('none') skips closing the users dropdown. Because the Users popup is rendered as
class="popup" (no toolbar-popup class), the earlier $('.toolbar-popup').removeClass('popup-show')
line does not affect it, leaving it open on Escape.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Escape-to-close calls `toggleDropDown('none')`, but that code path skips closing the `users` dropdown, so Escape won’t dismiss the Users popup.
### Issue Context
- `toggleDropDown('none')` currently has a hard-coded `if (thisModuleName === 'users') continue;`.
- `#users` in the template has `class="popup"` (no `toolbar-popup`), so `$('.toolbar-popup').removeClass('popup-show')` does not close it.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[211-246]
- src/static/js/pad_editbar.ts[329-336]
- src/templates/pad.html[348-376]
### Suggested fix
- In the `moduleName === 'none'` branch, close `users` unless it is sticky:
- Replace the unconditional skip with something like:
- `if (thisModuleName === 'users' && $('#users').hasClass('stickyUsers')) continue;`
- otherwise remove `popup-show` from `#users` the same way as other dropdowns.
- Alternatively, in the Escape handler, detect `#users.popup-show` and explicitly close it (again respecting `stickyUsers`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

13. ~~Show-more button default styling~~ ☑ 🐞 Bug ⚙ Maintainability

Description

The show-more control changed from a span to a button, but there is no CSS reset for native button
border/background/padding. When it becomes visible in cropped-toolbar mode, it can render with
browser-default button chrome and spacing.

Code

+      <button type="button" class="show-more-icon-btn" aria-label="Show more toolbar buttons" aria-expanded="false"></button> <!-- use on small screen to display hidden toolbar buttons -->

Evidence
The template now emits a ` for .show-more-icon-btn`. The existing CSS sets sizing/typography but
does not reset default button styles (border/background/padding/appearance), which were irrelevant
when the element was a ``.
src/templates/pad.html[64-78]
src/static/css/pad/toolbar.css[100-110]
src/static/skins/colibris/src/components/toolbar.css[116-124]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`.show-more-icon-btn` is now a `<button>` and may pick up browser default border/background/padding unless explicitly reset.
### Issue Context
The existing toolbar CSS styles `.show-more-icon-btn` (size, line-height, etc.) but does not neutralize native button UI. Previously this was a `<span>` so it had no native chrome.
### Fix Focus Areas
- src/templates/pad.html[64-78]
- src/static/css/pad/toolbar.css[100-110]
- src/static/skins/colibris/src/components/toolbar.css[116-124]
### Suggested fix
Add a reset for `.toolbar .show-more-icon-btn` (in both the default toolbar CSS and colibris skin if needed), for example:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

14. Blank-target export lacks noopener 🐞 Bug ⛨ Security

Description

The export links still use target="_blank" without rel="noopener noreferrer", allowing the opened
page to access window.opener. This enables tabnabbing/opener navigation if the opened document
executes script.

Code

+              <a id="exportetherpada" target="_blank" class="exportlink" aria-label="Export as Etherpad">
+                <span class="exporttype buttonicon buttonicon-file-powerpoint" id="exportetherpad" data-l10n-id="pad.importExport.exportetherpad" aria-hidden="true"></span>
          </a>
-              <a id="exporthtmla" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-code" id="exporthtml" data-l10n-id="pad.importExport.exporthtml"></span>
+              <a id="exporthtmla" target="_blank" class="exportlink" aria-label="Export as HTML">

Evidence
The PR touched the export link markup and kept target="_blank" but did not add `rel="noopener
noreferrer". The codebase already uses rel="noopener" on other target="_blank"` links,
indicating this protection is expected.
src/templates/pad.html[218-235]
src/node/handler/ExportHandler.ts[58-87]
src/templates/pad.html[183-186]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`target="_blank"` links without `rel="noopener noreferrer"` expose `window.opener` to the new tab.
### Issue Context
The export links in the import/export dialog open in a new tab and were modified in this PR. Other links in the same template already include `rel="noopener"`, so adding it here is consistent.
### Fix Focus Areas
- src/templates/pad.html[218-235]
### Suggested fix
Add `rel="noopener noreferrer"` to each export `<a ... target="_blank" ...>` element (all six). If you need referrer behavior, keep it consistent with existing link policies.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

15. ~~Escape won’t close userlist~~ ☑ 🐞 Bug ≡ Correctness

Description

The new Escape path calls toggleDropDown('none'), but toggleDropDown('none') intentionally skips the
'users' dropdown and #users is not a .toolbar-popup element. Escape pressed while focus is inside
the Users popup therefore won’t close it or restore focus to the trigger.

Code

src/static/js/pad_editbar.ts[329-336]
src/static/js/pad_editbar.ts[219-246]
src/templates/pad.html[348-376]

+    // Escape from inside any open popup: close the popup and let
+    // toggleDropDown('none') restore focus to the trigger.
+    if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) {
+      this.toggleDropDown('none');
+      evt.preventDefault();
+      return;
+    }

Evidence

Escape handling always routes to toggleDropDown('none') when focus is inside an open popup, but
toggleDropDown('none') skips closing the users dropdown. Because the Users popup is rendered as
class="popup" (no toolbar-popup class), the earlier $('.toolbar-popup').removeClass('popup-show')
line does not affect it, leaving it open on Escape.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Escape-to-close calls `toggleDropDown('none')`, but that code path skips closing the `users` dropdown, so Escape won’t dismiss the Users popup.
### Issue Context
- `toggleDropDown('none')` currently has a hard-coded `if (thisModuleName === 'users') continue;`.
- `#users` in the template has `class="popup"` (no `toolbar-popup`), so `$('.toolbar-popup').removeClass('popup-show')` does not close it.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[211-246]
- src/static/js/pad_editbar.ts[329-336]
- src/templates/pad.html[348-376]
### Suggested fix
- In the `moduleName === 'none'` branch, close `users` unless it is sticky:
- Replace the unconditional skip with something like:
- `if (thisModuleName === 'users' && $('#users').hasClass('stickyUsers')) continue;`
- otherwise remove `popup-show` from `#users` the same way as other dropdowns.
- Alternatively, in the Escape handler, detect `#users.popup-show` and explicitly close it (again respecting `stickyUsers`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

16. ~~Show-more button default styling~~ ☑ 🐞 Bug ⚙ Maintainability

Description

The show-more control changed from a span to a button, but there is no CSS reset for native button
border/background/padding. When it becomes visible in cropped-toolbar mode, it can render with
browser-default button chrome and spacing.

Code

+      <button type="button" class="show-more-icon-btn" aria-label="Show more toolbar buttons" aria-expanded="false"></button> <!-- use on small screen to display hidden toolbar buttons -->

Evidence
The template now emits a ` for .show-more-icon-btn`. The existing CSS sets sizing/typography but
does not reset default button styles (border/background/padding/appearance), which were irrelevant
when the element was a ``.
src/templates/pad.html[64-78]
src/static/css/pad/toolbar.css[100-110]
src/static/skins/colibris/src/components/toolbar.css[116-124]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`.show-more-icon-btn` is now a `<button>` and may pick up browser default border/background/padding unless explicitly reset.
### Issue Context
The existing toolbar CSS styles `.show-more-icon-btn` (size, line-height, etc.) but does not neutralize native button UI. Previously this was a `<span>` so it had no native chrome.
### Fix Focus Areas
- src/templates/pad.html[64-78]
- src/static/css/pad/toolbar.css[100-110]
- src/static/skins/colibris/src/components/toolbar.css[116-124]
### Suggested fix
Add a reset for `.toolbar .show-more-icon-btn` (in both the default toolbar CSS and colibris skin if needed), for example:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

17. Blank-target export lacks noopener 🐞 Bug ⛨ Security

Description

The export links still use target="_blank" without rel="noopener noreferrer", allowing the opened
page to access window.opener. This enables tabnabbing/opener navigation if the opened document
executes script.

Code

+              <a id="exportetherpada" target="_blank" class="exportlink" aria-label="Export as Etherpad">
+                <span class="exporttype buttonicon buttonicon-file-powerpoint" id="exportetherpad" data-l10n-id="pad.importExport.exportetherpad" aria-hidden="true"></span>
           </a>
-              <a id="exporthtmla" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-code" id="exporthtml" data-l10n-id="pad.importExport.exporthtml"></span>
+              <a id="exporthtmla" target="_blank" class="exportlink" aria-label="Export as HTML">

Evidence
The PR touched the export link markup and kept target="_blank" but did not add `rel="noopener
noreferrer". The codebase already uses rel="noopener" on other target="_blank"` links,
indicating this protection is expected.
src/templates/pad.html[218-235]
src/node/handler/ExportHandler.ts[58-87]
src/templates/pad.html[183-186]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`target="_blank"` links without `rel="noopener noreferrer"` expose `window.opener` to the new tab.
### Issue Context
The export links in the import/export dialog open in a new tab and were modified in this PR. Other links in the same template already include `rel="noopener"`, so adding it here is consistent.
### Fix Focus Areas
- src/templates/pad.html[218-235]
### Suggested fix
Add `rel="noopener noreferrer"` to each export `<a ... target="_blank" ...>` element (all six). If you need referrer behavior, keep it consistent with existing link policies.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

18. ~~Escape won’t close userlist~~ ☑ 🐞 Bug ≡ Correctness

Description

The new Escape path calls toggleDropDown('none'), but toggleDropDown('none') intentionally skips the
'users' dropdown and #users is not a .toolbar-popup element. Escape pressed while focus is inside
the Users popup therefore won’t close it or restore focus to the trigger.

Code

src/static/js/pad_editbar.ts[329-336]
src/static/js/pad_editbar.ts[219-246]
src/templates/pad.html[348-376]

+    // Escape from inside any open popup: close the popup and let
+    // toggleDropDown('none') restore focus to the trigger.
+    if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) {
+      this.toggleDropDown('none');
+      evt.preventDefault();
+      return;
+    }

Evidence

Escape handling always routes to toggleDropDown('none') when focus is inside an open popup, but
toggleDropDown('none') skips closing the users dropdown. Because the Users popup is rendered as
class="popup" (no toolbar-popup class), the earlier $('.toolbar-popup').removeClass('popup-show')
line does not affect it, leaving it open on Escape.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Escape-to-close calls `toggleDropDown('none')`, but that code path skips closing the `users` dropdown, so Escape won’t dismiss the Users popup.
### Issue Context
- `toggleDropDown('none')` currently has a hard-coded `if (thisModuleName === 'users') continue;`.
- `#users` in the template has `class="popup"` (no `toolbar-popup`), so `$('.toolbar-popup').removeClass('popup-show')` does not close it.
### Fix Focus Areas
- src/static/js/pad_editbar.ts[211-246]
- src/static/js/pad_editbar.ts[329-336]
- src/templates/pad.html[348-376]
### Suggested fix
- In the `moduleName === 'none'` branch, close `users` unless it is sticky:
- Replace the unconditional skip with something like:
 - `if (thisModuleName === 'users' && $('#users').hasClass('stickyUsers')) continue;`
 - otherwise remove `popup-show` from `#users` the same way as other dropdowns.
- Alternatively, in the Escape handler, detect `#users.popup-show` and explicitly close it (again respecting `stickyUsers`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

19. ~~Show-more button default styling~~ ☑ 🐞 Bug ⚙ Maintainability

Description

The show-more control changed from a span to a button, but there is no CSS reset for native button
border/background/padding. When it becomes visible in cropped-toolbar mode, it can render with
browser-default button chrome and spacing.

Code

+      <button type="button" class="show-more-icon-btn" aria-label="Show more toolbar buttons" aria-expanded="false"></button> <!-- use on small screen to display hidden toolbar buttons -->

Evidence
The template now emits a ` for .show-more-icon-btn`. The existing CSS sets sizing/typography but
does not reset default button styles (border/background/padding/appearance), which were irrelevant
when the element was a ``.
src/templates/pad.html[64-78]
src/static/css/pad/toolbar.css[100-110]
src/static/skins/colibris/src/components/toolbar.css[116-124]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`.show-more-icon-btn` is now a `<button>` and may pick up browser default border/background/padding unless explicitly reset.
### Issue Context
The existing toolbar CSS styles `.show-more-icon-btn` (size, line-height, etc.) but does not neutralize native button UI. Previously this was a `<span>` so it had no native chrome.
### Fix Focus Areas
- src/templates/pad.html[64-78]
- src/static/css/pad/toolbar.css[100-110]
- src/static/skins/colibris/src/components/toolbar.css[116-124]
### Suggested fix
Add a reset for `.toolbar .show-more-icon-btn` (in both the default toolbar CSS and colibris skin if needed), for example:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

20. Blank-target export lacks noopener 🐞 Bug ⛨ Security

Description

The export links still use target="_blank" without rel="noopener noreferrer", allowing the opened
page to access window.opener. This enables tabnabbing/opener navigation if the opened document
executes script.

Code

+              <a id="exportetherpada" target="_blank" class="exportlink" aria-label="Export as Etherpad">
+                <span class="exporttype buttonicon buttonicon-file-powerpoint" id="exportetherpad" data-l10n-id="pad.importExport.exportetherpad" aria-hidden="true"></span>
            </a>
-              <a id="exporthtmla" target="_blank" class="exportlink">
-                <span class="exporttype buttonicon buttonicon-file-code" id="exporthtml" data-l10n-id="pad.importExport.exporthtml"></span>
+              <a id="exporthtmla" target="_blank" class="exportlink" aria-label="Export as HTML">

Evidence
The PR touched the export link markup and kept target="_blank" but did not add `rel="noopener
noreferrer". The codebase already uses rel="noopener" on other target="_blank"` links,
indicating this protection is expected.
src/templates/pad.html[218-235]
src/node/handler/ExportHandler.ts[58-87]
src/templates/pad.html[183-186]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`target="_blank"` links without `rel="noopener noreferrer"` expose `window.opener` to the new tab.
### Issue Context
The export links in the import/export dialog open in a new tab and were modified in this PR. Other links in the same template already include `rel="noopener"`, so adding it here is consistent.
### Fix Focus Areas
- src/templates/pad.html[218-235]
### Suggested fix
Add `rel="noopener noreferrer"` to each export `<a ... target="_blank" ...>` element (all six). If you need referrer behavior, keep it consistent with existing link policies.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

21. ~~Escape won’t close userlist~~ ☑ 🐞 Bug ≡ Correctness

Description

The new Escape path calls toggleDropDown('none'), but toggleDropDown('none') intentionally skips the
'users' dropdown and #users is not a .toolbar-popup element. Escape pressed while focus is inside
the Users popup therefore won’t close it or restore focus to the trigger.

Code

src/static/js/pad_editbar.ts[329-336]
src/static/js/pad_editbar.ts[219-246]
src/templates/pad.html[348-376]

+    // Escape from inside any open popup: close the popup and let
+    // toggleDropDown('none') restore focus to the trigger.
+    if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) {
+      this.toggleDropDown('none');
+      evt.preventDefault();
+      return;
+    }

Evidence

Escape handling always routes to toggleDropDown('none') when focus is inside an open popup, but
toggleDropDown('none') skips closing the users dropdown. Because the Users popup is rendered as
class="popup" (no toolbar-popup class), the earlier $('.toolbar-popup').removeClass('popup-show')
line does not affect it, leaving it open on Escape.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Escape-to-close calls `toggleDropDown('none')`, but that code path skips closi...

qodo-code-review · 2026-04-23T03:33:43Z

Review Summary by Qodo

Accessibility: dialog semantics, focus management, icon labels, and html lang

🐞 Bug fix ✨ Enhancement 🧪 Tests

Walkthroughs

Description

• Add lang and dir attributes to HTML templates negotiated from Accept-Language header
• Implement dialog semantics (role, aria-modal, aria-labelledby/aria-label) on all popups
• Add focus management: remember trigger element, focus first focusable on open, restore on close
• Enable Escape key to close popups while preserving focus restoration
• Convert icon-only controls to semantic buttons with accessible labels (chat icon, close/pin
  buttons, show-more, export links)
• Fix invalid aria-role="document" typo on userlist; replace with role="region" and
  aria-live="polite"
• Add comprehensive Playwright test suite covering dialog ARIA, lang attribute, focus behavior, and
  button semantics

Diagram

flowchart LR
  A["Templates<br/>pad.html, index.html,<br/>timeslider.html"] -->|"Add lang/dir<br/>from Accept-Language"| B["HTML Element<br/>lang & dir attrs"]
  C["Popup Elements<br/>#settings, #users,<br/>#embed, etc."] -->|"Add ARIA<br/>semantics"| D["Dialog Roles<br/>aria-modal, aria-labelledby"]
  E["toggleDropDown<br/>in pad_editbar.ts"] -->|"Remember trigger &<br/>focus management"| F["Focus Restoration<br/>on popup close"]
  G["Escape Key<br/>Handler"] -->|"Close popup<br/>& restore focus"| F
  H["Icon-only Controls<br/>chat, export, show-more"] -->|"Convert to buttons<br/>with aria-label"| I["Semantic Buttons<br/>with accessible names"]
  J["Tests<br/>a11y_dialogs.spec.ts"] -->|"Verify all<br/>changes"| K["Dialog & Focus<br/>Coverage"]

File Changes

1. src/templates/pad.html ✨ Enhancement +36/-33

Add dialog ARIA semantics, focus management, and button labels

• Add lang and dir attributes to <html> element negotiated from Accept-Language header
• Add role="dialog", aria-modal="true", and aria-labelledby/aria-label to all popup divs
 (#settings, #import_export, #embed, #users, #mycolorpicker, #skin-variants, #connectivity)
• Add unique IDs to popup heading elements for aria-labelledby references
• Convert #chaticon from div with onclick to semantic <button> with aria-label="Open chat"
• Convert #titlecross and #titlesticky from anchors to buttons with descriptive aria-label
 attributes
• Convert .show-more-icon-btn from span to button with aria-label and aria-expanded attribute
• Add aria-label and aria-hidden="true" to all six export links and their inner icon spans
• Fix #otherusers from invalid aria-role="document" to role="region" with aria-live="polite"
 and aria-label
• Update theme switcher knob label from CSS-style identifier to human-readable text
• Add event handlers in chat.ts for button click events

src/templates/pad.html

2. src/static/js/pad_editbar.ts ✨ Enhancement +37/-1

Implement focus management and Escape-to-close for popups

• Add _lastTrigger field to remember the element that opened a popup for focus restoration
• Enhance toggleDropDown() to capture active element before opening popup
• Implement focus movement to first focusable element inside popup on open using
 requestAnimationFrame
• Restore focus to trigger element when all popups close
• Add Escape key handler in _bodyKeyEvent() to close open popups and trigger focus restoration
• Update .show-more-icon-btn click handler to toggle and sync aria-expanded attribute

src/static/js/pad_editbar.ts

3. src/static/js/chat.ts ✨ Enhancement +13/-0

Wire up chat button click handlers

• Add click event handlers for #chaticon, #titlecross, and #titlesticky to replace inline
 onclick attributes
• Move chat show/hide/stickToScreen logic from HTML onclick to jQuery event listeners

src/static/js/chat.ts

View more (5)

4. src/static/css/pad/chat.css ✨ Enhancement +16/-0

Style chat buttons and focus indicators

• Add CSS reset for button styling on #chaticon, #titlecross, #titlesticky (transparent
 background, no border, inherit font/color)
• Add :focus-visible outline styling for keyboard navigation on chat header buttons
• Add :focus-visible outline styling for #chaticon button

src/static/css/pad/chat.css

5. src/templates/index.html ✨ Enhancement +7/-1

Add lang/dir negotiation to index template

• Add server-side language negotiation logic using req.acceptsLanguages() against available
 languages
• Add lang and dir attributes to <html> element based on negotiated language

src/templates/index.html

6. src/templates/timeslider.html ✨ Enhancement +4/-1

Add lang/dir negotiation to timeslider template

• Add server-side language negotiation logic using req.acceptsLanguages() against available
 languages
• Add lang and dir attributes to <html> element based on negotiated language

src/templates/timeslider.html

7. src/tests/frontend-new/specs/a11y_dialogs.spec.ts 🧪 Tests +110/-0

Add comprehensive a11y dialog and focus tests

• Create new Playwright test suite for accessibility features
• Test <html> element has non-empty lang attribute
• Test settings popup has dialog semantics, Escape closes it, and focus returns to trigger
• Test import_export, embed, and users popups have proper dialog ARIA attributes
• Test all six export links have descriptive aria-label attributes
• Test chaticon is a button with accessible name
• Test chat header close/pin controls are buttons with labels
• Test otherusers region has role="region", aria-live="polite", and no invalid aria-role
 attribute
• Test show-more toolbar button has aria-label and aria-expanded attributes

src/tests/frontend-new/specs/a11y_dialogs.spec.ts

8. docs/superpowers/plans/2026-04-22-a11y-dialogs-labels-lang.md 📝 Documentation +404/-0

Add detailed accessibility implementation plan documentation

• Document comprehensive plan for accessibility improvements addressing dialog semantics, focus
 management, icon labels, and html lang
• Provide step-by-step implementation guide with 7 major tasks and 40+ subtasks
• Include code examples, test cases, and out-of-scope items
• Serve as reference documentation for the PR changes

docs/superpowers/plans/2026-04-22-a11y-dialogs-labels-lang.md

qodo-code-review · 2026-04-23T03:33:44Z

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Escape won’t close userlist 🐞 Bug ≡ Correctness

Description

The new Escape path calls toggleDropDown('none'), but toggleDropDown('none') intentionally skips the
'users' dropdown and #users is not a .toolbar-popup element. Escape pressed while focus is inside
the Users popup therefore won’t close it or restore focus to the trigger.

Code

src/static/js/pad_editbar.ts[329-336]
src/static/js/pad_editbar.ts[219-246]
src/templates/pad.html[348-376]

+    // Escape from inside any open popup: close the popup and let
+    // toggleDropDown('none') restore focus to the trigger.
+    if (evt.keyCode === 27 && $(':focus').closest('.popup.popup-show').length === 1) {
+      this.toggleDropDown('none');
+      evt.preventDefault();
+      return;
+    }

Evidence

Escape handling always routes to toggleDropDown('none') when focus is inside an open popup, but
toggleDropDown('none') skips closing the users dropdown. Because the Users popup is rendered as
class="popup" (no toolbar-popup class), the earlier $('.toolbar-popup').removeClass('popup-show')
line does not affect it, leaving it open on Escape.

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Escape-to-close calls `toggleDropDown('none')`, but that code path skips closing the `users` dropdown, so Escape won’t dismiss the Users popup.

### Issue Context
- `toggleDropDown('none')` currently has a hard-coded `if (thisModuleName === 'users') continue;`.
- `#users` in the template has `class="popup"` (no `toolbar-popup`), so `$('.toolbar-popup').removeClass('popup-show')` does not close it.

### Fix Focus Areas
- src/static/js/pad_editbar.ts[211-246]
- src/static/js/pad_editbar.ts[329-336]
- src/templates/pad.html[348-376]

### Suggested fix
- In the `moduleName === 'none'` branch, close `users` unless it is sticky:
 - Replace the unconditional skip with something like:
   - `if (thisModuleName === 'users' && $('#users').hasClass('stickyUsers')) continue;`
   - otherwise remove `popup-show` from `#users` the same way as other dropdowns.
- Alternatively, in the Escape handler, detect `#users.popup-show` and explicitly close it (again respecting `stickyUsers`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Show-more button default styling 🐞 Bug ⚙ Maintainability

Description

The show-more control changed from a span to a button, but there is no CSS reset for native button
border/background/padding. When it becomes visible in cropped-toolbar mode, it can render with
browser-default button chrome and spacing.

Code

+      <button type="button" class="show-more-icon-btn" aria-label="Show more toolbar buttons" aria-expanded="false"></button> <!-- use on small screen to display hidden toolbar buttons -->

Evidence
The template now emits a <button> for .show-more-icon-btn. The existing CSS sets
sizing/typography but does not reset default button styles (border/background/padding/appearance),
which were irrelevant when the element was a <span>.
src/templates/pad.html[64-78]
src/static/css/pad/toolbar.css[100-110]
src/static/skins/colibris/src/components/toolbar.css[116-124]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`.show-more-icon-btn` is now a `<button>` and may pick up browser default border/background/padding unless explicitly reset.

### Issue Context
The existing toolbar CSS styles `.show-more-icon-btn` (size, line-height, etc.) but does not neutralize native button UI. Previously this was a `<span>` so it had no native chrome.

### Fix Focus Areas
- src/templates/pad.html[64-78]
- src/static/css/pad/toolbar.css[100-110]
- src/static/skins/colibris/src/components/toolbar.css[116-124]

### Suggested fix
Add a reset for `.toolbar .show-more-icon-btn` (in both the default toolbar CSS and colibris skin if needed), for example:
```css
.toolbar .show-more-icon-btn {
 background: transparent;
 border: 0;
 padding: 0;
 font: inherit;
 color: inherit;
 appearance: none;
}
```

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Blank-target export lacks noopener 🐞 Bug ⛨ Security

Description

The export links still use target="_blank" without rel="noopener noreferrer", allowing the opened
page to access window.opener. This enables tabnabbing/opener navigation if the opened document
executes script.

Code