Closed Bug 1135915 Opened 5 years ago Closed 3 years ago

Polish the shadow and corners of the reader view panel

Categories

(Toolkit :: Reader Mode, defect, P5)

defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox53 --- verified

People

(Reporter: ntim, Assigned: akrawchyk, Mentored)

References

Details

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

Attachments

(3 files, 4 obsolete files)

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).
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)
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 :)
(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)
(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)
Hey,
Can I work on this bug?
Flags: needinfo?(margaret.leibovic)
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)
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)
(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
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)
(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.
Will try to see if I can build Firefox desktop.
Flags: needinfo?(mking)
(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.
(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?
(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)
Attached image Screen Shot 2015-10-29 at 1.33.47 PM.png (obsolete) —
This is a screenshot reflecting the proposed changes in comment 17.
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+
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)
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)
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)
Attached image Screen Shot 2016-07-18 at 7.34.34 PM.png (obsolete) —
Attachment #8680738 - Attachment is obsolete: true
Attached patch 1135915.patch (obsolete) — Splinter Review
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)
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)
Attachment #8772204 - Attachment is obsolete: true
Attached patch 1135915.patchSplinter Review
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)
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)
How's this bug doing Eric? Looks good to land?
Flags: needinfo?(epang)
You'll need an review+ from a reviewer.
Flags: needinfo?(epang)
Attachment #8772211 - Flags: review?(gijskruitbosch+bugs)
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-
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
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 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+
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
(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!
https://hg.mozilla.org/mozilla-central/rev/604725c4b3e0
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: qe-verify+
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.