Invalid html in aboutNetError.html: button nested in anchor
Categories
(Firefox :: Security, defect, P3)
Tracking
()
People
(Reporter: vhilla, Assigned: jasonmui2011, Mentored)
References
(Regression)
Details
(Keywords: good-first-bug, priv-triaged, regression, Whiteboard: [lang=html])
Attachments
(2 files, 1 obsolete file)
48 bytes,
patch
|
Details | Diff | Splinter Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
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).
Updated•1 year ago
|
Comment 1•1 year ago
|
||
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•1 year ago
|
||
Well before that it was xhtml, but the markup was the same, right? So not a regression really...
Reporter | ||
Comment 3•1 year ago
|
||
Bug 1461195 introduced the code
Comment 4•1 year ago
|
||
Actually the button and the associated code can be removed. It's hidden by default now.
Comment 5•1 year ago
|
||
(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:
<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•1 year ago
|
||
The associated pref for hiding the button is security.xfocsp.hideOpenInNewWindow
, which is always true. That change from bug 1888695 just reached stable.
Updated•1 year ago
|
Comment 7•1 year ago
|
||
Hi Manuel, if this bug is not yet solved, can you please assign me to solve this bug?
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.
Updated•1 year ago
|
Comment 9•10 months ago
|
||
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•10 months ago
|
||
For anyone: Feel free to just submit a patch, you'll get automatically assigned when doing so.
Assignee | ||
Comment 11•10 months ago
|
||
Hi Manuel, can you please assign me to solve this bug?
Assignee | ||
Comment 12•10 months ago
|
||
Updated•10 months ago
|
Assignee | ||
Comment 13•10 months ago
|
||
Bug 1882316 - Removed invalid button nested inside anchor in aboutNetError.html
Updated•10 months ago
|
Assignee | ||
Comment 14•10 months ago
|
||
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.
Assignee | ||
Comment 15•10 months ago
|
||
Updated•10 months ago
|
Updated•10 months ago
|
Assignee | ||
Comment 16•10 months ago
|
||
A generic error occurred in the code review bot. How do I get more details about this generic error?
Comment 17•10 months ago
|
||
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•10 months ago
|
||
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•10 months ago
|
||
Assignee | ||
Comment 20•10 months ago
|
||
(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 thatmoz-phab
submits updates to the correct patch.
Got it. Thanks.
Updated•10 months ago
|
Comment 21•10 months ago
|
||
bugherder |
Comment 22•10 months ago
|
||
Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.
Comment 23•10 months ago
|
||
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
towontfix
.
For more information, please visit BugBot documentation.
Assignee | ||
Comment 24•10 months ago
|
||
I tried to set the status to wontfix in the Tracking Flags section, but wontfix wasn't available in the dropdown.
Comment 25•10 months ago
|
||
(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! :-)
Updated•10 months ago
|
Assignee | ||
Comment 26•10 months ago
|
||
(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.
Updated•10 months ago
|
Description
•