Closed
Bug 1144822
Opened 10 years ago
Closed 10 years ago
hide the visually-hidden class of elements in reader view
Categories
(Toolkit :: Reader Mode, defect, P2)
Tracking
()
VERIFIED
FIXED
mozilla40
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 | ||
Updated•10 years ago
|
Blocks: desktop-reader
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → margaret.leibovic
Assignee | ||
Comment 1•10 years ago
|
||
Do you have a testcase where you've seen this in practice?
Flags: needinfo?(clarkbw)
Reporter | ||
Comment 2•10 years ago
|
||
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)
Reporter | ||
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
Bug 800165 is a similar issue.
Updated•10 years ago
|
Priority: -- → P2
Assignee | ||
Comment 6•10 years ago
|
||
/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 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
(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•10 years ago
|
||
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)
Comment 10•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Comment 11•10 years ago
|
||
(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)
Assignee | ||
Comment 12•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox38:
--- → wontfix
status-firefox38.0.5:
--- → affected
status-firefox39:
--- → affected
Updated•10 years ago
|
Attachment #8590541 -
Flags: approval-mozilla-beta?
Attachment #8590541 -
Flags: approval-mozilla-beta+
Attachment #8590541 -
Flags: approval-mozilla-aurora?
Attachment #8590541 -
Flags: approval-mozilla-aurora+
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
Updated•10 years ago
|
Updated•10 years ago
|
Flags: qe-verify+
Comment 15•10 years ago
|
||
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
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8590541 -
Attachment is obsolete: true
Attachment #8619803 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•