Update Photon Preferences according to visual review suggestion

RESOLVED FIXED in Firefox 57

Status

()

P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: rickychien, Assigned: evanxd)

Tracking

(Blocks: 1 bug)

57 Branch
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [photon-preference])

Attachments

(18 attachments)

147.44 KB, image/png
Details
83.01 KB, image/jpeg
Details
181.28 KB, image/png
Details
238.74 KB, image/png
Details
208.07 KB, image/png
Details
413.16 KB, image/jpeg
Details
41.71 KB, image/png
Details
59 bytes, text/x-review-board-request
mconley
: review+
HHuang
: ui-review+
Details
59.52 KB, image/png
Details
43.12 KB, image/png
Details
85.78 KB, image/png
Details
85.05 KB, image/png
Details
74.72 KB, image/png
Details
76.09 KB, image/png
Details
77.37 KB, image/png
Details
349.88 KB, image/png
Details
78.77 KB, image/png
Details
72.17 KB, 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)
Component: Theme → Preferences
Blocks: 1357306
No longer blocks: 1392532
Flags: qe-verify-
QA Contact: hani.yacoub
Whiteboard: [photon-preference][triage] → [photon-preference]
Remove transparency from the menu icons and labels in Normal state.
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
The current Radio Buttons are slightly bigger than Checkboxes, please be sure all of them are set to 20x20 px.
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.
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.
Please refer to the screenshot and help to fix those margins.
Flags: needinfo?(hhuang)
(Assignee)

Updated

2 years ago
Assignee: nobody → evan
Priority: P2 → P1
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
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)
"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

2 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.
(Assignee)

Comment 12

2 years ago
needinfo Helen to confirm Comment 11.
Flags: needinfo?(hhuang)
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)

Updated

2 years ago
Blocks: 1399708
(Assignee)

Comment 14

2 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

2 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."
Duplicate of this bug: 1399786
Above are the screenshots for clarifying the issues on Comment 18, let me know if any questions.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1391119
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

2 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)
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 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

2 years ago
Attachment #8906968 - Flags: review?(mconley)
(Assignee)

Comment 41

2 years ago
Hi Mike,

Could you help review the patch?

Helen already have reviewed it and approved it.

Thank you.

Comment 42

2 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+
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!
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
Before search. (correct status)
Comment hidden (mozreview-request)
(Assignee)

Comment 47

2 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

2 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 49

2 years ago
Thank you for the review, Helen and Mike.
(Assignee)

Comment 50

2 years ago
The try and about:xxx pages are good. Let's land the patch.
Keywords: checkin-needed

Comment 51

2 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

2 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
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)
Flags: needinfo?(rchien) → needinfo?(evan)
Why is this patch touching textbox.css?
Blocks: 1401439
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.
Do you think we can fix this issue here? If so, we bug 1401439 can be marked as dup.
Duplicate of this bug: 1401439
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 61

2 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

2 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

2 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)
Duplicate of this bug: 1399708
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 68

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8918870e7a20
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.