Make WordPress Core

Opened 4 months ago

Closed 2 months ago

Last modified 2 months ago

#65491 closed defect (bug) (fixed)

Add support for the autofocus attribute within dialog elements

Reported by: afercia Owned by: afercia
Priority: normal Milestone: 7.1
Component: Formatting Version:
Severity: normal Keywords: has-patch has-unit-tests has-test-info commit
Cc: Focuses: accessibility

Description (last modified by westonruter)

Splitting this out from #64576

#64576 is going to allow for the command and commandfor in the kses. These attributes can be used to handle native popovers and dialogs.

For the <dialog> element, one more attribute is very important to handle initial focus within a dialog: the autofocus attribute.

This attribute should be allowed in the kses. As @westonruter mentioned in https://core.trac.wordpress.org/ticket/64576#comment:8 it would be good to explore a way to allow autofocus only within the context of a <dialog> element.

Attachments (1)

After applying the patch autofocus is not removed.png​ (33.3 KB ) - added by sanayasir 3 months ago.

Download all attachments as: .zip

Change History (42)

#1 @afercia
4 months ago

  • Type defect (bug) → enhancement

#2 @westonruter
4 months ago

  • Description modified (diff)

This ticket was mentioned in ​PR #12255 on ​WordPress/wordpress-develop by ​@sukhendu2002.


4 months ago
#3

  • Keywords has-patch has-unit-tests added

#4 @westonruter
3 months ago

I think this will require use of the HTML API to sanitize properly. Ideally Kses would be using the HTML API itself, and then this could be added on top of that better foundation. Otherwise, it would seem that this wouldn't be implementable in Kses and would instead require a separate function using the HTML API which runs along with the other Kses filter functions.

This ticket was mentioned in ​Slack in #accessibility by joedolson. ​View the logs.


3 months ago

#6 @afercia
3 months ago

  • Owner set to afercia
  • Status new → assigned

#7 @joedolson
3 months ago

Adding the HTML API creates a lot of additional complexity here, and also potentially means we have a KSES-like rule that can't be filtered the same way as other KSES rules. That should be considered carefully.

I think we need to decide whether it's more important that we ensure this attribute is available where needed or that it's prevented in cases where it's undesirable.

This ticket was mentioned in ​PR #12453 on ​WordPress/wordpress-develop by ​@afercia.


3 months ago
#8

Trac ticket: https://core.trac.wordpress.org/ticket/65491

## Use of AI Tools

AI assistance: Yes
Tool(s): GitHub Copilot, VSCode
Model(s): MAI-Code-1-Flash.
Used for: Initial code skeleton and test suggestions; final implementation and tests were reviewed and edited by me.

#9 @afercia
3 months ago

  • Keywords needs-testing added

In ​https://github.com/WordPress/wordpress-develop/pull/12453 I'm trying a new approach by using small helpers added in kses.php. They are small and self-contained, no use of the HTML API. The PR adds two tests.

Basically the helpers track whether a KSES pass is in the context of a <dialog> element and if so the autofocus attribute is allowed. An autofocus attribute added on the <dialog> element itself, it is just added to the allowed attributes.

Review and feedback welcome. Cc @westonruter if you have a chance, that would be much appreciated.

#10 @afercia
3 months ago

I would like to see this enhancement make it for 7.1. The linked PR needs a new code review to check whether the proposed approach is viable. It's small and self-contained. I would greatly appreciate some eyes from @westonruter or @SergeyBiryukov when you have a chance.

Otherwise, we should:

  • Either move this ticket to 'bug', as traditionally lack of accessibility has always been considered a bug.
  • Or, punt it to next release.

I would vote for the first option, also because this is really a small, yet important, change.

#11 follow-up: @westonruter
3 months ago

Is it important right now, though? We don't have any existing uses of it. It's more for the future so we can build on it, right?

I would feel more comfortable if this were implemented as part of refactoring Kses on top of the HTML API instead of trying to shim it into the existing system. cc @dmsnell

#12 in reply to: ↑ 11 ; follow-up: @afercia
3 months ago

Replying to westonruter:

Is it important right now, though? We don't have any existing uses of it. It's more for the future so we can build on it, right?

Since [62531] we are allowing command and commandfor for buttons, which opens the possibility to use native <dialog> elements to plugins, themes, and authors. The autofocus attribute is an important part of a native dialog implementation, without it what we've done so far would just be incomplete.

Refactoring Kses on top of the HTML API would be an interesting project but it would be a big effort that also implies security concerns and backwards compatibility. I don't see it as an easy task that can be completed in the next release cycle. I'd also argue that it would go against the WordPress philosophy of don't refactor for the sake of refactoring.

