1152143 - Make the aboutReader.css "#moz-reader-content" and ".content" consistent.

Status: RESOLVED INVALID

(Firefox Graveyard :: Reading List, defect)

Description

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

In bug 1149520, Margaret said "It's a bit confusing that we didn't replace all the .content selectors with #moz-reader-content, since developers looking at this file in the future won't have the context around the font-size change. Perhaps file a [good first bug] for that?"

I believe the consensus is to move everything to a more unique class, in particular ".moz-reader-content".  Don't forget to change both the android and windows/global version.  :)

Comment 1

[Tyler Steiman]

Attachment: bug1152143_moz-reader-content_change.diff

If I understand the problem correctly this patch should make everything match up. If not please let me know if I'm missing something.

Comment 2

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

Looks good to me.  Let's see what adw thinks…

Comment 3

[Tyler Steiman]

(In reply to Blake Winton (:bwinton) from comment #2)
> Looks good to me.  Let's see what adw thinks…

Hey Blake,

Are you a reviewer? I'm a little confused when it comes to the mentor/reviewer as I've been previously told to not send my patches to the mentor... just curious. Thanks!

Comment 4

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

Nope.  I'm a mentor, reporter, and someone who likes giving his opinion, but I'm not a reviewer.  ;)

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

I can review the patches for this bug.

Comment 6

[Tyler Steiman]

Comment on attachment 8615210 [details] [diff] [review]
bug1152143_moz-reader-content_change.diff

Review of attachment 8615210 [details] [diff] [review]:
-----------------------------------------------------------------

Bug 1152143 - Creates .moz-reader-content class, making the class information in aboutReader.html consistent to what is in the .css files.

Comment 7

[Tyler Steiman]

Hey Jared, have you been able to take a look at the patch? Just curious & thanks!

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8615210 [details] [diff] [review]
bug1152143_moz-reader-content_change.diff

Review of attachment 8615210 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/reader/content/aboutReader.html
@@ +21,5 @@
>        <h1 id="reader-title"></h1>
>        <div id="reader-credits" class="credits"></div>
>      </div>
>  
> +    <div class="moz-reader-content">

This is a bit confusing because there is a <div> four lines lower that has id="moz-reader-content". I don't think we should have a <div class="moz-reader-content"/> as well as a different <div id="moz-reader-content"/>

This would mean that the CSS changes you've made are probably for the wrong div, since you're changing `#moz-reader-content` to `.moz-reader-content` in the CSS files, where this used to be `.content`.

Comment 9

[Tyler Steiman]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Comment on attachment 8615210 [details] [diff] [review]
> bug1152143_moz-reader-content_change.diff
> 
> Review of attachment 8615210 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/reader/content/aboutReader.html
> @@ +21,5 @@
> >        <h1 id="reader-title"></h1>
> >        <div id="reader-credits" class="credits"></div>
> >      </div>
> >  
> > +    <div class="moz-reader-content">
> 
> This is a bit confusing because there is a <div> four lines lower that has
> id="moz-reader-content". I don't think we should have a <div
> class="moz-reader-content"/> as well as a different <div
> id="moz-reader-content"/>
> 
> This would mean that the CSS changes you've made are probably for the wrong
> div, since you're changing `#moz-reader-content` to `.moz-reader-content` in
> the CSS files, where this used to be `.content`.

What would be the appropriate way to fix this? Blake in the OP stated there was a consensus to move everything to a '.moz-reader-content' class. Maybe I'm understanding the problem incorrectly but the information I received was to replace the .content selector and create a .moz-reader-content

Thoughts?

Comment 10

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

I think Margaret will have much better thoughts than I will here, so let's rely on her…  ;)

Comment 11

[:Margaret Leibovic]

Sorry for the slow response!

Now that we're using scoped style sheets, we don't need to worry as much about prefixing the different ids/class names. In fact, since this bug was filed, we no longer use .content anywhere in our desktop CSS selectors, although I do see us still using it in the mobile CSS:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css#100

and also in our JS here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm#130

However, since the scoped style change, .content and #moz-reader-content are actually different elements.

To fix this bug, I would just try removing the .content class altogether, and replacing places where we use .content in CSS selectors with #moz-reader-content.

To fix this bug, I would actually just remove the .content class altogether

Comment 12

[Tyler Steiman]

Attachment: moz-reader-content_change updates

Margaret,

I've attached an updated patch based on your comment. In aboutReader.html: 
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/content/aboutReader.html?force=1#25

