1357306 - [meta] Preferences visual refresh

Status: NEW

(Firefox :: Settings UI, enhancement)

Description

[Evan Tseng [:evanxd]]

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.

Comment 1

[Evan Tseng [:evanxd]]

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

Comment 2

[Evan Tseng [:evanxd]]

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

Comment 3

[Ricky Chien [:rickychien] (inactive)]

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

Comment 4

[Mike Conley (:mconley) (:⚙️)]

I think that's a fine approach for developing on 56.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 6

[Ricky Chien [:rickychien] (inactive)]

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?

Comment 7

[Ricky Chien [:rickychien] (inactive)]

> 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 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1357306 - Add MOZ_PHOTON_PREFERENCES build-time flag to help with implementing visual refresh of preferences.

Review commit: https://reviewboard.mozilla.org/r/156610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156610/

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 10

[Ricky Chien [:rickychien] (inactive)]

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

Comment 11

[Ricky Chien [:rickychien] (inactive)]

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

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Jared, bug 1380585 has reported for addressing this issue.

Comment 13

[Ricky Chien [:rickychien] (inactive)]

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

[Chris Manchester (limited bugmail, email directly)]

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

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Moving discussion to bug 1380585.