Skip to content

Give all HTML button elements a type#18517

Merged
timvandermeij merged 1 commit into
mozilla:masterfrom
timvandermeij:viewer-button-types
Aug 1, 2024
Merged

Give all HTML button elements a type#18517
timvandermeij merged 1 commit into
mozilla:masterfrom
timvandermeij:viewer-button-types

Conversation

@timvandermeij

@timvandermeij timvandermeij commented Jul 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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.

@timvandermeij @calixteman
Give all HTML button elements a type
f1f58bb 
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

calixteman commented Jul 30, 2024

Copy link
Copy Markdown
Contributor

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

@timvandermeij

timvandermeij commented Jul 30, 2024

Copy link
Copy Markdown
Collaborator 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

timvandermeij commented Jul 30, 2024

Copy link
Copy Markdown
Collaborator Author

/botio-linux preview

@moz-tools-bot

moz-tools-bot commented Jul 30, 2024

Copy link
Copy Markdown
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

moz-tools-bot commented Jul 30, 2024

Copy link
Copy Markdown
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

timvandermeij commented Jul 30, 2024

Copy link
Copy Markdown
Collaborator Author

/botio integrationtest

@moz-tools-bot

moz-tools-bot commented Jul 30, 2024

Copy link
Copy Markdown
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

moz-tools-bot commented Jul 30, 2024

Copy link
Copy Markdown
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

moz-tools-bot commented Jul 30, 2024

Copy link
Copy Markdown
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

moz-tools-bot commented Jul 30, 2024

Copy link
Copy Markdown
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
@timvandermeij timvandermeij deleted the viewer-button-types branch August 1, 2024 18:06
@YuvrajKaushal YuvrajKaushal mentioned this pull request Sep 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees

No one assigned

Labels

viewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@timvandermeij @calixteman @moz-tools-bot @Snuffleupagus