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

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.

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

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/315763ca22c1 Reader View bottom margin too big for <li> elements, r= Gijs.

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