[meta] Preferences visual refresh

NEW
Unassigned

Status

()

2 years ago
9 months ago

People

(Reporter: evanxd, Unassigned)

Tracking

(Depends on: 4 bugs, Blocks: 1 bug, {meta})

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-preference])

Attachments

(1 obsolete attachment)

(Reporter)

Description

2 years ago
The draft visual refresh design document: https://mozilla.invisionapp.com/share/PWB9S9I8N#/screens

We will update all UI components on the Preferences page (about:preferences) to Photon style.

In a previous meeting, the designer mentioned we will show explanation dialogs when users hover on UI components.
(Reporter)

Updated

2 years ago
Depends on: 1357308
(Reporter)

Comment 1

2 years ago
The document[1] lists all UI components in Preferences. We will update them in the visual refresh work.

[1]: https://docs.google.com/presentation/d/1DkeuDyroJZROF4k_IOtXJ4GZxisxTl_0MIGKNsOsqEU/edit#slide=id.p
(Reporter)

Updated

2 years ago
Whiteboard: [photon][photon-preference]
(Reporter)

Updated

2 years ago
Depends on: 1357312
(Reporter)

Updated

2 years ago
Depends on: 1357313
(Reporter)

Updated

2 years ago
Depends on: 1357314
(Reporter)

Updated

2 years ago
Depends on: 1357315
(Reporter)

Updated

2 years ago
No longer depends on: 1357308
(Reporter)

Updated

2 years ago
Whiteboard: [photon][photon-preference] → [photon-preference]
(Reporter)

Updated

2 years ago
Depends on: 1357308
(Reporter)

Updated

2 years ago
Depends on: 1358952
Target Milestone: --- → Firefox 57

Updated

2 years ago
Depends on: 1365910

Updated

2 years ago
No longer depends on: 1365910
Keywords: meta
(Reporter)

Comment 2

2 years ago
Base on the visual refresh spec[1], we need to do refresh for the below items.

