Closed
Bug 1398050
Opened 7 years ago
Closed 7 years ago
Update Photon Preferences according to visual review suggestion
Categories
(Firefox :: Settings UI, defect, P1)
Tracking
()
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: rickychien, Assigned: evanxd)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-preference])
Attachments
(18 files)
(deleted),
image/png
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
mconley
:
review+
HHuang
:
ui-review+
|
Details |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details |
This bug is going to list all visual issues or regressions after visual review. According Photon visual spec [1], any unmatched or unaddressed visual issues will be listed below. Helen, please review the visual changes kindly based on the latest Nightly including macOS/Windows/Linux, and write down all issues you think they are unmatched to spec. Thanks [1] https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens
Flags: needinfo?(hhuang)
Reporter | ||
Updated•7 years ago
|
Component: Theme → Preferences
Reporter | ||
Updated•7 years ago
|
Updated•7 years ago
|
Flags: qe-verify-
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage] → [photon-preference]
Comment 2•7 years ago
|
||
Missing color box while hovering and pressing Firefox Support, It should be applied the same style as Navigation. Please refer to the visual spec here https://mozilla.invisionapp.com/share/X8BGCX9PD#/244683005_Navigation
Comment 3•7 years ago
|
||
The current Radio Buttons are slightly bigger than Checkboxes, please be sure all of them are set to 20x20 px.
Comment 4•7 years ago
|
||
Please check all the Action Button which with another Action Button on the top/bottom of it, for example, the buttons under Forms & Passwords; the margin between those buttons should be 8px. The Action Buttons which are on the right side of a setting within the content area, the minimum width should be 150px.
Comment 5•7 years ago
|
||
The line-height of Body text should be 22px. Refer to spec here https://mozilla.invisionapp.com/share/X8BGCX9PD#/244683138_Content_-_General If the link text at the beginning of a sentence due to the string is wrapped, remove the space before it. Also, I found some link text is miss-alignments. Please help to fix it. The components, like Checkbox or Action Button, should be aligned with the first line when a single string is wrapped. For example, the setting "Query OCSP responder...." under Certificates.
Comment 6•7 years ago
|
||
Please refer to the screenshot and help to fix those margins.
Flags: needinfo?(hhuang)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → evan
Priority: P2 → P1
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 7•7 years ago
|
||
The tooltip bottom arrow of menulist doesn't align with the one on button. Solution: Set [1] margin: 0 8px; to margin: 2px 8px; can solve easily. [1] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/toolkit/themes/shared/in-content/common.inc.css#197
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•7 years ago
|
||
"Text and Background" groupbox should align with "Link Colors" Here is one line CSS fix. Change `groupbox + groupbox` to `prefpane > groupbox + groupbox` can fix it. [1] http://searchfox.org/mozilla-central/source/browser/themes/shared/incontentprefs/preferences.inc.css#33
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
Discussed the checkbox align issue mentioned at Comment 5 with Helen, we won't fix it at this stage since that is the default behavior of the checkbox XUL component. And after we use new 22px line-height, it looks better than previous one.
Comment 13•7 years ago
|
||
Per discussion, I realize it's not easy to implement for now, so I'm okay with won't fix it at this stage. Hoping it could be fixed in further iterations.
Flags: needinfo?(hhuang)
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #7) > Created attachment 8906966 [details] > tooltip-arrow-bottom.png > > The tooltip bottom arrow of menulist doesn't align with the one on button. > > Solution: Set [1] margin: 0 8px; to margin: 2px 8px; can solve easily. > > [1] > http://searchfox.org/mozilla-central/rev/ > 51eae084534f522a502e4b808484cde8591cb502/toolkit/themes/shared/in-content/ > common.inc.css#197 Let's fix the tooltip issue at Bug 1399708 since this issue already exists in current m-c code base.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 years ago
|
||
Helen and I reviewed Preferences page on Windows, Linux, and macOS, we found out new issues. Helen will make a new comment to clearly describe them with screenshots. The below is our draft note, and I'm working on fixing them. ## Linux - In General Page, the margin between Firefox Start Page and buttons should be 8px - Having 8px before dropdowns on Linux - Add 8px space before link text "What's new" under Firefox Updates - Add 8px space before the button "Change Device Name" under Device Name - The input field and "Change Device Name" button under Device Name should align with each other ## Mac - The height of the input field under Device Name should be 30px ## Windows - The font style of description under "Do not disturb me" should be updated. - In General Page, the margin between Firefox Start Page and buttons should be 8px - Having 8px before dropdowns on Windows - Add 8px space before link text "What's new" under Firefox Updates - Check all the height of buttons, it should be 30px ## All - Firefox Support - Remove transparency - Change button background when it is actived - Under History -> Set to "Use custom settings for history", the margin above of the item "Always use... mode" should be 8px (window, linux) - Remove extra space between "Nightly will remember... you visit." and "You may want... individual cookies."
Comment 20•7 years ago
|
||
Comment 21•7 years ago
|
||
Comment 22•7 years ago
|
||
Comment 23•7 years ago
|
||
Comment 24•7 years ago
|
||
Comment 25•7 years ago
|
||
Comment 26•7 years ago
|
||
Above are the screenshots for clarifying the issues on Comment 18, let me know if any questions.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•7 years ago
|
||
Comment on attachment 8906968 [details] Bug 1398050 - Polish preferences page to match visual spec. Hi Helen, Could you review the UI changes? Thank you.
Attachment #8906968 -
Flags: ui-review?(hhuang)
Comment 38•7 years ago
|
||
I've reviewed the visual of Preferences on Mac and Linux, looks okay to me. Still Waiting for the build of Windows version.
Comment hidden (mozreview-request) |
Comment 40•7 years ago
|
||
Comment on attachment 8906968 [details] Bug 1398050 - Polish preferences page to match visual spec. Just reviewed the Windows version of Preferences, all the platforms look good now. Thank you!
Attachment #8906968 -
Flags: ui-review?(hhuang) → ui-review+
Assignee | ||
Updated•7 years ago
|
Attachment #8906968 -
Flags: review?(mconley)
Assignee | ||
Comment 41•7 years ago
|
||
Hi Mike, Could you help review the patch? Helen already have reviewed it and approved it. Thank you.
Comment 42•7 years ago
|
||
mozreview-review |
Comment on attachment 8906968 [details] Bug 1398050 - Polish preferences page to match visual spec. https://reviewboard.mozilla.org/r/178720/#review186026 Thanks! This code looks fine to me. I didn't visually test the patch, since Helen already ui-r+'d this. I did have one small nit regarding the naming of a class, see below. ::: browser/components/preferences/in-content/main.xul:917 (Diff revision 15) > <groupbox id="performanceGroup" data-category="paneGeneral" hidden="true"> > <caption class="search-header" hidden="true"><label>&performance.label;</label></caption> > > <hbox align="center"> > <checkbox id="useRecommendedPerformanceSettings" > + class="tailWithLearnMore" Nit: I've noticed that surrounding class attributes have their classes named like: term-term2-term3 So perhaps this should be: tail-with-learn-more
Attachment #8906968 -
Flags: review?(mconley) → review+
Reporter | ||
Comment 43•7 years ago
|
||
Evan, We should take more care of every change of in-content.css, and make sure that we don't cause any new in-content regression again from our patch. Please check all about:xxx from about:about before landing this patch. Thanks!
Reporter | ||
Comment 44•7 years ago
|
||
I noticed that the menuitem which has arrow indicator will become taller than the original state (no arrow indicator). For example, "Remember history" is taller. It might be caused by line-height: 30px setting at label [1]. It's definitely not a regression from this patch but do you think this issue can be fixed in your patch as well? [1] http://searchfox.org/mozilla-central/source/browser/themes/shared/incontentprefs/preferences.inc.css#60
Comment hidden (mozreview-request) |
Assignee | ||
Comment 47•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8906968 [details] Bug 1398050 - Polish preferences page to match visual spec. https://reviewboard.mozilla.org/r/178720/#review186026 > Nit: I've noticed that surrounding class attributes have their classes named like: > > term-term2-term3 > > So perhaps this should be: > > tail-with-learn-more Sure, let's update it.
Assignee | ||
Comment 48•7 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #43) > Evan, > > We should take more care of every change of in-content.css, and make sure > that we don't cause any new in-content regression again from our patch. > Please check all about:xxx from about:about before landing this patch. > Thanks! Sure, we should do it. Let's do it. (In reply to Ricky Chien [:rickychien] from comment #44) > Created attachment 8909572 [details] > menuitem-too-tall.png > > I noticed that the menuitem which has arrow indicator will become taller > than the original state (no arrow indicator). For example, "Remember > history" is taller. It might be caused by line-height: 30px setting at label > [1]. > > It's definitely not a regression from this patch but do you think this issue > can be fixed in your patch as well? > > [1] > http://searchfox.org/mozilla-central/source/browser/themes/shared/ > incontentprefs/preferences.inc.css#60 I think this patch could not fix this, so let's file a new bug to address it.
Assignee | ||
Comment 50•7 years ago
|
||
The try and about:xxx pages are good. Let's land the patch.
Keywords: checkin-needed
Comment 51•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 424e8503e87f -d ff2b91e284d2: rebasing 421223:424e8503e87f "Bug 1398050 - Polish preferences page to match visual spec. r=mconley" (tip) merging browser/components/preferences/in-content/main.xul warning: conflicts while merging browser/components/preferences/in-content/main.xul! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment 53•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2f1a9242f33d Polish preferences page to match visual spec. r=mconley
Keywords: checkin-needed
Comment 54•7 years ago
|
||
Backed out for failing reftest reftest/tests/editor/reftests/xul/empty-1.xul and more: https://hg.mozilla.org/integration/autoland/rev/4823939d5e4bf44463881c4644c88ce5d8c6209c Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2f1a9242f33df333d4e7ca123a5c8afb0a712987&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=131948741&repo=autoland > REFTEST TEST-UNEXPECTED-FAIL | file:///Z:/task_1505817067/build/tests/reftest/tests/editor/reftests/xul/empty-1.xul == file:///Z:/task_1505817067/build/tests/reftest/tests/editor/reftests/xul/empty-ref.xul | image comparison, max difference: 146, number of differing pixels: 4899 etc.
Flags: needinfo?(rchien)
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(rchien) → needinfo?(evan)
Reporter | ||
Comment 56•7 years ago
|
||
Evan, I'm sure the issue of https://bugzilla.mozilla.org/show_bug.cgi?id=1398050#c44 can be easily addressed by below one line change http://searchfox.org/mozilla-central/source/browser/themes/shared/incontentprefs/preferences.inc.css#59 Change label { to *:not(menuitem) > label { This only take effect on preferences.inc.css so it won't cause regressions to other in-content pages.
Reporter | ||
Comment 57•7 years ago
|
||
Do you think we can fix this issue here? If so, we bug 1401439 can be marked as dup.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 61•7 years ago
|
||
mozreview-review |
Comment on attachment 8906968 [details] Bug 1398050 - Polish preferences page to match visual spec. https://reviewboard.mozilla.org/r/178720/#review186948 ::: browser/components/preferences/in-content/main.xul:436 (Diff revision 19) > class="extension-controlled-button accessory-button" > label="&disableExtension.label;" /> > </hbox> > <hbox align="center"> > <checkbox id="browserContainersCheckbox" > + class="tail-with-learn-more" nit: this line should be indented properly
Comment hidden (mozreview-request) |
Assignee | ||
Comment 63•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8906968 [details] Bug 1398050 - Polish preferences page to match visual spec. https://reviewboard.mozilla.org/r/178720/#review186948 > nit: this line should be indented properly Sure, let's fix it. Thanks.
Assignee | ||
Comment 64•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #55) > Why is this patch touching textbox.css? Update the margin of textbox to match visual spec. But let's update this in preferences.inc.css. Then I think the reftest will be good.
Flags: needinfo?(evan)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 68•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8918870e7a20 Polish preferences page to match visual spec. r=mconley
Comment 69•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8918870e7a20
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•