Closed Bug 1292536 Opened 6 years ago Closed 6 years ago

Desktop Reader View side bar on non-widescreen screens limits visibility over the text

Categories

(Toolkit :: Reader Mode, defect)

50 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox51 --- verified

People

(Reporter: me, Assigned: arai)

References

Details

Attachments

(2 files)

Attached image textBehind.png
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20160804004004

Steps to reproduce:

Just click on a random page that supports the Readers view.
The page I used is our universities home page.
https://www.uni-wuerzburg.de/startseite/


Actual results:

The side bar (with font, text-to-speech..) just limits the visibility of the text. An the vertical scroll bar can not be moved to see the text behind the sidebar.


Expected results:

The text to be moved or start from where the side bar end, instead of going behind the sidebar.
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
"padding-inline-start: 51px;" rule for body is added based on the absolute width of the body.

> @media (max-width: 785px) {
>   body {
>     padding-top: 64px;
>     padding-inline-end: 0;
>     padding-bottom: 64px;
>     padding-inline-start: 51px;
>   }
> }

but the rule cannot avoid the overlap when the width is a bit wider than 785px.
also, "785px" have no meaning as the font-size and the width of the content can be modifies by reader mode UI.

can we simply add the "padding-inline-start: 51px;" regardless of the width?
Status: UNCONFIRMED → NEW
Component: Untriaged → Reader Mode
Ever confirmed: true
Flags: needinfo?(jaws)
Product: Firefox → Toolkit
See Also: → 1132656
If we set the padding regardless of the width then the text wouldn't always be centered (it would be centered, then shifted by 51px). However, if we add 51px of padding to both sides of the document then it may fix this pretty easily.

Tooru, would you like to write a patch for this bug?
Flags: needinfo?(jaws) → needinfo?(arai.unmht)
Yes, I'll take this.

but I'm not sure where the "center" is, for this UI.

as there is vertical toolbar on one side, isn't "shifted" correct?
like, when there is a scrollbar, the "center" is also "shifted" to the other side.

anyway, will post a patch shortly.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Flags: needinfo?(arai.unmht)
Changed body's padding to "padding: 64px 51px;" regardless of width.
this patch adds padding 51px to both side (per comment #2)

Is there any kind of automated test that can be done for this change?
Attachment #8779517 - Flags: review?(jaws)
Comment on attachment 8779517 [details] [diff] [review]
Always add horizontal padding to Reader Mode body to avoid overlap with toolbar.

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

This looks good. There aren't any automated tests for these types of UI changes.
Attachment #8779517 - Flags: review?(jaws) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9b2065fa983504a628646c742a913ec3a1a3b71
Bug 1292536 - Always add horizontal padding to Reader Mode body to avoid overlap with toolbar. r=jaws
https://hg.mozilla.org/mozilla-central/rev/a9b2065fa983
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: qe-verify+
Since I am not be able to reproduce the initial issue using an old Aurora build from 2016-08-04 (because of the way Reader View is implemented), I can only test on latest builds to see if it reproduces anymore. 
Thus I verified that websites that support Reader View are properly aligned using Fx 51 beta 10 across platforms (Windows 10 64-bit, Mac OS X 10.12.2 and Ubuntu 16.04 64-bit).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.