1149520 - Reader mode toolbar repaints during font size change

Status: RESOLVED FIXED

(Firefox Graveyard :: Reading List, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

It would be better to not do this. The suspected cause is having the font size change on a parent of the toolbar.

Comment 1

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

Attachment: The first attempt.

Okay, how does this look?

(Jeff: This is the patch that I showed you on the netbook earlier today.)

Thanks!

Comment 2

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

Comment on attachment 8586225 [details] [diff] [review]
The first attempt.

(It would be nice to know if this broke Mobile, while I'm at it…)

Comment 3

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

Attachment: The properly-based version.

After talking with Jeff, here's a version of the patch that doesn't depend on a patch that's going to get WONTFIXED…  ;)

(He also gave me a verbal f+, for what that's worth. :)

Comment 4

[:Margaret Leibovic]

Comment on attachment 8586251 [details] [diff] [review]
The properly-based version.

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

::: toolkit/themes/windows/global/aboutReader.css
@@ +83,5 @@
>    margin-top: 40px;
>    display: none;
>    text-align: center;
>    width: 100%;
> +  font-size: 0.9em;

On quick glance, you're going to need to update the mobile aboutReader.css as well, e.g.:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/themes/core/aboutReader.css#77

Comment 5

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

Attachment: A version with Android changes, too.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8586309 [details] [diff] [review]
A version with Android changes, too.

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

Built and tested on mobile, looks good to me.

Comment 7

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

Comment on attachment 8586309 [details] [diff] [review]
A version with Android changes, too.

Redirecting to Jaws.

Comment 8

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

Comment on attachment 8586309 [details] [diff] [review]
A version with Android changes, too.

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

I don't understand why all of the CSS changes needed to be made. Switching from `rem` to `em` is not a 1:1 switch, as `em` will have cascading effects. It looks like we could just take the AboutReader.jsm changes and be set here.

Comment 9

[:Margaret Leibovic]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Comment on attachment 8586309 [details] [diff] [review]
> A version with Android changes, too.
> 
> Review of attachment 8586309 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand why all of the CSS changes needed to be made. Switching
> from `rem` to `em` is not a 1:1 switch, as `em` will have cascading effects.
> It looks like we could just take the AboutReader.jsm changes and be set here.

I think the issue is that rem pulls from the font size set on the root html element, so changing the font size on the #container element instead of the root html element (the change in AboutReader.jsm) will cause the rem values to break.

However, you make a good point about the cascading em values, I didn't think about that. That's why I originally chose to use rem.

Comment 10

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

Yeah, what Margaret said.  So, what's the way forward here if we can't use rem and don't want to use em?  Should I go through and audit the usages of em to make sure they don't cascade?

Comment 11

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

Yeah, good point. We'll need to make some CSS changes as we can't reference the root element anymore. There is a cascade effect present in this patch because the font-size is defined for `.content` and then redefined for `.content h1` but the same values are used from the `rem` implementation.

Comment 12

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

Attachment: With css tweaked to account for ems.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
> Yeah, good point. We'll need to make some CSS changes as we can't reference
> the root element anymore. There is a cascade effect present in this patch
> because the font-size is defined for `.content` and then redefined for
> `.content h1` but the same values are used from the `rem` implementation.

So, I did have to tweak all the line-heights, because those were picking up the font-size in ems.
But the cascading for .content is okay, because it's defined as "1em", so there's no cascade for sub-elements.  (Or rather, there is a cascade, but it's all based off of 1, so it's okay.)

I did a test with the values I changed, and font-size5, and the values I got seem reasonable:

// For font-size5 == 18px;

 .message {
-  font-size: 0.9rem;  // 16.2px
+  font-size: 0.9em;   // 16.2px
 }

 .domain {
-  font-size: 0.9rem;     // 16.2px
-  line-height: 1.33rem;  // 23.94px
+  font-size: 0.9em;      // 16.2px
+  line-height: 1.33em;   // 23.98px
 }
 
 .header > h1 {
-  font-size: 1.33rem;    // 23.94px
-  line-height: 1.66rem;  // 29.88px
+  font-size: 1.33em;     // 23.93px
+  line-height: 1.66em;   // 29.92px
 }
 
 .header > .credits {
-  font-size: 0.9rem;     // 16.2px
-  line-height: 1.33rem;  // 23.94px
+  font-size: 0.9em;      // 16.2px
+  line-height: 1.33em;   // 23.98px
 }
 
 .content {
-  font-size: 1rem;        // 18px
-  line-height: 1.6rem;    // 28.8px
+  font-size: 1em;         // 18px
+  line-height: 1.6em;     // 28.8px
 }
 
 .content h1 {
-  font-size: 1.33rem;
-  line-height: 1.66rem;
+  font-size: 1.33em;
+  line-height: 1.66em;
 // 23.94px / 29.88px
 // UNTESTED
}
 
 .content h2 {
-  font-size: 1.1rem;
-  line-height: 1.66rem;
+  font-size: 1.1em;
+  line-height: 1.66em;
 // 19.8px / 29.88px
 // UNTESTED
}
 
 .content h3 {
-  font-size: 1rem;         // 18px
-  line-height: 1.66rem;    // 29.88px
+  font-size: 1em;          // 18px
+  line-height: 1.66em;     // 29.88px
 }
 
 .content .caption,
 .content .wp-caption-text,
 .content figcaption {
-  font-size: 0.9rem;
-  line-height: 1.33rem;
+  font-size: 0.9em;
+  line-height: 1.33em;
   font-style: italic;
 // 16.2px / 23.94px
 // UNTESTED
 }


If you'ld feel better, we could replace ".content" with "#reader-content" or "#container > .content" to make sure it only matches the top level…

Comment 13

[:Margaret Leibovic]

(In reply to Blake Winton (:bwinton) from comment #12)

> If you'ld feel better, we could replace ".content" with "#reader-content" or
> "#container > .content" to make sure it only matches the top level…

Given the fact that Readability.js doesn't strip class names, this sounds like a very good idea.

Comment 14

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

(In reply to :Margaret Leibovic from comment #13)
> (In reply to Blake Winton (:bwinton) from comment #12)
> 
> > If you'ld feel better, we could replace ".content" with "#reader-content" or
> > "#container > .content" to make sure it only matches the top level…
> 
> Given the fact that Readability.js doesn't strip class names, this sounds
> like a very good idea.

Seconded, but we should add a more unique namespace still. I think .aboutreader-content or .moz-aboutreader-content will be good.

Comment 15

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

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #14)
> (In reply to :Margaret Leibovic from comment #13)
> > (In reply to Blake Winton (:bwinton) from comment #12)
> > 
> > > If you'ld feel better, we could replace ".content" with "#reader-content" or
> > > "#container > .content" to make sure it only matches the top level…
> > 
> > Given the fact that Readability.js doesn't strip class names, this sounds
> > like a very good idea.

Does it strip ids, or could we run into conflicts with "#reader-content"?
(Alternatively, _could_ it strip specific ids?)

> Seconded, but we should add a more unique namespace still. I think
> .aboutreader-content or .moz-aboutreader-content will be good.

How about "#moz-reader-content"?  (Having "about" in there feels a little like an implementation detail to me.)

Comment 16

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

After we know what is supposed to be visible text (removing things like .visually-hidden), I don't see why we would want to pull in element attributes from the original page, but this is starting to sound like a separate bug.

#moz-reader-content is fine with me.

Comment 17

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

Attachment: Now using an id instead!

Okay, so this was a big enough change that I think a second review pass makes sense, just to see if I caught all the cases you were thinking of…

Comment 18

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

Comment on attachment 8589207 [details] [diff] [review]
Now using an id instead!

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

::: mobile/android/themes/core/aboutReader.css
@@ +146,5 @@
>  /* This covers caption, domain, and credits
>     texts in the reader UI */
>  
> +#moz-reader-content .wp-caption-text,
> +#moz-reader-content figcaption,

Actually, now that I think about this more I don't think we ever have a figcaption or .wp-caption-text outside of the reader-content. Do we need to reference this ID in the selector?

Comment 19

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

Comment on attachment 8589207 [details] [diff] [review]
Now using an id instead!

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

Let's not remove the ID from the selectors, trying to keep changes smaller since this will be getting uplifted.

Comment 20

[:Margaret Leibovic]

Comment on attachment 8589207 [details] [diff] [review]
Now using an id instead!

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

::: toolkit/themes/windows/global/aboutReader.css
@@ +168,3 @@
>  }
>  
>  .content a {

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?

Comment 21

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

So, Jaws said that he wouldn't mind using a more unique class (like .moz-reader-content), so I think the good-first-bug will suggest that instead, if that's okay with you.

Comment 22

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

Comment on attachment 8589207 [details] [diff] [review]
Now using an id instead!

Approval Request Comment
[Feature/regressing bug #]: reading list
[User impact if declined]: the animations will be more janky when changing font size.
[Describe test coverage new/current, TreeHerder]: Manual
[Risks and why]: reasonably low risk, because it's mostly css changes.
[String/UUID change made/needed]: none.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/624b3ac9abf6

Comment 24

[Marco Mucci [:MarcoM]]

Hi Blake, can you provide a point value.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8589207 [details] [diff] [review]
Now using an id instead!

Should be in 38 beta 3

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a8383fdd860

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ab02e48d0c2