Reader View bottom margin too big for <li> elements

VERIFIED FIXED in Firefox 51

Status

()

P2
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: leewangzhong, Assigned: rakhisharma, Mentored)

Tracking

(Blocks: 1 bug)

41 Branch
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: [about-reader-ui][lang=css])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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).
(Reporter)

Updated

3 years ago
Component: Untriaged → Reader Mode
OS: Unspecified → All
Product: Firefox → Toolkit
Hardware: Unspecified → All

Updated

2 years ago
Mentor: gijskruitbosch+bugs
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Whiteboard: [about-reader-ui][lang=css]

Updated

2 years ago
Blocks: 1286221

Updated

2 years ago
Assignee: nobody → Rakhish1994
Comment hidden (mozreview-request)
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.
Attachment #8779449 - Flags: review?(jaws) → review-
Comment hidden (mozreview-request)

Updated

2 years ago
Attachment #8779449 - Flags: review?(gijskruitbosch+bugs)

Comment 4

2 years ago
mozreview-review
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?
Attachment #8779449 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8779449 - Flags: review?(jaws) → review-
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

2 years ago
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?
Flags: needinfo?(Rakhish1994)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Flags: needinfo?(Rakhish1994)

Comment 10

2 years ago
mozreview-review
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?
Attachment #8779449 - Flags: review?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
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.
Attachment #8779449 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 14

2 years ago
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.

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/315763ca22c1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: qe-verify+
Verified as fixed using Firefox 51 beta 9 under Win 10 64-bit.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.