800165 - Reader mode shows items that have display: none on the original page (like in Firefox support articles)

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect, P2)

Description

[Verdi [:verdi] Best to slack me]

https://support.mozilla.org/en-US/kb/zoom-and-out-websites-firefox-mobile this article contains a section that we show only to version 10 and lower of Firefox for android. When I enable reader mode that section is visible but it should not be.

Comment 1

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

Margaret, I thought this'd be fixed now that we have the CSS stuff... but it doesn't seem to be, e.g. https://support.mozilla.org/en-US/kb/how-do-i-set-up-firefox-sync - do you know why?

Comment 2

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #1)
> Margaret, I thought this'd be fixed now that we have the CSS stuff... but it
> doesn't seem to be, e.g.
> https://support.mozilla.org/en-US/kb/how-do-i-set-up-firefox-sync - do you
> know why?

If you inspect the content of that article, it looks like that hidden element has a style="display: none;" set on it. I would think that readability would throw away hidden elements like that, but maybe it doesn't... that seems like it would be a low-risk fix.

Maybe before we clear styles here, we should check to see if the element is display: none; and remove it if it is:
https://github.com/mozilla/readability/blob/master/Readability.js#L1060

Comment 3

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

(In reply to :Margaret Leibovic from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > Margaret, I thought this'd be fixed now that we have the CSS stuff... but it
> > doesn't seem to be, e.g.
> > https://support.mozilla.org/en-US/kb/how-do-i-set-up-firefox-sync - do you
> > know why?
> 
> If you inspect the content of that article, it looks like that hidden
> element has a style="display: none;" set on it. I would think that
> readability would throw away hidden elements like that, but maybe it
> doesn't... that seems like it would be a low-risk fix.
> 
> Maybe before we clear styles here, we should check to see if the element is
> display: none; and remove it if it is:
> https://github.com/mozilla/readability/blob/master/Readability.js#L1060

Yup, makes sense, let's do that.

Comment 4

[oneonestar]

Please revert bug 1144822 comment 13 after finishing this bug fix. bug 1144822 comment 13 will hide all the elements that have a common hidden class names, such as .hidden. The problem is a element with the class "hidden" *should not* be hidden if that there is no css saying that ".hidden {display:none}".

The current situation is:
<span style="display:none">A</span>
<span class="hidden">B</span>

The above code will display B in normal view which is correct.
But it will display A in reader view.
bug 1144822 comment 13 just hard coded to hide the common hidden class name and I believe this is wrong.

Comment 5

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

(In reply to oneonestar from comment #4)
> Please revert bug 1144822 comment 13 after finishing this bug fix. bug
> 1144822 comment 13 will hide all the elements that have a common hidden
> class names, such as .hidden. The problem is a element with the class
> "hidden" *should not* be hidden if that there is no css saying that ".hidden
> {display:none}".
> 
> The current situation is:
> <span style="display:none">A</span>
> <span class="hidden">B</span>
> 
> The above code will display B in normal view which is correct.
> But it will display A in reader view.
> bug 1144822 comment 13 just hard coded to hide the common hidden class name
> and I believe this is wrong.

There is no way for reader mode to know whether an element was hidden or not. It doesn't have access to the original document, so it won't know if .hidden involves display:none or not.

Comment 6

[:Margaret Leibovic]

I'm sorry, I haven't had time to work on this.

Comment 9

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

We now remove elements with inline styles that include display: none. Fixed in github via https://github.com/mozilla/readability/pull/285 and landed in m-c via bug 1467742 so fixed in 62. We haven't changed our handling of .hidden because, as noted, readability doesn't have access to the page CSS so doesn't know whether things are or aren't hidden.

Comment 10

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

(sumo pages don't have reader mode available anymore, I think that's either deliberate or tracked elsewhere)