1218224 - Reader View bottom margin too big for <li> elements

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P2)

Description

[Franklin?]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151014143721

Steps to reproduce:

This page has a Table of Contents as a list:
https://ponyfoo.com/articles/es6-promises-in-depth
(archived: https://web.archive.org/web/20150928184517/http://ponyfoo.com/articles/es6-promises-in-depth)


Actual results:

<li> items have a large margin below. It looks bad.


Expected results:

It should look less bad.

---

Specifically, in chrome://global/skin/aboutReaderContent.css, we have this rule:
    p, code, pre, blockquote, ul, ol, li, figure, .wp-caption {
      margin: 0 0 30px 0;
    }

List items might want to use a smaller margin, since they're less of a bundle than paragraphs. Also, if we have:
    <li> text
        <ul>
            <li> more text </li>
        </ul>
    </li>

then there won't be a margin between "text" and "more text" (as shown in the first item on the Table of Contents of the example page).

Comment 1

[Rakhi(:rakhisharma)]

Attachment: Bug 1218224 - Reader View bottom margin too big for <li> elements, r= Gijs.

Review commit: https://reviewboard.mozilla.org/r/70434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70434/

Comment 2

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

Comment on attachment 8779449 [details]
Bug 1218224 - Reader View bottom margin too big for <li> elements, r= Gijs.

https://reviewboard.mozilla.org/r/70434/#review68162

::: toolkit/themes/shared/aboutReaderContent.css:105
(Diff revision 1)
>  code,
>  pre,
>  blockquote,
>  ul,
>  ol,
>  li,

You should remove "li" from this list here, as it will now add a margin-bottom of 20px for the last item.

Comment 3

[Rakhi(:rakhisharma)]

Comment on attachment 8779449 [details]
Bug 1218224 - Reader View bottom margin too big for <li> elements, r= Gijs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70434/diff/1-2/

Comment 4

[:Gijs (he/him)]

Comment on attachment 8779449 [details]
Bug 1218224 - Reader View bottom margin too big for <li> elements, r= Gijs.

https://reviewboard.mozilla.org/r/70434/#review70246

::: toolkit/themes/shared/aboutReaderContent.css:112
(Diff revision 2)
> +li:not(:last-child) {
> +  margin: -10px -10px 5px -10px;
> +}

The new patch doesn't have any padding set for the <li> anymore, so now the negative margins will pull text too close together, right?

Comment 5

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

Comment on attachment 8779449 [details]
Bug 1218224 - Reader View bottom margin too big for <li> elements, r= Gijs.

I haven't tested out the patch. Since Gijs already looked at it I'll defer to him.

Comment 6

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

Sorry for the churn on this. I decided to look closer at this to help you out some more since I may have lead you in the wrong direction earlier. My apologies.

I think we really should just remove the big chunk of CSS that sets these styles on various elements. The page looks fine in reader mode to me without them, and I don't understand why we want these negative margins on so many elements in the first place.

I think we should remove this chunk of CSS,

> p,
> code,
> pre,
> blockquote,
> ul,
> ol,
> li,
> figure,
> .wp-caption {
>   margin: -10px -10px 20px -10px;
>   padding: 10px;
>   border-radius: 5px;
> }

We could then edit the following rule to add the border-radius there:

>  p > img:only-child,
>  p > a:only-child > img:only-child,
>  .wp-caption img,
>  figure img {
>    display: block;
>+   border-radius: 5px;
>  }

since I think the border-radius was only intended to apply to images within these elements.

Comment 7

[:Gijs (he/him)]

OK, so Jared and I talked a bunch about this, and I think we came to the conclusion that we should leave the rule Jared mentioned in comment #6 alone for now, because the padding and border-radius are used by the highlight of the "narrate" (text to speech) feature on reader mode pages.

To fix the styling for <li>, please add a rule that gives <li> elements margin-bottom: 0.

Then to improve things for nested lists, please add a rule using this selector:

li > ul,
li > ol {
}

to give those items margin-bottom -10px. This will reduce the space after the nested list items a bit.

Does that make sense?

Comment 8

[Rakhi(:rakhisharma)]

Comment on attachment 8779449 [details]
Bug 1218224 - Reader View bottom margin too big for <li> elements, r= Gijs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70434/diff/2-3/

Comment 9

[Rakhi(:rakhisharma)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb1749577228

Comment 10

[:Gijs (he/him)]

Comment on attachment 8779449 [details]
Bug 1218224 - Reader View bottom margin too big for <li> elements, r= Gijs.

https://reviewboard.mozilla.org/r/70434/#review70986

Hey Rakhi, this looks like it's the same patch as before, rather than the one based on the comments in comment 7?

Comment 11

[Rakhi(:rakhisharma)]

Comment on attachment 8779449 [details]
Bug 1218224 - Reader View bottom margin too big for <li> elements, r= Gijs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70434/diff/3-4/

Comment 12

[Rakhi(:rakhisharma)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf717247e284

Comment 13

[:Gijs (he/him)]

Comment on attachment 8779449 [details]
Bug 1218224 - Reader View bottom margin too big for <li> elements, r= Gijs.

https://reviewboard.mozilla.org/r/70434/#review71010

This looks OK to me.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/315763ca22c1
Reader View bottom margin too big for <li> elements, r= Gijs.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/315763ca22c1

Comment 16

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed using Firefox 51 beta 9 under Win 10 64-bit.