If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Firefox 38.0.5

Status

()

Toolkit
Reader Mode
P3
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 wontfix, firefox38.0.5 verified, firefox39 fixed, firefox40 verified)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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)
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

3 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

3 years ago
Assignee: nobody → margaret.leibovic
(Assignee)

Comment 3

3 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

3 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

3 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

3 years ago
Created attachment 8593667 [details] [diff] [review]
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/
Attachment #8593667 - Flags: feedback?(gijskruitbosch+bugs)

Comment 7

3 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)
Priority: -- → P3
(Assignee)

Comment 8

3 years ago
Created attachment 8595967 [details] [diff] [review]
WIP - Used scoped styles for reader view browser UI

Oops, forgot the hg add.
Attachment #8593667 - Attachment is obsolete: true
Attachment #8595967 - Flags: feedback?(gijskruitbosch+bugs)

Comment 9

3 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

3 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

3 years ago
Created attachment 8596751 [details] [diff] [review]
(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).
Attachment #8595967 - Attachment is obsolete: true
Attachment #8596751 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 12

3 years ago
Created attachment 8596755 [details] [diff] [review]
(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.
Attachment #8596755 - Flags: review?(gijskruitbosch+bugs)

Comment 13

3 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

3 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

3 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+
(Assignee)

Updated

3 years ago
Blocks: 1158194

Comment 16

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/63669a3ca6c3
https://hg.mozilla.org/integration/fx-team/rev/d8e9d0c53615
(Assignee)

Comment 17

3 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

3 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+
https://hg.mozilla.org/mozilla-central/rev/63669a3ca6c3
https://hg.mozilla.org/mozilla-central/rev/d8e9d0c53615
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
QA Contact: andrei.vaida
(Assignee)

Comment 20

2 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
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

2 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

2 years ago
Attachment #8596755 - Flags: approval-mozilla-beta?
Attachment #8596755 - Flags: approval-mozilla-aurora?
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 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.
This needs rebasing :(
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 26

2 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)
(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.
I'll make sure this gets looked at, the moment builds are available for Beta 9.
status-firefox38: --- → fixed
status-firefox39: --- → fixed
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+
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
status-firefox40: fixed → verified
Setting the status flags accordingly this time, sorry about the noise.
status-firefox38: fixed → verified
status-firefox40: verified → fixed
How was this verified fixed on branches where these patches never landed?!?!
status-firefox38: verified → affected
status-firefox38.0.5: --- → affected
status-firefox39: fixed → affected
Flags: needinfo?(andrei.vaida)
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

2 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

2 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)
(Assignee)

Updated

2 years ago
Depends on: 1160577
(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.)
(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
Last Resolved: 2 years ago2 years ago
Flags: needinfo?(andrei.vaida)

Comment 38

2 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
status-firefox40: fixed → verified
Presumably we need to change the release approvals to beta approvals.
status-firefox38: affected → wontfix
Flags: needinfo?(sledru)
(Assignee)

Comment 40

2 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)
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
status-firefox39: affected → fixed
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 42

2 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

2 years ago
Created attachment 8601203 [details] [diff] [review]
(rebased for beta) Move reader content styles to scoped style sheet
(Assignee)

Comment 44

2 years ago
Created attachment 8601204 [details] [diff] [review]
(rebased for beta) Move reader controls styles to scoped style sheet
(Assignee)

Comment 45

2 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)
Per the closure message, 38.0.5 merged to release today. Please push there instead.
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-release/rev/80a9584ac5e4
https://hg.mozilla.org/releases/mozilla-release/rev/c64ca42b7490
status-firefox38.0.5: affected → fixed
You did it :) Clearing the ni
Flags: needinfo?(sledru)

Comment 49

2 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
status-firefox38.0.5: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.