1132656 - Reader mode toolbar overlaps content if window becomes too narrow.

Status: VERIFIED FIXED

(Toolkit :: Reader Mode, defect, P2)

Description

[:Margaret Leibovic]

Follow-up to bug 1120735.

We probably need to add some extra padding to the left side of the body to account for the toolbar.

Comment 2

[Vaibhav Bhosale]

Hey Margaret,
Can I work on this bug?

Comment 3

[Vaibhav Bhosale]

(In reply to Vaibhav Bhosale from comment #2)
> Hey Margaret,
> Can I work on this bug?

Comment 4

[Vaibhav Bhosale]

(In reply to Vaibhav Bhosale from comment #2)
> Hey Margaret,
> Can I work on this bug?

Comment 5

[Vaibhav Bhosale]

Hey,
I apologise for the two random comments. Since my phone was hanged it happened.
I will try it won't happen again.

Comment 6

[Vaibhav Bhosale]

Hey Margaret,
Can you share some screen-shot where the problem lies.
Thanks!

Comment 7

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

(In reply to Vaibhav Bhosale from comment #6)
> Hey Margaret,
> Can you share some screen-shot where the problem lies.
> Thanks!

Hi Vaibhav, thank you for requesting to work on this bug. Unfortunately I have already begun working with someone else who will take this bug.

There are other bugs available that you can find at http://www.joshmatthews.net/bugsahoy/. I would recommend that if you find a bug that you want to work on, please do not hesitate to begin working on it and uploading your work to the bug. Many mentors do not assign a bug until a patch is uploaded because some people struggle with getting a build environment set up.

Comment 8

[Abhinav Koppula]

Attachment: toolbarOverlap.patch

Added media query to take care of the left padding.

Comment 9

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

Comment on attachment 8577132 [details] [diff] [review]
toolbarOverlap.patch

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

::: toolkit/themes/windows/global/aboutReader.css
@@ +10,5 @@
>  }
>  
> +@media screen and (max-width: 742px) {
> +  body {
> +    padding: 64px 0 64px 51px;

This is close but it will break for right-to-left (RTL) languages.

Please use the following:
padding-top: 64px;
-moz-padding-end: 0;
padding-bottom: 64px;
-moz-padding-start: 51px;

Comment 10

[Abhinav Koppula]

Attachment: toolbarOverlap.patch

Added moz-padding to make it generic for RTL languages

Comment 11

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

Comment on attachment 8577897 [details] [diff] [review]
toolbarOverlap.patch

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

::: toolkit/themes/windows/global/aboutReader.css
@@ +8,5 @@
>    margin-left: auto;
>    margin-right: auto;
>  }
>  
> +@media screen and (max-width: 742px) {

We shouldn't specify the `screen` media type here. With this patch on Windows 8.1, I was able to resize the window to a point where the toolbar still overlapped the content by a few pixels. Let's bump this up to 785px. With those changes, this should just be:
@media (max-width: 785px) {
  ...
}

Comment 12

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

Hi Abhinav, any update here?

Comment 13

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

Attachment: Patch

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a03a128298b0

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a03a128298b0

Comment 16

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 40.0a1 (2015-04-02), using Ubuntu 14.04 (x64), Windows 7 (x64) and Mac OS X 10.9.5.

Comment 17

[:Margaret Leibovic]

Comment on attachment 8586381 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: reader view
[User impact if declined]: reader view toolbar overlaps content if window is narrow
[Describe test coverage new/current, TreeHerder]: landed on nightly
[Risks and why]: low-risk small CSS change
[String/UUID change made/needed]: none

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bfd5c1d7c847

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b8343ae0a9dc

Comment 20

[Andrei Vaida [:avaida]]

Verified fixed on Beta 38.0b2-build1 (20150406174117) as well, using Windows 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Comment 21

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I don't think this needs additional verification on Firefox 39, since it was already verified on 38 and 40.