I changed to: 

    <div class="moz-reader-content">
      <style scoped>
        @import url("chrome://global/skin/aboutReaderContent.css");
      </style>
      <div id="moz-reader-content"></div>
    </div>

Should these stay the way it is in the patch or should they both just be id's? Thanks for the help!!

Comment 13

[:Margaret Leibovic]

Comment on attachment 8621972 [details] [diff] [review]
moz-reader-content_change updates

Review of attachment 8621972 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me, but I haven't had the chance to build and test this on Android yet. I just have one piece of feedback.

Also, can you test an article with large images to make sure that your AboutReader.jsm change didn't break anything? I think this is a good article to test:
https://medium.com/backchannel/first-alan-adler-invented-the-aerobie-now-he-s-created-the-perfect-cup-of-coffee-c5e94ccc538e

You should verify that we go through this logic block properly:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/reader/AboutReader.jsm#711

::: toolkit/components/reader/content/aboutReader.html
@@ +21,5 @@
>        <h1 id="reader-title"></h1>
>        <div id="reader-credits" class="credits"></div>
>      </div>
>  
> +    <div class="moz-reader-content">

This new class name isn't used anywhere in your patch, so you should just remove this class attribute altogether.

Comment 14

[Tyler Steiman]

Attachment: bug1152143_moz-reader-content_change.diff

Margaret, 

Thanks for the feedback, I have removed the class selector all together, just wanted to be sure I was getting mixed info in terms of what was going on so I'm sorry if I'm making this a slower process than it needs to be.

After building I tested for large images and it indeed displayed correctly although now I want a cup of that guy's coffee.

Comment 15

[Tyler Steiman]

Margaret,

Have you been able to look over the updated patch? Just checking in, thanks!

Muhsin

Comment 16

[:Margaret Leibovic]

Comment on attachment 8622827 [details] [diff] [review]
bug1152143_moz-reader-content_change.diff

Review of attachment 8622827 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry it took me so long to test this and review it! I did find a small problem, so we should update this before landing.

::: mobile/android/themes/core/aboutReader.css
@@ +96,5 @@
>  body.dark > .container > .header > .credits {
>    color: #aaaaaa;
>  }
>  
> +body.light > .container > #moz-reader-content .caption,

Oh, this doesn't actually work anymore, since #moz-reader-content isn't a direct descendant of .container.

We could replace this with:
body.light > .container > div > #moz-reader-content

Or, I think it would be better to just follow what we do on desktop [1], and avoid these long parent chains by replacing them with something like:

body.light .caption

This should be fine, since these selectors wouldn't match anything in the UI that we create.

[1] http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/aboutReader.css?force=1#114

Comment 17

[Tyler Steiman]

Attachment: bug1152143_moz-reader-content_change.diff

Margaret,

Assuming since the rest of Android code in the last patch contained #moz-reader-content being called from .container I'm assuming the rest of the code needed to be updated to your recommendation as well so this patch reflects that. If not let me know, I hope I understood correctly. Thanks!

Comment 18

[:Margaret Leibovic]

Comment on attachment 8626771 [details] [diff] [review]
bug1152143_moz-reader-content_change.diff

Review of attachment 8626771 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/themes/core/aboutReader.css
@@ +96,5 @@
>  body.dark > .container > .header > .credits {
>    color: #aaaaaa;
>  }
>  
> +body.light > .caption,

This isn't going to work, because we want this style to apply to all descendant nodes that match the .cation selector, not just direct descendants.

You can read these MDN articles for more details about how these different types of CSS selectors work:
https://developer.mozilla.org/en-US/docs/Web/CSS/Child_selectors
https://developer.mozilla.org/en-US/docs/Web/CSS/Descendant_selectors

Comment 19

[Tyler Steiman]

I'm currently between some things so I may not be able to finish this patch up soon.

Comment 20

[Chirath R [:|chirath]]

I would like to work on this bug.Can anyone please assign me to it?.

Comment 21

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

Consider it assigned!  

Comment 22

[Soumya Lahiri]

Hello, I'd like to work on this. Just to confirm,
https://searchfox.org/mozilla-central/source/toolkit/themes/shared/aboutReader.css
is the file I should be looking into right?

Comment 23

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

That's a good question, but I'm afraid at this point I have no idea.
Jaws or Gijs might be able to provide more information… Let's see what they think.

Comment 24

[:Gijs (he/him)]

I'm afraid there's nothing left to do here. Things have changed a lot in the last 5 years, and we're using a .moz-reader-content class in toolkit, and the android file is unused (and should actually be removed from the repo, I suspect).