1470229 - Reader mode strips aria-label

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P1)

Description

[Eitan Isaacson [:eeejay]]

STR:

1. Load https://s.codepen.io/joe-watkins/debug/gKWJva
2. Observe that the first "Read Next" link has an aria-label attribute.
3. Toggle reader mode
4. Observe the the fist "Read Next" link is missing it's aria-label

We should preserve all 'role' and 'aria-*' attributes. Some attributes that define relations like aria-labelledby with ids that were stripped will be broken. I wouldn't worry about it, and leave it to our a11y engine to try and fail to resolve those.

Comment 1

[Eitan Isaacson [:eeejay]]

Looks like this is done in nsTreeSanitizer via nsIParserUtils. 'role' seems to be fine.

Should we create a new flag about preserving aria attributes, or should they really be part of the default HTML attributes corpus defined here:
https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/dom/base/nsTreeSanitizer.cpp#151

Flagging xidorn because I don't know who to ask!

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

I have some idea about how to fix it. Keep the ni? for now.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1470229 part 1 - Make the starting-with check in nsTreeSanitizer::SanitizeAttributes nicer.

Review commit: https://reviewboard.mozilla.org/r/252226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252226/

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1470229 part 2 - Allow aria attributes.

Review commit: https://reviewboard.mozilla.org/r/252228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252228/

Comment 5

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Comment on attachment 8986975 [details]
Bug 1470229 part 1 - Make the starting-with check in nsTreeSanitizer::SanitizeAttributes nicer.

https://reviewboard.mozilla.org/r/252226/#review259098

Comment 6

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Comment on attachment 8986976 [details]
Bug 1470229 part 2 - Allow aria attributes.

https://reviewboard.mozilla.org/r/252228/#review259100

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06abfbc384f3
part 1 - Make the starting-with check in nsTreeSanitizer::SanitizeAttributes nicer. r=hsivonen
https://hg.mozilla.org/integration/autoland/rev/e17505b46d02
part 2 - Allow aria attributes. r=hsivonen

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/06abfbc384f3
https://hg.mozilla.org/mozilla-central/rev/e17505b46d02

Comment 9

[Maria Berlinger [:mberlinger], Services QA]

I have managed to reproduce this issue on an affected Firefox 62.0b2 (20180621152331) build using Windows 10 x64  by following the STR from comment 0. 

This issue is verified fixed using Firefox 63.0b7 (20180917143811) on the following OSes: Windows 10 x64, Ubuntu 18.04 x64, Windows 7 x64, macOS 10.13.6.