1144822 - hide the visually-hidden class of elements in reader view

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P2)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

Readability seems to set a 'visually-hidden' class on certain, what appear to be superfluous elements, and my take is that we shouldn't be displaying these.

Can we add to our CSS a rule like:

.visually-hidden { display: none; }

Comment 1

[:Margaret Leibovic]

Do you have a testcase where you've seen this in practice?

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: fed-signals.png

This is a working test case, the attachment highlights just one of the areas that are hidden with this change.

http://www.nytimes.com/2015/03/19/business/economy/fed-interest-rates-fomc-meeting.html

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Ok, so it turns out this is coming from the content but we're likely to see a lot of it so if we add rules for the most common CSS frameworks we can properly hide a bunch of content our users shouldn't be seeing.

This is the distinct list from a couple of major vendors:

.visually-hidden, .visuallyhidden, .hidden, .invisible, .sr-only

Here's the research from a sample of the larger CSS frameworks:

html5-boilerplate
https://github.com/h5bp/html5-boilerplate/blob/v5.0.0/dist/doc/css.md#common-helpers
.visuallyhidden, .hidden, .invisible

yahoo ydn / a11yproject
https://developer.yahoo.com/blogs/ydn/clip-hidden-content-better-accessibility-53456.html
.visually-hidden, .hidden

bootstrap
http://getbootstrap.com/css/#helper-classes
.hidden, .invisible, .sr-only

zurb offers a custom solution without a rule name ( http://foundation.zurb.com/docs/accessibility.html )

Comment 4

[Justin Dolske [:Dolske]]

Ah, so it's not Readability setting this class (as comment 0 suggested), but rather the pages set these for A11Y-ish reasons.

I suppose eventually we could try to map chunks of our output back to the original document, see if they're hidden there, and remove/hide them in Reader View too. But a CSS hack for the common cases in comment 3 sounds like a cheap way to immediately make things incrementally better.

Comment 5

[Justin Dolske [:Dolske]]

Bug 800165 is a similar issue.

Comment 6

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1144822/margaret

/r/6811 - Bug 1144822 - Hide elements with common hidden class names in reader content. r=Gijs

Pull down this commit:

hg pull -r fcc915dbce07b135f27d4fcaaeda4f8cf8f2f47d https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[:Gijs (he/him)]

Comment on attachment 8590541 [details]
MozReview Request: bz://1144822/margaret

Can you file a followup bug to create a shared set of styles for desktop and mobile?

Comment 8

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #7)
> Comment on attachment 8590541 [details]
> MozReview Request: bz://1144822/margaret
> 
> Can you file a followup bug to create a shared set of styles for desktop and
> mobile?

Filed bug 1153371.

https://hg.mozilla.org/integration/fx-team/rev/ca60d6b7caf2

Comment 9

[:Gijs (he/him)]

Margaret, I just realized, maybe we also need to fix: https://github.com/mozilla/readability/blob/master/Readability.js#L805 ?

(we may want to refactor that class code altogether as I'm pretty sure it only cleans some nodes... it should ideally be all or nothing, IMO)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ca60d6b7caf2

Comment 11

[:Margaret Leibovic]

(In reply to :Gijs Kruitbosch from comment #9)
> Margaret, I just realized, maybe we also need to fix:
> https://github.com/mozilla/readability/blob/master/Readability.js#L805 ?
> 
> (we may want to refactor that class code altogether as I'm pretty sure it
> only cleans some nodes... it should ideally be all or nothing, IMO)

Oh, good call... yeah, I agree this should be all or nothing, and in the short term, I think we'd prefer nothing.

However, I'm now wondering if we're not properly prepared to maintain class names in the readability content. For example, if an element has a "domain" class, we're going to apply this style to it:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/aboutReader.css#106

We've been relying on the .content parent selector to prevent content-related styles from affecting our reader view UI, but we must not have thought about reader view UI styles affecting the content... I'll file another bug for that.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8590541 [details]
MozReview Request: bz://1144822/margaret

Gah, I missed this one.

Approval Request Comment
[Feature/regressing bug #]: reader view
[User impact if declined]: some elements will be visible when they should be hidden
[Describe test coverage new/current, TreeHerder]: no automated test coverage, baked in nightly
[Risks and why]: low-risk, small CSS tweak 
[String/UUID change made/needed]: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bbebbcae56f4

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/e4a70d181871

Comment 15

[Andrei Vaida [:avaida]]

Verified fixed on 38.0.5b1 (build2: 20150511143336), using Ubuntu 14.04 (x64). Used the working test case from Comment 2 along with a few other, to make sure that hidden elements remain hidden in Reader View.

Comment 17

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1144822 - Hide elements with common hidden class names in reader content. r=Gijs