Reader mode toolbar repaints during font size change

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jrmuizel, Assigned: bwinton)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 40
x86
macOS
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

It would be better to not do this. The suspected cause is having the font size change on a parent of the toolbar.
Posted patch The first attempt. (obsolete) — Splinter Review
Okay, how does this look?

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

Thanks!
Attachment #8586225 - Flags: review?(bmcbride)
Attachment #8586225 - Flags: feedback?(jmuizelaar)
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…)
Attachment #8586225 - Flags: feedback?(margaret.leibovic)
Posted patch The properly-based version. (obsolete) — Splinter Review
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. :)
Attachment #8586225 - Attachment is obsolete: true
Attachment #8586225 - Flags: review?(bmcbride)
Attachment #8586225 - Flags: feedback?(margaret.leibovic)
Attachment #8586225 - Flags: feedback?(jmuizelaar)
Attachment #8586251 - Flags: review?(bmcbride)
Attachment #8586251 - Flags: feedback?(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
Attachment #8586251 - Attachment is obsolete: true
Attachment #8586251 - Flags: review?(bmcbride)
Attachment #8586251 - Flags: feedback?(margaret.leibovic)
Attachment #8586309 - Flags: review?(bmcbride)
Attachment #8586309 - Flags: review?(margaret.leibovic)
Status: NEW → ASSIGNED
Flags: qe-verify-
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.
Attachment #8586309 - Flags: review?(margaret.leibovic) → review+
Comment on attachment 8586309 [details] [diff] [review]
A version with Android changes, too.

Redirecting to Jaws.
Attachment #8586309 - Flags: review?(bmcbride) → review?(jaws)
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.
Attachment #8586309 - Flags: review?(jaws) → review-
(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.
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?
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Flags: needinfo?(margaret.leibovic)
(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…
Attachment #8586309 - Attachment is obsolete: true
Attachment #8588784 - Flags: review?(jaws)
(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.
(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.
(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.)
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(jaws)
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.
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(jaws)
Attachment #8588784 - Flags: review?(jaws) → review+
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…
Attachment #8588784 - Attachment is obsolete: true
Attachment #8589207 - Flags: review?(margaret.leibovic)
Attachment #8589207 - Flags: review?(jaws)
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 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.
Attachment #8589207 - Flags: review?(jaws) → review+
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?
Attachment #8589207 - Flags: review?(margaret.leibovic) → review+
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.
Keywords: checkin-needed
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.
Attachment #8589207 - Flags: approval-mozilla-beta?
Attachment #8589207 - Flags: approval-mozilla-aurora?
Blocks: 1152143
https://hg.mozilla.org/mozilla-central/rev/624b3ac9abf6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Hi Blake, can you provide a point value.
Blocks: 1132074
Iteration: --- → 40.1 - 13 Apr
Flags: needinfo?(bwinton)
Flags: firefox-backlog+
Points: --- → 3
Flags: needinfo?(bwinton)
Comment on attachment 8589207 [details] [diff] [review]
Now using an id instead!

Should be in 38 beta 3
Attachment #8589207 - Flags: approval-mozilla-beta?
Attachment #8589207 - Flags: approval-mozilla-beta+
Attachment #8589207 - Flags: approval-mozilla-aurora?
Attachment #8589207 - Flags: approval-mozilla-aurora+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.