There's no guarantee that such a refactoring would be completed in a reasonable time. Personally, I'd lean towards implementing what we can implement now and add support for autofocus. If in the future Kses will be refactored we could then refactor also the support for autofocus.

#13 in reply to: ↑ 12 @westonruter
3 months ago

Replying to afercia:

Refactoring Kses on top of the HTML API would be an interesting project but it would be a big effort that also implies security concerns and backwards compatibility. I don't see it as an easy task that can be completed in the next release cycle. I'd also argue that it would go against the WordPress philosophy of don't refactor for the sake of refactoring.

It's not just for the sake of refactoring though. Rebuilding Kses on top of the HTML API would make it much more robust and fix a lot of bugs. It would also make it easier to build in such rules for allowing attributes (or elements) conditionally based on their ancestor context.

#14 @afercia
3 months ago

t would also make it easier to build in such rules for allowing attributes (or elements) conditionally based on their ancestor context.

Yes, I totally understand that. However, we only have one use case for now. I wouldn't be opposed to implementing a basic support for determining the 'context' for now. It could be replaced by a more solid implementation in the future, once it is proved that there will be more use cases.

This ticket was mentioned in ​Slack in #accessibility by joedolson. ​View the logs.


3 months ago

#16 @joedolson
3 months ago

This issue should move forward as a bug. It's a bug because the underlying problem is insufficient support in KSES to handle context-specific attribute validation. While refactoring KSES is a good idea, and would probably have a ton of benefits, it should not block us from implementing an otherwise minor change that is needed to be able to allow users to create dialog elements in a proper way.

If there's a concern that the PR would create a long-term API problem that couldn't be resolved by a refactor, that may be worth pushing back on; but in that case, I'd also want to revert support for command and commandfor invokers, as we're not ready to make use of them.

#17 @joedolson
3 months ago

  • Type enhancement → defect (bug)

#18 @sanayasir
3 months ago

Tested this ticket in WordPress Playground.

After the patch: The autofocus attribute is preserved on the <dialog> element after saving and reopening the post.

The issue is fixed, and the behavior now works as expected according to the ticket requirements.

This ticket was mentioned in ​Slack in #accessibility by joedolson. ​View the logs.


3 months ago

#20 @softglaze
3 months ago

  • Keywords has-test-info added

Test report for PR 12453 against current trunk (7.1-beta2-src).

Environment: Windows host, wordpress-develop Docker environment. Manual verification via wp eval-file running wp_kses_post() on identical input on both trunk and the PR branch; unit tests via the local PHPUnit suite.

Unit tests:

Full kses-filtered suite passes on the PR branch, including the PR's new tests:

OK (452 tests, 1553 assertions) — PHPUnit 9.6.35, 58s, no failures or errors.

Manual verification of the context-scoped allowance (trunk → PR 12453):

<dialog><button autofocus> — trunk strips autofocus; with the PR it survives.
<dialog autofocus> (attribute on the dialog element itself) — trunk strips it; with the PR it survives.
<button autofocus> with no dialog anywhere in the content — stripped on both trunk and PR, unchanged.
<dialog>…</dialog><button autofocus> (autofocus after a closed dialog) — still stripped with the PR, confirming the context tracking correctly ends at </dialog> and the allowance does not leak to subsequent content.
<dialog><script>alert(1)</script><button autofocus> — <script> remains fully stripped inside the dialog while autofocus on the button survives, confirming the allowance does not relax any other tag/attribute filtering within the dialog context.

Result: the allowance behaves exactly as scoped — autofocus is permitted only within (or on) a <dialog> element, with no leakage outside that context and no side effects on other kses filtering. Tests well.

This ticket was mentioned in ​Slack in #core-test by softglaze. ​View the logs.


3 months ago

​@westonruter commented on ​PR #12453:


3 months ago
#22

Note: I added narrowed PHP typing information to assist with my review. I'll commit that those separately.

#23 @westonruter
3 months ago

  • Keywords changes-requested added

Unfortunately, the implementation is not currently ready to ship. I've added tests that show how the guardrails can be circumvented. This really needs the HTML API to be done securely and correctly. See [review](​https://github.com/WordPress/wordpress-develop/pull/12453#pullrequestreview-4769005688) (with analysis from Claude Fable 5).

​@afercia commented on ​PR #12453:


3 months ago
#24

@westonruter Thanks for your thorough review and feedback. The case about HTML comments is interesting. Fair enough. At least, we proved there is the need for a context-aware mechanism in KSES. I think this PR can be closed. I will submit a new, clean, PR to allow autofocus only on the dialog element.

