1882316 - Invalid html in aboutNetError.html: button nested in anchor

Status: RESOLVED FIXED

(Firefox :: Security, defect, P3)

Description

[Vincent Hilla [:vhilla]]

Line 94-103 contain a button nested within an anchor element. This violates the content model of anchor elements, specifically they should have no interactive content descendants (spec).

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1874701

:emilio, since you are the author of the regressor, bug 1874701, could you take a look?

For more information, please visit BugBot documentation.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Well before that it was xhtml, but the markup was the same, right? So not a regression really...

Comment 3

[Vincent Hilla [:vhilla]]

Bug 1461195 introduced the code

Comment 4

[Tom Schuster]

Actually the button and the associated code can be removed. It's hidden by default now.

Comment 5

[:Gijs (out for now; he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

Well before that it was xhtml, but the markup was the same, right? So not a regression really...

Ah, sorry. I went to the link from comment 0, then clicked "show latest version without this line" on the lines introduced by your patch, and that shows:

https://searchfox.org/mozilla-central/rev/9e365a24190dc6fca49eecc260e586d7792d1e6d/toolkit/content/aboutNetError.html#97-101

<div id="advancedPanelButtonContainer" class="button-container">
  <button id="advancedPanelReturnButton" class="primary" data-telemetry-id="return_button_adv" data-l10n-id="neterror-return-to-previous-page-recommended-button"></button>
  <button id="advancedPanelTryAgainButton" class="primary try-again" data-l10n-id="neterror-try-again-button" hidden=""></button>
  <button id="exceptionDialogButton" data-telemetry-id="exception_button" data-l10n-id="neterror-override-exception-button"></button>
</div>

which looked like buttons that were in a div rather than an <a>. I didn't realize they were different buttons, and the relevant one is actually https://searchfox.org/mozilla-central/rev/9e365a24190dc6fca49eecc260e586d7792d1e6d/toolkit/content/aboutNetError.html#65-68 which indeed was there before.

OOI, isn't the XHTML parsing model different in the sense that it would never "correct" broken markup like this? I don't know whether the HTML parser bits cares about the spec bits linked in comment 0 to the point of reparenting/parsing this particular violation, and didn't bother to check. I just happened to notice this bug on file when doing something else and so probably didn't give it the care and attention it deserved. Apologies for that.

(In reply to Tom Schuster (MoCo) from comment #4)

Actually the button and the associated code can be removed. It's hidden by default now.

Hidden as in can still be shown, or just... never shown?

Comment 6

[Tom Schuster]

The associated pref for hiding the button is security.xfocsp.hideOpenInNewWindow, which is always true. That change from bug 1888695 just reached stable.

Comment 7

[anshumanjofficial]

Hi Manuel, if this bug is not yet solved, can you please assign me to solve this bug?

Comment 8

[Manuel Bucher [:manuel]]

Hi, sure. Please reach out if you need assistance (Here on the bug tracker or via Matrix in #introduction:mozilla.org or #developer:mozilla.org and ping me @mbucher:mozilla.org). I think for now fixing the html would be the scope of the bug.
Removing the pref would also be fine for this bug, but I prefer opening another bug for that and fixing the html here due to the recentness of the pref flip.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 10

[Manuel Bucher [:manuel]]

For anyone: Feel free to just submit a patch, you'll get automatically assigned when doing so.

Comment 11

[Jason]

Hi Manuel, can you please assign me to solve this bug?

Comment 12

[Jason]

Attachment: Bug 1882316 - Removed invalid button nested inside anchor in aboutNetError.html r=manuel

Comment 13

[Jason]

Attachment: Bug 1882316 - Removed invalid button nested inside anchor in aboutNetError.html r=manuel

Bug 1882316 - Removed invalid button nested inside anchor in aboutNetError.html

Comment 14

[Jason]

Comment on attachment 9432744 [details] [diff] [review]
Bug 1882316 - Removed invalid button nested inside anchor in aboutNetError.html r=manuel

Fixed the lint problems from previous submission.

Comment 15

[Jason]

Attachment: Bug 1882316 - Removed invalid button nested inside anchor in aboutNetError.html r=Gijs

Comment 16

[Jason]

A generic error occurred in the code review bot. How do I get more details about this generic error?

Comment 17

[Manuel Bucher [:manuel]]

https://treeherder.mozilla.org/jobs?repo=try&revision=f147396bd4d6b38bad7ca5f9abb7dcdbafb9161a
By clicking through to the treeherder link.

When I click through the build until the treeherder job, there aren't any failures there. https://treeherder.mozilla.org/jobs?repo=try&revision=f147396bd4d6b38bad7ca5f9abb7dcdbafb9161a From my perspective, this generic error looks like it can be ignored (I would ignore if it were on my patch).

Maybe something is wrong with the base commit. For that you could rebase and make sure there aren't any patches between yours and current tip when running moz-phab submit.

Comment 18

[Manuel Bucher [:manuel]]

There seem to be three patches doing the same thing. We only need one.

• One with review from me -> You can probably also mark the one with my review as abandoned (take the action on the bottom of the phabricator page), due to the newer one with emilios review existing.
• One attached directly -> you can mark this one as obsolet
• One with review from Gijs/emilio (this one received the most updates).

Make sure the last line of the commit message stays Differential Revision: https://phabricator.services.mozilla.com/D226590 so that moz-phab submits updates to the correct patch.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/482d62afdbdc
Removed invalid button nested inside anchor in aboutNetError.html r=emilio

Comment 20

[Jason]

(In reply to Manuel Bucher [:manuel] from comment #18)

There seem to be three patches doing the same thing. We only need one.

• One with review from me -> You can probably also mark the one with my review as abandoned (take the action on the bottom of the phabricator page), due to the newer one with emilios review existing.
• One attached directly -> you can mark this one as obsolet
• One with review from Gijs/emilio (this one received the most updates).

Make sure the last line of the commit message stays Differential Revision: https://phabricator.services.mozilla.com/D226590 so that moz-phab submits updates to the correct patch.

Got it. Thanks.

Comment 21

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/482d62afdbdc

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jasonmui2011, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox132 to wontfix.

For more information, please visit BugBot documentation.

Comment 24

[Jason]

I tried to set the status to wontfix in the Tracking Flags section, but wontfix wasn't available in the dropdown.

Comment 25

[:Gijs (out for now; he/him)]

(In reply to Jason from comment #24)

I tried to set the status to wontfix in the Tracking Flags section, but wontfix wasn't available in the dropdown.

It's in the "status" part rather than the "tracking" part. It's possible that you don't get a second dropdown if you don't have the "editbugs" permission ( https://wiki.mozilla.org/BMO/UserGuide#Permissions_and_Groups is the best doc I can find and is not very clear about what you can/can't do with certain permissions). Clearly it has not occurred to the people writing the bugbot automation that occasionally bugs get fixed by people who don't have permission to do as it asks... Sorry about that.

I'll check in with the bugbot and bugzilla folks to see what we can do to avoid this happening to anyone else. In the interim I have personally given you editbugs. Use your new powers wisely - and thank you again for the bugfix! :-)

Comment 26

[Jason]

(In reply to :Gijs (he/him) from comment #25)

(In reply to Jason from comment #24)

I tried to set the status to wontfix in the Tracking Flags section, but wontfix wasn't available in the dropdown.

It's in the "status" part rather than the "tracking" part. It's possible that you don't get a second dropdown if you don't have the "editbugs" permission ( https://wiki.mozilla.org/BMO/UserGuide#Permissions_and_Groups is the best doc I can find and is not very clear about what you can/can't do with certain permissions). Clearly it has not occurred to the people writing the bugbot automation that occasionally bugs get fixed by people who don't have permission to do as it asks... Sorry about that.

I'll check in with the bugbot and bugzilla folks to see what we can do to avoid this happening to anyone else. In the interim I have personally given you editbugs. Use your new powers wisely - and thank you again for the bugfix! :-)

Thanks. I can see "wontfix" in the "status" dropdown now.