Closed
Bug 1154028
Opened 9 years ago
Closed 9 years ago
Make sure reader view UI styles don't accidentally get applied to readability content
Categories
(Toolkit :: Reader Mode, defect, P3)
Toolkit
Reader Mode
Tracking
()
VERIFIED
FIXED
mozilla40
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
Attachments
(4 files, 2 obsolete files)
20.66 KB,
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
20.81 KB,
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
11.96 KB,
patch
|
Details | Diff | Splinter Review | |
22.40 KB,
patch
|
Details | Diff | Splinter Review |
Right now we inconsistently remove class names from readability content. While preserving the class names allows us to do things like bug 1144822, I just realized that we don't take any measures to make sure our reader view UI styles won't be applied to readability content. For example, a preserved "domain" class name would result in these styles being applied: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/aboutReader.css#106 I think we could probably replace a lot of these with ids, since we only have single elements for a lot of the reader view UI. Gijs, do you know if we strip ids?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 1•9 years ago
|
||
I looked into this a little while ago, and we don't seem to strip ids, but it looks like we strip classes at https://dxr.mozilla.org/mozilla-central/source/toolkit/components/reader/Readability.js#814
Comment 2•9 years ago
|
||
I think bwinton is right. I think we should try to use scoped styles for the Reader UI styles, and avoid using classes for any of Readability's own output so that we don't need to bother stripping IDs or classes from the document.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → margaret.leibovic
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #2) > I think bwinton is right. > > I think we should try to use scoped styles for the Reader UI styles, and > avoid using classes for any of Readability's own output so that we don't > need to bother stripping IDs or classes from the document. One issue with this is that we have different styles for the desktop/mobile reader UI. Would we have two different scoped style elements and then just disable one? I'm worried that this would get kinda gross... Alternately, we could fork the HTML, but that could be kinda dangerous, as we share AboutReader.jsm. But that could actually help us avoid needing to hide elements with CSS that are only used by one platform. My main concern with either of these approaches is that they feel risky at this point.
Flags: needinfo?(gijskruitbosch+bugs)
Comment 4•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #3) > (In reply to :Gijs Kruitbosch from comment #2) > > I think bwinton is right. > > > > I think we should try to use scoped styles for the Reader UI styles, and > > avoid using classes for any of Readability's own output so that we don't > > need to bother stripping IDs or classes from the document. > > One issue with this is that we have different styles for the desktop/mobile > reader UI. Would we have two different scoped style elements and then just > disable one? I'm worried that this would get kinda gross... > > Alternately, we could fork the HTML, but that could be kinda dangerous, as > we share AboutReader.jsm. But that could actually help us avoid needing to > hide elements with CSS that are only used by one platform. > > My main concern with either of these approaches is that they feel risky at > this point. I think we can have two separate stylesheets and one <style scoped> element that CSS @imports the right one, either based on an ifdef or a media query. Does that make sense? The contents of the <style> element could be very small - just the @import - and then the CSS for the actual styles would remain in separate files. That'll keep the patch small and easy to review/assess for uplift as well.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #4) > (In reply to :Margaret Leibovic from comment #3) > > (In reply to :Gijs Kruitbosch from comment #2) > > > I think bwinton is right. > > > > > > I think we should try to use scoped styles for the Reader UI styles, and > > > avoid using classes for any of Readability's own output so that we don't > > > need to bother stripping IDs or classes from the document. > > > > One issue with this is that we have different styles for the desktop/mobile > > reader UI. Would we have two different scoped style elements and then just > > disable one? I'm worried that this would get kinda gross... > > > > Alternately, we could fork the HTML, but that could be kinda dangerous, as > > we share AboutReader.jsm. But that could actually help us avoid needing to > > hide elements with CSS that are only used by one platform. > > > > My main concern with either of these approaches is that they feel risky at > > this point. > > I think we can have two separate stylesheets and one <style scoped> element > that CSS @imports the right one, either based on an ifdef or a media query. > Does that make sense? The contents of the <style> element could be very > small - just the @import - and then the CSS for the actual styles would > remain in separate files. That'll keep the patch small and easy to > review/assess for uplift as well. Okay, that makes sense. I remember we did talk about this, sorry for forgetting! Investigating further, I'm not sure if this is really going to totally solve our problems... we do use class names for our type/color settings, and those are styles that we would want to apply to the content. Maybe we'll need to change our approach there to use attributes on the content container, rather than class names. Another issue is that the markup for our reader UI isn't nicely contained in a single element that's a sibling to the readability content, so I think we'll need multiple of these scoped style elements.
Assignee | ||
Comment 6•9 years ago
|
||
Here's a WIP... I feel like what we really need is a scoped style for the content element as well, and then we can get rid of all these .content and #moz-reader-content selectors. Perhaps we should just try to do everything with scoped styles... but then we'll have to address that font/color style concern. I also pushed this to reviewboard because it built on top of another patch, but reviewboard didn't publish it here for some reason (and it still sucks at hg renames): https://reviewboard.mozilla.org/r/7171/
Attachment #8593667 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 7•9 years ago
|
||
Comment on attachment 8593667 [details] [diff] [review] WIP - Used scoped styles for reader view browser UI Review of attachment 8593667 [details] [diff] [review]: ----------------------------------------------------------------- This patch seems to be missing aboutReader.css assuming that still exists (the override jar.mn entries are still there...)
Attachment #8593667 -
Flags: feedback?(gijskruitbosch+bugs)
Updated•9 years ago
|
Priority: -- → P3
Assignee | ||
Comment 8•9 years ago
|
||
Oops, forgot the hg add.
Attachment #8593667 -
Attachment is obsolete: true
Attachment #8595967 -
Flags: feedback?(gijskruitbosch+bugs)
Comment 9•9 years ago
|
||
Comment on attachment 8595967 [details] [diff] [review] WIP - Used scoped styles for reader view browser UI Review of attachment 8595967 [details] [diff] [review]: ----------------------------------------------------------------- I'm really sorry, but I'm confused... this still removes a lot of CSS without putting it back somewhere... how does that work?
Attachment #8595967 -
Flags: feedback?(gijskruitbosch+bugs)
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #9) > Comment on attachment 8595967 [details] [diff] [review] > WIP - Used scoped styles for reader view browser UI > > Review of attachment 8595967 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm really sorry, but I'm confused... this still removes a lot of CSS > without putting it back somewhere... how does that work? I'm sorry, I used |hg mv| to copy aboutReader.css to aboutReaderContent.css, but then I added back aboutReader.css. So basically, both of these files start from the existing aboutReader.css, then remove the stuff that's not relevant to them. I thought this would be better for version control history, as we would be able to see when styles were originally created. However, I'm not even sure that this approach is what we should do, given my comments in comment 6, but I haven't had a chance to think about this since last week.
Assignee | ||
Comment 11•9 years ago
|
||
For this bug, I decided to avoid touching the mobile styles. In the future I think this could lead to a shared content style sheet (bug 1153371).
Attachment #8595967 -
Attachment is obsolete: true
Attachment #8596751 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 12•9 years ago
|
||
I found that the scoped style sheets would not apply rules that required selectors outside of their scope (e.g. |.sans-serif .remove-button| needs to know about .sans-serif on the body), so I had to move some of those rules to aboutReader.css. I've tested these patches on various articles on both desktop and mobile, and things appear to be working properly, but we'll want to QA this to make sure we didn't break some edge case.
Attachment #8596755 -
Flags: review?(gijskruitbosch+bugs)
Comment 13•9 years ago
|
||
Comment on attachment 8596751 [details] [diff] [review] (Part 1) Move reader content styles to scoped style sheet Review of attachment 8596751 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/themes/shared/aboutReaderContent.css @@ +88,5 @@ > -moz-padding-start: 16px; > } > > +.light blockquote, > +.sepia blockquote { Do these continue to work? Your comment makes me think that they don't work here. @@ +93,4 @@ > -moz-border-start: 2px solid #333333; > } > > +.dark blockquote { Same.
Attachment #8596751 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 14•9 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #13) > Comment on attachment 8596751 [details] [diff] [review] > (Part 1) Move reader content styles to scoped style sheet > > Review of attachment 8596751 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/themes/shared/aboutReaderContent.css > @@ +88,5 @@ > > -moz-padding-start: 16px; > > } > > > > +.light blockquote, > > +.sepia blockquote { > > Do these continue to work? Your comment makes me think that they don't work > here. > > @@ +93,4 @@ > > -moz-border-start: 2px solid #333333; > > } > > > > +.dark blockquote { > > Same. Ah, I see these disappeared in the other patch. Nevermind me!
Comment 15•9 years ago
|
||
Comment on attachment 8596755 [details] [diff] [review] (Part 2) Move reader controls styles to scoped style sheet Review of attachment 8596755 [details] [diff] [review]: ----------------------------------------------------------------- Gosh, splinter really needs a better way to deal with these kinds of patches. Anyway, this looks awesome. Thanks!
Attachment #8596755 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/63669a3ca6c3 https://hg.mozilla.org/integration/fx-team/rev/d8e9d0c53615
Assignee | ||
Comment 17•9 years ago
|
||
I filed an issue here about preserving class names and ids in the reader content: https://github.com/mozilla/readability/issues/184
Assignee | ||
Comment 18•9 years ago
|
||
Before uplifting this, I'd like to verify that it didn't cause any regressions in the reader view styles. I tested locally with various articles and testcases, but it would be good to get some more eyes testing this.
Flags: qe-verify+
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63669a3ca6c3 https://hg.mozilla.org/mozilla-central/rev/d8e9d0c53615
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•9 years ago
|
QA Contact: andrei.vaida
Assignee | ||
Comment 20•9 years ago
|
||
It looks like this did regression changing the font size, but bwinton is working on fixing that in bug 1158294.
Depends on: 1158294
Comment 21•9 years ago
|
||
This blocks various branch uplifts (or at least makes them more difficult). Can we please uplift to aurora/beta?
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8596751 [details] [diff] [review] (Part 1) Move reader content styles to scoped style sheet Approval Request Comment [Feature/regressing bug #]: Desktop reader view [User impact if declined]: We may accidentally apply the wrong styles to reader view content. [Describe test coverage new/current, TreeHerder]: Tested locally, landed on mozilla-central. No automated tests for these styles, so I would like QA verification. (One regression was fixed in bug 1158294) [Risks and why]: This should be low risk because it's just moving around existing styles, but there are a lot of changes, so we may have accidentally broken some styles (like what happened in bug 1158294). [String/UUID change made/needed]: none
Flags: needinfo?(margaret.leibovic)
Attachment #8596751 -
Flags: approval-mozilla-beta?
Attachment #8596751 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8596755 -
Flags: approval-mozilla-beta?
Attachment #8596755 -
Flags: approval-mozilla-aurora?
Comment 23•9 years ago
|
||
Comment on attachment 8596751 [details] [diff] [review] (Part 1) Move reader content styles to scoped style sheet [Triage Comment] Should be in 38 beta 9
Attachment #8596751 -
Flags: approval-mozilla-release+
Attachment #8596751 -
Flags: approval-mozilla-beta?
Attachment #8596751 -
Flags: approval-mozilla-aurora?
Attachment #8596751 -
Flags: approval-mozilla-aurora+
Comment 24•9 years ago
|
||
Comment on attachment 8596751 [details] [diff] [review] (Part 1) Move reader content styles to scoped style sheet Approved for uplift to aurora and beta. Looks like the risk is limited to reader view styles and we need this to take the other reader view patches.
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25) > This needs rebasing :( I think we should see what's landed to make sure we land changes in the order they landed. This is a big nasty patch, and I would rather not try to rebase it. Looking at the filelog, it looks like we're missing bug 1127234 on aurora, which is probably leading to this issue. Although I'm a bit confused about how bug 1149068 didn't fail to apply as well...
Flags: needinfo?(margaret.leibovic)
Comment 27•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #26) > Although I'm a bit confused about how bug 1149068 didn't fail to apply as well... I rebased around it.
Comment 28•9 years ago
|
||
I'll make sure this gets looked at, the moment builds are available for Beta 9.
status-firefox38:
--- → fixed
status-firefox39:
--- → fixed
Comment 29•9 years ago
|
||
Comment on attachment 8596755 [details] [diff] [review] (Part 2) Move reader controls styles to scoped style sheet [Triage Comment]
Attachment #8596755 -
Flags: approval-mozilla-release+
Attachment #8596755 -
Flags: approval-mozilla-beta?
Attachment #8596755 -
Flags: approval-mozilla-aurora?
Attachment #8596755 -
Flags: approval-mozilla-aurora+
Comment 30•9 years ago
|
||
I looked through most of the high-traffic websites [1] on Beta 38.0b9-build1 (20150429135941), but I didn't notice anything suspicious related to Reader View's page styling. I'm gonna go ahead and mark this fix verified, but I'll keep it on my radar so that it'll get further exposure in any upcoming regression testing performed on this area. [1] http://goo.gl/w3Syfr
Status: RESOLVED → VERIFIED
Comment 31•9 years ago
|
||
Setting the status flags accordingly this time, sorry about the noise.
Comment 32•9 years ago
|
||
How was this verified fixed on branches where these patches never landed?!?!
status-firefox38.0.5:
--- → affected
Flags: needinfo?(andrei.vaida)
Comment 33•9 years ago
|
||
Needs rebasing for Aurora uplift (even with bug 1127234 uplifted). Also note that bug 1127234 was only approved for beta, not release, so I'm not sure what that means for the approvals here.
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 34•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #33) > Needs rebasing for Aurora uplift (even with bug 1127234 uplifted). Also note > that bug 1127234 was only approved for beta, not release, so I'm not sure > what that means for the approvals here. Gah, maybe that's because of bug 1154063. I'll look into this and see if I can just rebase. Did this also not apply to beta? All the outstanding reader view fixes that need uplift only need to land on 38.0.5, so we don't need these on release, only beta (if I understand the way we're using these repos right now correctly).
Assignee | ||
Comment 35•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #34) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #33) > > Needs rebasing for Aurora uplift (even with bug 1127234 uplifted). Also note > > that bug 1127234 was only approved for beta, not release, so I'm not sure > > what that means for the approvals here. > > Gah, maybe that's because of bug 1154063. I'll look into this and see if I > can just rebase. Nope, that wasn't the cause. Looking at hg log for mozilla-central and aurora, it seems like there are a lot of different things that landed in different orders that's making this a pain. I can just rebase this patch, but I'm worried this is potentially concealing problems we have with uplifting. If we want to just uplift everything in these CSS files, maybe we should just land a changeset that copies what's in mozilla-central to aurora and beta, instead of dealing with individually uplifting all of these bugs. dolske, what do you think? Also, how many more outstanding reader view CSS polish bugs are there that we're trying to ship for 38.0.5? Maybe we should just want until the dust settles on all these changes on mozilla-central and then uplift them all with this copy/paste method.
Flags: needinfo?(margaret.leibovic) → needinfo?(dolske)
Comment 36•9 years ago
|
||
(In reply to :Margaret Leibovic from comment #35) > Also, how many more outstanding reader view CSS polish bugs are there that > we're trying to ship for 38.0.5? Maybe we should just want until the dust > settles on all these changes on mozilla-central and then uplift them all > with this copy/paste method. I only know of one, bug 1158289. (There are some more, but they've all landed on m-c already.)
Comment 37•9 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #32) > How was this verified fixed on branches where these patches never landed?!?! Right. It seems I misunderstood Comment 23 and Comment 24 here, sorry. I'll follow up on this when I have the test results for m-c.
Status: VERIFIED → RESOLVED
Closed: 9 years ago → 9 years ago
Flags: needinfo?(andrei.vaida)
Comment 38•9 years ago
|
||
(In reply to Andrei Vaida, QA [:avaida] from comment #37) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #32) > > How was this verified fixed on branches where these patches never landed?!?! > > Right. It seems I misunderstood Comment 23 and Comment 24 here, sorry. I'll > follow up on this when I have the test results for m-c. Verified as fixed on latest 40.0a1 (2015-05-03), under Windows 7 64 bit: *no* issues related to Reader View's page styling encountered while going through high-traffic websites via http://goo.gl/w3Syfr
Status: RESOLVED → VERIFIED
Comment 39•9 years ago
|
||
Presumably we need to change the release approvals to beta approvals.
Flags: needinfo?(sledru)
Assignee | ||
Comment 40•9 years ago
|
||
I'm just working on rebasing theses and I'll land on aurora/beta. I'll manually diff the CSS files with the ones on fx-team to make sure we're not missing anything.
Flags: needinfo?(dolske)
Comment 41•9 years ago
|
||
This was pushed to Aurora and is now burning Windows. Pretty sure it's a one-liner fix. I've tried pinging on IRC to no avail. Please fix ASAP. https://treeherder.mozilla.org/logviewer.html#?job_id=801493&repo=mozilla-aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/a28c77cdf7bc https://hg.mozilla.org/releases/mozilla-aurora/rev/3d89006cfd37
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 42•9 years ago
|
||
Sorry about that, fixed a bustage fix: https://hg.mozilla.org/releases/mozilla-aurora/rev/6ca442247173
Flags: needinfo?(margaret.leibovic)
Assignee | ||
Comment 43•9 years ago
|
||
Assignee | ||
Comment 44•9 years ago
|
||
Assignee | ||
Comment 45•9 years ago
|
||
I tried pushing to beta, but I got a merge day closure message, so I'll leave that to the sheriffs to take care of. Also, I manually verified the diffs for aurora and beta with these patches applied, and the differences should be covered by the outstanding bugs that require uplift: https://bugzilla.mozilla.org/show_bug.cgi?id=1158302 https://bugzilla.mozilla.org/show_bug.cgi?id=1158294 https://bugzilla.mozilla.org/show_bug.cgi?id=1158281 https://bugzilla.mozilla.org/show_bug.cgi?id=1160577 https://bugzilla.mozilla.org/show_bug.cgi?id=1158322 https://bugzilla.mozilla.org/show_bug.cgi?id=1154063
Flags: needinfo?(ryanvm)
Comment 46•9 years ago
|
||
Per the closure message, 38.0.5 merged to release today. Please push there instead.
Flags: needinfo?(ryanvm)
Comment 47•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-release/rev/80a9584ac5e4 https://hg.mozilla.org/releases/mozilla-release/rev/c64ca42b7490
Updated•9 years ago
|
Comment 49•9 years ago
|
||
Various high-traffic websites via http://goo.gl/w3Syfr were tested during our regression testing on 38.0.5b1 (build2: 20150511143336), across the following platforms: Windows 7 x64, Windows 8.1 x86, Mac OS X 10.9.5 and Ubuntu 14.04 x64. 3 new issues related to Reader View's page styling were found [1] and already tracked via Github. Based on the above results, marking accordingly. [1] https://github.com/mozilla/readability/issues/218 - Specific pages from washingtonpost.com are missing their byline information https://github.com/mozilla/readability/issues/219 - Image is scaled differently for same washingtonpost article https://github.com/mozilla/readability/issues/220 - France24.com missing content
Updated•9 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•