Closed Bug 1144822 Opened 5 years ago Closed 4 years ago

hide the visually-hidden class of elements in reader view

Categories

(Toolkit :: Reader Mode, defect, P2)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- verified
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: clarkbw, Assigned: Margaret)

References

Details

Attachments

(2 files, 1 obsolete file)

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; }
Assignee: nobody → margaret.leibovic
Do you have a testcase where you've seen this in practice?
Flags: needinfo?(clarkbw)
Attached image 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
Flags: needinfo?(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 )
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.
Bug 800165 is a similar issue.
Priority: -- → P2
/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/
Attachment #8590541 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8590541 - Flags: review?(gijskruitbosch+bugs) → review+
(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
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)
Flags: needinfo?(margaret.leibovic)
https://hg.mozilla.org/mozilla-central/rev/ca60d6b7caf2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(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.
Flags: needinfo?(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
Attachment #8590541 - Flags: approval-mozilla-beta?
Attachment #8590541 - Flags: approval-mozilla-aurora?
Attachment #8590541 - Flags: approval-mozilla-beta?
Attachment #8590541 - Flags: approval-mozilla-beta+
Attachment #8590541 - Flags: approval-mozilla-aurora?
Attachment #8590541 - Flags: approval-mozilla-aurora+
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
QA Contact: andrei.vaida
Attachment #8590541 - Attachment is obsolete: true
Attachment #8619803 - Flags: review+
You need to log in before you can comment on or make changes to this bug.