- Font family, size, color
- Background color
- Margin size
- [Web components](https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/232303757) (checkbox, radio button, input field, button, dropdown menu)
- Update and add icons/images
  - menu
  - firefox account
  - [Bug 1357308](http://bugzil.la/1357308): [meta] Add content and images on sub-pages in Preferences 
- Subdialogs (need detail info)
- Search function
  - Remove search result icon

[1]: https://mozilla.invisionapp.com/share/X8BGCX9PD#/screens/240458122
(Reporter)

Updated

2 years ago
Depends on: 1377021
No longer depends on: 1357308
No longer depends on: 1357312
No longer depends on: 1357313
No longer depends on: 1357314
No longer depends on: 1357315
(Reporter)

Updated

2 years ago
No longer depends on: 1360532
(Reporter)

Updated

2 years ago
Depends on: 1377330
Jared / Mike,

We're going to implement visual spec in release 56 but it will be enabled by default in 57. In order to introduce a pref on/off mechanism for the purpose, I'd propose a dynamic CSS injection approach which comes from [1] and example [2], and then create a separate CSS file like browser/themes/shared/incontentprefs/photon-preferences.css which will be loaded by loadSheetUsingURIString() API.

Otherwise, we can use a similar approach [3] to inject CSS through insertStyleSheet().

Both of them work for us but I've no idea what's difference.

Do you think this proposal make sense?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1373655#c4
[2] http://searchfox.org/mozilla-central/source/toolkit/content/datepicker.xhtml#40-47
[3] http://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/browser/extensions/formautofill/bootstrap.js#19-31
Flags: needinfo?(mconley)
Flags: needinfo?(jaws)
I think that's a fine approach for developing on 56.
Flags: needinfo?(mconley)
Why do we need to inject anything at runtime? I would prefer that we don't do any dynamic injection. We should implement this as close to what we will eventually ship as possible, so as not to get results now that might differ when we switch later to a static xml-stylesheet.

Can we just add the xml-stylesheet declaration to the preferences.xul file and wrap it with %ifdef NIGHTLY_BUILD ? We can also just set the href on the xml-stylesheet if the pref is set to true within init_all if you need a runtime pref for testing.
Flags: needinfo?(jaws)
Build-time solution sounds good to me as well.

IIRC, enabling a feature behind build-time flag needs to require source code and add flag in mozconfig before building Firefox. This will require some development knowledge. If QA is able to do it, I think it would be probably fine. 

In other words, it also means that user has no chance to see new visual refresh by switching a prefs if they download Nightly through the download page.

I believe both of run-time and build-time approaches require a final clean up step to sweep those extra code, and you're right the extra code of run-time approach seems like to be more than build-time one. But I don't think both of them would introduce too much effort, so I'd prefer run-time approach since it seems to fit in our requirement like shipping pure front-end feature.

What do you think?
Flags: needinfo?(jaws)
> Can we just add the xml-stylesheet declaration to the preferences.xul file and wrap it with %ifdef NIGHTLY_BUILD ? We can also > just set the href on the xml-stylesheet if the pref is set to true within init_all if you need a runtime pref for testing.

BTW, I'm not sure your proposal here, can you give me a more concrete example for that? Thank you.
Comment hidden (mozreview-request)
Thinking more about this, adding a separate stylesheet or injecting styles will not work in the long term, and when it comes time to merge them in with the stylesheets that we are already shipping we may introduce bugs based on the order of the rules changing. I have attached a patch that provides a build-time flag and allows you to put CSS in preferences.inc.css which is where most of our styling already lives. This prevents developers from needing to add a new file or worry about changes in the application of the CSS.
Assignee: nobody → jaws
Flags: needinfo?(jaws)
Cool, patch looks good to me!

This bug has marked as [meta] is using for tracking Photon visual refresh. Can you file a separate bug and depend on this for addressing build-time flag solution?

Thanks
Flags: needinfo?(jaws)

Comment 11

2 years ago
mozreview-review
Comment on attachment 8885831 [details]
Bug 1357306 - Add MOZ_PHOTON_PREFERENCES build-time flag to help with implementing visual refresh of preferences.

https://reviewboard.mozilla.org/r/156610/#review161872
Attachment #8885831 - Flags: review?(rchien)
Jared, bug 1380585 has reported for addressing this issue.
Jared,

Here is some insights about determining the build-time and run-time approaches.

IIRC, build-time solution will turn visual refresh on by default in Nightly which will impact on our QA testing since the test plan sign-off is based on official Nightly (download from website). It would be impossible for QA to verify Fx56 alongs with some visual changes in Fx57, since there might be a few Fx57 visual changes which could be landed during the end of Fx56 cycle. That might force us to postpone our Fx57 works until all Fx56 works are done (Search regressions + Reorg V2 + Reorg V2 regressions).

Landing Fx57 in Cedar branch could be another good strategic for us but unfortunately QA sign-off will be based on official Nightly, so it seems to be impossible to go for this solution as well.

Above constrain make us hard to adopt build-time solution IMO. We still prefer to adopt run-time solution since it looks like more flexible.

> Thinking more about this, adding a separate stylesheet or injecting styles will not work in the long term, and when it comes time to merge them in with the stylesheets that we are already shipping we may introduce bugs based on the order of the rules changing.

I'm unsure about the CSS merging issue relating to order of the rules changing. Can you give us more details about what's your concerned? Or is that issue could be a risk for us?

Bug 1380585 has filed so we can continue the discussion on there, and I will also upload my run-time solution WIP once the patch is ready.

Thanks

Comment 14

2 years ago
mozreview-review
Comment on attachment 8885831 [details]
Bug 1357306 - Add MOZ_PHOTON_PREFERENCES build-time flag to help with implementing visual refresh of preferences.

https://reviewboard.mozilla.org/r/156610/#review162224
Attachment #8885831 - Flags: review?(cmanchester) → review+
(Reporter)

Updated

2 years ago
Depends on: 1382135
(Reporter)

Updated

2 years ago
No longer depends on: 1377163
(Reporter)

Updated

2 years ago
Depends on: 1382514
(Reporter)

Updated

2 years ago
Depends on: 1382515
(Reporter)

Updated

2 years ago
Depends on: 1386531
(Reporter)

Updated

2 years ago
Depends on: 1386536
(Reporter)

Updated

2 years ago
Depends on: 1388984
(Reporter)

Updated

2 years ago
Depends on: 1388997
(Reporter)

Updated

2 years ago
Depends on: 1389002

Updated

2 years ago
Depends on: 1013701

Updated

2 years ago
Depends on: 1149093
(Reporter)

Updated

2 years ago
Depends on: 1392532

Updated

2 years ago
Depends on: 1392534

Updated

2 years ago
Depends on: 1392544

Updated

2 years ago
Depends on: 1392584

Updated

2 years ago
Depends on: 1392588
Depends on: 1392601
No longer depends on: 1358952

Updated

2 years ago
Depends on: 1393070

Updated

2 years ago
Depends on: 1393354

Updated

2 years ago
Depends on: 1393370

Updated

2 years ago
Depends on: 1393373

Updated

2 years ago
Depends on: 1393409

Updated

2 years ago
Depends on: 1393413

Updated

2 years ago
Depends on: 1393415

Updated

2 years ago
Depends on: 1393446

Updated

2 years ago
Depends on: 1393417

Updated

2 years ago
Depends on: 1394426

Updated

2 years ago
Depends on: 1396599

Updated

2 years ago
Depends on: 1399075

Updated

2 years ago
Depends on: 1399118

Updated

2 years ago
Depends on: 1399128

Updated

2 years ago
Depends on: 1399150
No longer depends on: 1399150
No longer depends on: 1399118

Updated

2 years ago
Depends on: 1399363

Updated

a year ago
Depends on: 1399786

Updated

a year ago
Depends on: 1400177
You need to log in before you can comment on or make changes to this bug.