#25 @afercia
3 months ago

@westonruter thanks again. Given you are the person with the best technical context, would you mind creating a new ticket to propose the (partial) refactoring of KSES by using the HTML API? Thank you.

This ticket was mentioned in ​PR #12682 on ​WordPress/wordpress-develop by ​@afercia.


3 months ago
#26

Trac ticket: https://core.trac.wordpress.org/ticket/65491

Allows the autofocus attribute in KSES, only for the <dialog> element for now.

## Use of AI Tools

None

#27 @afercia
3 months ago

In ​https://github.com/WordPress/wordpress-develop/pull/12682 the autofocus attribute is only allowed for the dialog element, for now.

This is a partial step. It helps implementing better native dialogs when setting initial focus on the dialog itself is the desired behavior.

#28 @mirmpro
3 months ago

Test Report

Patch tested: ​https://github.com/WordPress/wordpress-develop/pull/12453

Environment

  • WordPress: 7.1-beta3
  • Subdirectory: No
  • PHP: 8.4.16
  • Server: nginx/1.26.1
  • Database: mysqli (Server: 8.0.35 / Client: mysqlnd 8.4.16)
  • Browser: Chrome 150.0.0.0
  • OS: Windows 10/11
  • Theme: Twenty Twenty-Five 1.5
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.3.0
    • WordPress Beta Tester 4.0.0

Steps taken

  1. Created a new post using the Block Editor on WordPress 7.1-beta3.
  2. Added a Custom HTML block with a dialog element containing an autofocused button alongside a regular button outside the dialog.
  3. Published the post and inspected the rendered HTML source code on the front-end page.
  4. Checked that the autofocus attribute remained intact on the button inside the dialog element.
  5. Patch is solving the problem

Expected result

  • The autofocus attribute should remain on elements inside dialog tags after post content is saved and displayed.

Actual Results

  • The patch works as expected. Buttons placed inside dialog tags keep their autofocus attribute in the rendered front-end page output.

Additional Notes

  • N/A

Screenshots/Screencast with results

  • N/A

Support Content

  • N/A

#29 @nazmulasif
3 months ago

The autofocus attribute working as expected.https://drive.google.com/file/d/1VC0KL5tenMhi6pRd6U_IimamtbnS4m2g/view

#30 @joedolson
3 months ago

  • Keywords commit added; changes-requested removed

#31 @joedolson
3 months ago

  • Keywords needs-testing removed

#32 @afercia
3 months ago

  • Resolution → fixed
  • Status assigned → closed

In 62850:

KSES: Allow the autofocus attribute on dialog elements.

First step to allow the usage of the autofocus attribute for native dialog elements. More work will follow to add a context-aware mechanism to KSES and allow the attribute on dialog element children.

Props westonruter, joedolson, mukesh27, afercia.
Fixes #65491.

#33 @afercia
3 months ago

  • Milestone 7.1 → Future Release
  • Resolution fixed
  • Status closed → reopened

Reopening and setting to Future release, as [62850] only fixed a first part of this issue.

#34 @westonruter
2 months ago

  • Milestone Future Release → 7.1
  • Resolution → fixed
  • Status reopened → closed

@afercia I think this ticket should remain 7.1 and a new ticket created for the Future Release follow-up. Otherwise there would be a commit without a ticket in this milestone.

#35 @afercia
2 months ago

Right, will create the new one.

#36 @mirmpro
2 months ago

@westonruter @afercia Hi Sir, I think you missed my name for the props, I have participated on this ticket. Thanks

Last edited 2 months ago by mirmpro (previous) (diff)

#37 @nazmulasif
2 months ago

@westonruter i missed the props though i participated in the ticket. Could you please check?

#38 follow-ups: @afercia
2 months ago

I added the Missed Prop: nazmulasif and mirmpro via the .org admin. Not sure they will appear on this ticket or the changeset but they whould be attributed, I guess.

#39 in reply to: ↑ 38 @mirmpro
2 months ago

Replying to afercia:

I added the Missed Prop: nazmulasif and mirmpro via the .org admin. Not sure they will appear on this ticket or the changeset but they whould be attributed, I guess.

Thank you very much!

#40 in reply to: ↑ 38 @softglaze
2 months ago

Replying to afercia:

I added the Missed Prop: nazmulasif and mirmpro via the .org admin. Not sure they will appear on this ticket or the changeset but they whould be attributed, I guess.

Same thing happened with me (if you think I should have props too), can you please do it for me, if possible?

#41 @afercia
2 months ago

Added softglaze and sanayasir as well.

Note: See TracTickets for help on using tickets.