Skip to content
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.

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

Give all HTML button elements a type #18517

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Jul 30, 2024

The HTML button elements we use are all regular buttons that don't submit form data to a server. According to
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#notes those buttons should all have their type set to button explicitly, but we only do that a handful of them. This commit fixes the issue by consistently giving all our buttons the button type.

Extracts a part of #18385.

The HTML button elements we use are all regular buttons that don't
submit form data to a server. According to
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#notes
those buttons should all have their type set to `button` explicitly, but
we only do that a handful of them. This commit fixes the issue by
consistently giving all our buttons the `button` type.

Co-authored-by: Calixte Denizet <calixte.denizet@gmail.com>
@calixteman
Copy link
Contributor

I discovered this issue thanks to a warning in Edge console.

@timvandermeij
Copy link
Contributor Author

I have verified that all buttons now have the button type using the following one-liner (the XFA ones originate from the PDF contents and should stay like this):

$ grep -r '<button' * | grep -v 'type="button"'
grep: test/pdfs/xfa_filled_imm1344e.pdf: binary file contains the text
test/unit/xfa_tohtml_spec.js:            <button/>
test/unit/xfa_tohtml_spec.js:            <button/>

This can be compared to master where the command found 60 occurrences instead:

$ grep -r '<button' * | grep -v 'type="button"' | wc -l
60

@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/ab493312e211266/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/ab493312e211266/output.txt

Total script time: 1.21 mins

Published

@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/957a1d6e95a1158/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/144226ddc948f48/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/144226ddc948f48/output.txt

Total script time: 8.43 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/957a1d6e95a1158/output.txt

Total script time: 17.70 mins

  • Integration Tests: FAILED

@timvandermeij timvandermeij merged commit ec0f0f3 into mozilla:master Aug 1, 2024
6 checks passed
@timvandermeij timvandermeij deleted the viewer-button-types branch August 1, 2024 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants