Polish the shadow and corners of the reader view panel

VERIFIED FIXED in Firefox 53

Status

()

Toolkit
Reader Mode
P5
normal
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: ntim, Assigned: Andrew Krawchyk, Mentored)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 verified)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
The reader view panel looks unpolished :
- it's shadow is incredibly thick and doesn't match the panels we currently have in the chrome
- it's border-radius is also very big, and it also doesn't match the panels we have in the chrome (and the platform styles as well).

Comment 1

3 years ago
mmaslaney, can you specify some of the details about what the panel shadow/border-radius should be? I think I just guessed based on what the old Android styles were, but we should fix this :)
Mentor: margaret.leibovic
Flags: needinfo?(mmaslaney)
Whiteboard: [lang=css]
Are we talking about the sidebar controls, or the type controls specifically?

For sidebar controls: https://projects.invisionapp.com/d/main#/console/2556448/57292979/preview

For Type Controls: http://invis.io/XT21LURF8

The Type Control panel should mimic the shadow and radius of the menu dropdown per platform. So, Windows would not have a radius. I believe iOS has a Radius of 4px, though it may be 8px on retina screens.
Flags: needinfo?(mmaslaney)

Comment 3

3 years ago
Hi,I would like to take this up as my first bug.Can I be guided through this please? I have the nightly release built on my system. 

Thanks :)

Comment 4

3 years ago
(In reply to ANKIT JENA from comment #3)
> Hi,I would like to take this up as my first bug.Can I be guided through this
> please? I have the nightly release built on my system. 
> 
> Thanks :)

Hi Ankit! First of all, you should try reproducing this issue locally. You can test reader view by setting "reader.parse-on-load.enabled" to true in about:config, and then navigate to a webpage that we detect as an article, and click on the reader view button in the toolbar.

After that, you should take a look at the font style popup in the controls. You can use the regular web developer inspector tool to inspect the HTML/CSS here.

After that, you should start making CSS changes to this file:
http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/aboutReader.css

You should try to get the design to match what mmaslaney specified in comment 2. If you're unsure if things are looking right, you should post screenshots for him to review. Once you're both happy with how things are looking, you can generate a patch for me to review.

Good luck!
Assignee: nobody → mas1ankit
Priority: -- → P5
Hi Ankit, have you been able to make progress on this bug?
Flags: needinfo?(mas1ankit)

Comment 6

3 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Hi Ankit, have you been able to make progress on this bug?

Hi Jared,I'm sorry I have been held up by other commitments.You may assign this one to someone else.
Flags: needinfo?(mas1ankit)

Comment 7

3 years ago
Hey,
Can I work on this bug?
Flags: needinfo?(margaret.leibovic)

Comment 8

3 years ago
bwinton has been doing a lot of work on this panel, and I'm not sure if this bug is still valid.

bwinton, are there still issues to address here?
Flags: needinfo?(margaret.leibovic) → needinfo?(bwinton)
(Reporter)

Comment 9

3 years ago
The style looks better on OSX, but still doesn't fit right on Windows, mainly because of the thick shadow and the gray background which are not consistent with the australis panels.
The work I've been doing has been on the inside of the panel.  I believe Michael's final paragraph in comment #2 still applies:

> The Type Control panel should mimic the shadow and radius of the menu
> dropdown per platform. So, Windows would not have a radius. I believe iOS
> has a Radius of 4px, though it may be 8px on retina screens.
Flags: needinfo?(bwinton)

Comment 11

3 years ago
(In reply to Vaibhav Bhosale from comment #7)
> Hey,
> Can I work on this bug?

Sounds like the answer here is "yes"!

I'm marking bwinton as a mentor as well, since he's been doing more work on this panel than I have recently.
Assignee: nobody → vaibhavbhosale15
Mentor: bwinton

Updated

3 years ago
Summary: Polish the reader view panel → Polish the shadow and corners of the reader view panel
Whiteboard: [lang=css] → [lang=css][reader-ui]
I'd like to see this happen.  Unassigning to not camp interesting mentor tickets.  Matt, this might interest you as well -- a little context to parse and understand first :)
Assignee: vaibhavbhosale15 → nobody
Flags: needinfo?(mking)

Comment 13

3 years ago
(In reply to Nick Alexander :nalexander from comment #12)
> I'd like to see this happen.  Unassigning to not camp interesting mentor
> tickets.  Matt, this might interest you as well -- a little context to parse
> and understand first :)

FYI, this bug only affects desktop Firefox.

Comment 14

3 years ago
Will try to see if I can build Firefox desktop.
Flags: needinfo?(mking)

Comment 15

3 years ago
(In reply to Matt King from comment #14)
> Will try to see if I can build Firefox desktop.

The good news is that it's easier than building Firefox for Android :)

Let us know if you need help! The desktop Firefox front-end team hangs out in #fx-team on IRC, so that's a good place to ask questions if you have any.
(Assignee)

Comment 16

3 years ago
(In reply to Michael Maslaney [:mmaslaney] (mmaslaney@mozilla.com) from comment #2)
> Are we talking about the sidebar controls, or the type controls specifically?
> 
> For sidebar controls:
> https://projects.invisionapp.com/d/main#/console/2556448/57292979/preview
> 
> For Type Controls: http://invis.io/XT21LURF8
> 
> The Type Control panel should mimic the shadow and radius of the menu
> dropdown per platform. So, Windows would not have a radius. I believe iOS
> has a Radius of 4px, though it may be 8px on retina screens.

I need a login to view the sidebar controls, is it possible to make that public?
(Assignee)

Comment 17

3 years ago
Created attachment 8680737 [details] [diff] [review]
initial dropdown-popup polish for reader view

(In reply to :Margaret Leibovic from comment #4)
> After that, you should start making CSS changes to this file:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> aboutReader.css

I believe the styles have been moved to https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/shared/aboutReaderControls.css#125

It also looks like the border-radius has been changed to 4px, but I can't find that change. The farthest back I can track the aboutReaderControls.css file is to http://hg.mozilla.org/mozilla-central/rev/d8e9d0c53615 where it was created.

I've changed the box-shadow and the bottom-border-width on the .dropdown-popup to better reflect the comps in comment 2, and attached a patch 
to this bug.

(In reply to Blake Winton (:bwinton) (:☕️) from comment #10)
> The work I've been doing has been on the inside of the panel.  I believe
> Michael's final paragraph in comment #2 still applies:
> 
> > The Type Control panel should mimic the shadow and radius of the menu
> > dropdown per platform. So, Windows would not have a radius. I believe iOS
> > has a Radius of 4px, though it may be 8px on retina screens.

For removing the border-radius on Windows, I'm unsure of how to do that. There doesn't seem to be any reader styles in the https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global folder. Do I need to create a new one?
Attachment #8680737 - Flags: feedback?(margaret.leibovic)
(Assignee)

Comment 18

3 years ago
Created attachment 8680738 [details]
Screen Shot 2015-10-29 at 1.33.47 PM.png

This is a screenshot reflecting the proposed changes in comment 17.

Comment 19

3 years ago
Comment on attachment 8680737 [details] [diff] [review]
initial dropdown-popup polish for reader view

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

Thanks for the patch! This seems fine from a code perspective, but we should get a UX person to take a look as well to confirm the visual changes.
Attachment #8680737 - Flags: feedback?(margaret.leibovic) → feedback+

Comment 20

3 years ago
mmaslaney, are you still the right person to ask about reader view UI changes? Can you take a look at this screenshot to see if it matches what you were thinking?
Assignee: nobody → akrawchyk
Flags: needinfo?(mmaslaney)

Comment 21

2 years ago
Andrew, sorry the ball got dropped on this one. :-(

Blake, can you take a look in terms of UI/UX and/or push this forward to someone who's still looking at this? Thank you!
Flags: needinfo?(mmaslaney) → needinfo?(bwinton)
According to Shorlander (cc-ed), Przemek is someone who could look at this…
Flags: needinfo?(bwinton) → needinfo?(pabratowski)
Redirecting
Flags: needinfo?(pabratowski) → needinfo?(epang)

Updated

2 years ago
Whiteboard: [lang=css][reader-ui] → [lang=css][about-reader-ui]
Thanks Andrew! Overall this looks good but the arrow doesn't seem to be aligned correctly, it seems to be a couple of pixels too far to the right, Andrew can take a look at needinfo me when changed?

Just want to note a couple of items to keep in mind for later on: 

The corner radius should not be rounded on windows but speaking with Gijs and Shorlander it sounds like it's more complicated then it's worth for now.  

The drop shadow also seems to be using the OSX styling instead of the one used for windows panels but will also have the same complications as above.
Flags: needinfo?(epang) → needinfo?(akrawchyk)
(Assignee)

Comment 25

2 years ago
Created attachment 8772204 [details]
Screen Shot 2016-07-18 at 7.34.34 PM.png
Attachment #8680738 - Attachment is obsolete: true
(Assignee)

Comment 26

2 years ago
Created attachment 8772207 [details] [diff] [review]
1135915.patch

Eric, I agree with your feedback that the arrow is not aligned correctly. The issue seemed to be with the RM-Type-Controls-Arrow.svg file having extra background color bleeding through by 1px. I've adjusted that file and updated the aboutReaderControls.css file to address the change. I've attached an updated patch and an updated screenshot to this bug.

Regarding the cross-platform issues, I'm happy to work on them but not sure where to start. Please let me know what I should do next.
Attachment #8680737 - Attachment is obsolete: true
Flags: needinfo?(akrawchyk) → needinfo?(epang)
(Assignee)

Comment 29

2 years ago
Eric, I also realize I might need to update the generated SVG code by removing the Adobe Illustrator reference and adding back the Mozilla MPL notice.
(In reply to Andrew Krawchyk from comment #26)
> Created attachment 8772207 [details] [diff] [review]
> 1135915.patch
> 
> Eric, I agree with your feedback that the arrow is not aligned correctly.
> The issue seemed to be with the RM-Type-Controls-Arrow.svg file having extra
> background color bleeding through by 1px. I've adjusted that file and
> updated the aboutReaderControls.css file to address the change. I've
> attached an updated patch and an updated screenshot to this bug.
> 
> Regarding the cross-platform issues, I'm happy to work on them but not sure
> where to start. Please let me know what I should do next.

Hi Andrew, thanks for updating this!  It's looking much better but it's still not quite there.  The darkness of the outline of the arrow doesn't match the rest of the panel.  (if you look at other panels such as bookmarks you'll see that the outline is all consistent around the box and arrow).  Let me know if it's an asset problem or something you can fix.  If it's the asset, please let me know what I can do.  Regarding the cross platform issues, I don't think there's anything we can do atm so we'll just keep them in mind for the future :).
Flags: needinfo?(epang) → needinfo?(akrawchyk)
(Assignee)

Comment 31

2 years ago
Created attachment 8772506 [details]
Screen Shot 2016-07-19 at 2.31.04 PM.png
Attachment #8772204 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
Created attachment 8772507 [details] [diff] [review]
1135915.patch

Eric, I've updated the polish with more polish! The asset was the issue, but I hope I was able to handle it. There was too much opacity on the border, and the color did not match the dropdown's border. I've updated that and attached the updated screenshot and patch.
Attachment #8772207 - Attachment is obsolete: true
Flags: needinfo?(akrawchyk) → needinfo?(epang)
(Assignee)

Comment 33

2 years ago
Comment on attachment 8772211 [details]
Bug 1135915 - fix svg viewbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65072/diff/1-2/
(In reply to Andrew Krawchyk from comment #32)
> Created attachment 8772507 [details] [diff] [review]
> 1135915.patch
> 
> Eric, I've updated the polish with more polish! The asset was the issue, but
> I hope I was able to handle it. There was too much opacity on the border,
> and the color did not match the dropdown's border. I've updated that and
> attached the updated screenshot and patch.

From a UX/Visual perspective this looks good to me now (sorry for being so picky)! Thanks for your help with this Andrew!
Flags: needinfo?(epang)
(Assignee)

Comment 35

2 years ago
How's this bug doing Eric? Looks good to land?
Flags: needinfo?(epang)
(Reporter)

Comment 36

2 years ago
You'll need an review+ from a reviewer.
Flags: needinfo?(epang)
(Reporter)

Updated

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

Comment 37

2 years ago
mozreview-review
Comment on attachment 8772211 [details]
Bug 1135915 - fix svg viewbox;

https://reviewboard.mozilla.org/r/65072/#review91324

::: toolkit/themes/shared/aboutReaderControls.css:155
(Diff revision 2)
> -  width: 24px;
> +  width: 16px;
>    height: 24px;
>    background-image: url("chrome://global/skin/reader/RM-Type-Controls-Arrow.svg");
> +  background-size: cover;

This is confusing. The background image is 24x24px, according to the SVG viewbox, but the element is only 16px wide. I then also don't see any effect from background-size cover in a quick test: http://jsbin.com/havagovupi/edit?html,css,output

What am I missing? Can't we just make the SVG match the dimensions of the dropdown arrow?
Attachment #8772211 - Flags: review?(gijskruitbosch+bugs) → review-

Comment 38

2 years ago
Gonna add myself as a mentor so bugmail from this bug gets filtered and hopefully it won't be dropped a third time. :-(
Mentor: gijskruitbosch+bugs
Comment hidden (mozreview-request)
(Assignee)

Comment 40

a year ago
Thanks for the feedback Gijs! I adjusted the viewbox to be consistent with the CSS width, and removed the background-size: cover.

I've pushed an updated commit to the review. I'm not sure I did this correctly.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 41

a year ago
mozreview-review
Comment on attachment 8772211 [details]
Bug 1135915 - fix svg viewbox;

https://reviewboard.mozilla.org/r/65072/#review97892

Looks good, thanks!
Attachment #8772211 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED

Updated

a year ago
Flags: needinfo?(gijskruitbosch+bugs)

Comment 43

a year ago
(In reply to Pulsebot from comment #42)
> Pushed by gijskruitbosch@gmail.com:
> https://hg.mozilla.org/integration/autoland/rev/604725c4b3e0
> fix svg viewbox; r=Gijs

Ah, hm, I should have fixed the commit message before landing this. :-(
Too late now - sorry!

Comment 44

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/604725c4b3e0
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: qe-verify+

Comment 45

a year ago
I have reproduced this issue using Firefox 39.0a1 (ID=20150223103044) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 53.0b1, 55.0a1 on Win 8.1 x64 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.