Fix articles showing cookie information in reader mode #746
Conversation
tigleym
requested review from
niklasbaumgardner and
tigleym
and removed request for
niklasbaumgardner
Readability.js
Outdated
| // User is not able to see elements applied with both "aria-modla = true" and "role = dialog" | ||
| if (node.getAttribute("aria-modal") == "true" && node.getAttribute("role") == "dialog") { | ||
| node = this._removeAndGetNext(node); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This should be added to the check in isProbablyVisible.
There was a problem hiding this comment.
We should remove this check since it should be handled in the one above now
EDIT
On second thought, perhaps it makes sense to keep this check separate. Let's keep this here and remove the check added in isProbablyVisible. Sorry about that!
…dability into fix_article_show_cookie_info
Readability.js
Outdated
| @@ -2202,7 +2208,9 @@ Readability.prototype = { | |||
| return (!node.style || node.style.display != "none") | |||
| && !node.hasAttribute("hidden") | |||
| //check for "fallback-image" so that wikimedia math images are displayed | |||
| && (!node.hasAttribute("aria-hidden") || node.getAttribute("aria-hidden") != "true" || (node.className && node.className.indexOf && node.className.indexOf("fallback-image") !== -1)); | |||
| && (!node.hasAttribute("aria-hidden") || node.getAttribute("aria-hidden") != "true" || (node.className && node.className.indexOf && node.className.indexOf("fallback-image") !== -1)) | |||
| // user can not see element applied with both attribute "aria-modla = true" and "role = dialog" | |||
Readability.js
Outdated
| // User is not able to see elements applied with both "aria-modla = true" and "role = dialog" | ||
| if (node.getAttribute("aria-modal") == "true" && node.getAttribute("role") == "dialog") { | ||
| node = this._removeAndGetNext(node); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
We should remove this check since it should be handled in the one above now
EDIT
On second thought, perhaps it makes sense to keep this check separate. Let's keep this here and remove the check added in isProbablyVisible. Sorry about that!
|
I think all the review comments were addressed so let's get this merged. |
|
@tthhtao sorry this sat for such a long time! |
Applying the
aria-modalproperty to an element withrole="dialog"replaces the technique of usingaria-hiddenin the background. So I think this kind of element should also be considered "invisible" to the reader.