1154028 - Make sure reader view UI styles don't accidentally get applied to readability content

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P3)

Description

[:Margaret Leibovic]

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?

Comment 1

[Blake Winton (:bwinton) (:☕️)]

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

[:Gijs (he/him)]

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.

Comment 3

[:Margaret Leibovic]

(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.

Comment 4

[:Gijs (he/him)]

(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.

Comment 5

[:Margaret Leibovic]

(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.

Comment 6

[:Margaret Leibovic]

Attachment: WIP - Used scoped styles for reader view browser UI

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/

Comment 7

[:Gijs (he/him)]

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...)

Comment 8

[:Margaret Leibovic]

Attachment: WIP - Used scoped styles for reader view browser UI

Oops, forgot the hg add.

Comment 9

[:Gijs (he/him)]

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?

Comment 10

[:Margaret Leibovic]

(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.

Comment 11

[:Margaret Leibovic]

Attachment: (Part 1) Move reader content styles to scoped style sheet

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).

Comment 12

[:Margaret Leibovic]

Attachment: (Part 2) Move reader controls styles to scoped style sheet

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.

Comment 13

[:Gijs (he/him)]

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.

Comment 14

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

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!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/63669a3ca6c3
https://hg.mozilla.org/integration/fx-team/rev/d8e9d0c53615

Comment 17

[:Margaret Leibovic]

I filed an issue here about preserving class names and ids in the reader content:
https://github.com/mozilla/readability/issues/184

Comment 18

[:Margaret Leibovic]

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.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63669a3ca6c3
https://hg.mozilla.org/mozilla-central/rev/d8e9d0c53615

Comment 20

[:Margaret Leibovic]

It looks like this did regression changing the font size, but bwinton is working on fixing that in bug 1158294.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This blocks various branch uplifts (or at least makes them more difficult). Can we please uplift to aurora/beta?

Comment 22

[:Margaret Leibovic]

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

Comment 23

[Sylvestre Ledru [:Sylvestre]]

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

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing :(

Comment 26

[:Margaret Leibovic]

(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...

Comment 27

[Ryan VanderMeulen [:RyanVM]]

(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

[Andrei Vaida [:avaida]]

I'll make sure this gets looked at, the moment builds are available for Beta 9.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8596755 [details] [diff] [review]
(Part 2) Move reader controls styles to scoped style sheet

[Triage Comment]

Comment 30

[Andrei Vaida [:avaida]]

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

Comment 31

[Andrei Vaida [:avaida]]

Setting the status flags accordingly this time, sorry about the noise.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

How was this verified fixed on branches where these patches never landed?!?!

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 34

[:Margaret Leibovic]

(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).

Comment 35

[:Margaret Leibovic]

(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.

Comment 36

[Blake Winton (:bwinton) (:☕️)]

(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

[Andrei Vaida [:avaida]]

(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.

Comment 38

[Ada [:adalucinet]]

(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

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Presumably we need to change the release approvals to beta approvals.

Comment 40

[:Margaret Leibovic]

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.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[:Margaret Leibovic]

Sorry about that, fixed a bustage fix:
https://hg.mozilla.org/releases/mozilla-aurora/rev/6ca442247173

Comment 43

[:Margaret Leibovic]

Attachment: (rebased for beta) Move reader content styles to scoped style sheet

Comment 44

[:Margaret Leibovic]

Attachment: (rebased for beta) Move reader controls styles to scoped style sheet

Comment 45

[:Margaret Leibovic]

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

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Per the closure message, 38.0.5 merged to release today. Please push there instead.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/80a9584ac5e4
https://hg.mozilla.org/releases/mozilla-release/rev/c64ca42b7490

Comment 48

[Sylvestre Ledru [:Sylvestre]]

You did it :) Clearing the ni

Comment 49

[Ada [:adalucinet]]

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