Closed
Bug 1135915
Opened 9 years ago
Closed 8 years ago
Polish the shadow and corners of the reader view panel
Categories
(Toolkit :: Reader Mode, defect, P5)
Toolkit
Reader Mode
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).
Comment 1•9 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•9 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•9 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
Updated•9 years ago
|
Priority: -- → P5
Comment 5•9 years ago
|
||
Hi Ankit, have you been able to make progress on this bug?
Flags: needinfo?(mas1ankit)
Comment 6•9 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)
Updated•9 years ago
|
Assignee: mas1ankit → nobody
Comment 8•9 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•9 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.
Comment 10•9 years ago
|
||
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•9 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•9 years ago
|
Summary: Polish the reader view panel → Polish the shadow and corners of the reader view panel
Updated•9 years ago
|
Whiteboard: [lang=css] → [lang=css][reader-ui]
Comment 12•9 years ago
|
||
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•9 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 15•9 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•9 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•9 years ago
|
||
(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•9 years ago
|
||
This is a screenshot reflecting the proposed changes in comment 17.
Comment 19•9 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•9 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•8 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)
Comment 22•8 years ago
|
||
According to Shorlander (cc-ed), Przemek is someone who could look at this…
Flags: needinfo?(bwinton) → needinfo?(pabratowski)
Updated•8 years ago
|
Whiteboard: [lang=css][reader-ui] → [lang=css][about-reader-ui]
Comment 24•8 years ago
|
||
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•8 years ago
|
||
Attachment #8680738 -
Attachment is obsolete: true
Assignee | ||
Comment 26•8 years ago
|
||
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 27•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/65072/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/65072/
Assignee | ||
Comment 28•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ba66190dc26
Assignee | ||
Comment 29•8 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.
Comment 30•8 years ago
|
||
(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•8 years ago
|
||
Attachment #8772204 -
Attachment is obsolete: true
Assignee | ||
Comment 32•8 years ago
|
||
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•8 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/
Comment 34•8 years ago
|
||
(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•8 years ago
|
||
How's this bug doing Eric? Looks good to land?
Flags: needinfo?(epang)
Reporter | ||
Updated•8 years ago
|
Attachment #8772211 -
Flags: review?(gijskruitbosch+bugs)
Comment 37•8 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•8 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•8 years 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•8 years 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+
Comment 42•8 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/604725c4b3e0 fix svg viewbox; r=Gijs
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Updated•8 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 43•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/604725c4b3e0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•7 years ago
|
Flags: qe-verify+
Comment 45•7 years 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•