Open Bug 1357306 Opened 8 years ago Updated 2 years ago

[meta] Preferences visual refresh

Categories

(Firefox :: Settings UI, enhancement)

enhancement

Tracking

()

Firefox 57

People

(Reporter: evanxd, Unassigned)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: meta, Whiteboard: [photon-preference])

Attachments

(1 obsolete file)

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.
Depends on: 1357308
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
Whiteboard: [photon][photon-preference]
Depends on: 1357312
Depends on: 1357313
Depends on: 1357314
Depends on: 1357315
No longer depends on: 1357308
Whiteboard: [photon][photon-preference] → [photon-preference]
Depends on: 1357308
Depends on: 1358952
Target Milestone: --- → Firefox 57
Depends on: 1365910
No longer depends on: 1365910
Keywords: meta
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
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
No longer depends on: 1360532
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.
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 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 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+
Moving discussion to bug 1380585.
Flags: needinfo?(jaws)
Assignee: jaws → nobody
Attachment #8885831 - Attachment is obsolete: true
Depends on: 1382135
No longer depends on: 1377163
Depends on: 1382514
Depends on: 1382515
Depends on: 1386531
Depends on: 1386536
Depends on: 1388348
Depends on: 1388984
Depends on: 1388997
Depends on: 1389002
Depends on: 1013701
Depends on: 1149093
Depends on: 1392532
Depends on: 1392534
Depends on: 1392544
Depends on: 1392584
Depends on: 1392588
Depends on: 1392601
No longer depends on: 1358952
Depends on: 1393070
Depends on: 1393354
Depends on: 1393370
Depends on: 1393373
Depends on: 1393409
Depends on: 1393413
Depends on: 1393415
Depends on: 1393446
Depends on: 1393417
Depends on: 1394426
Depends on: 1396599
Depends on: 1399075
Depends on: 1399118
Depends on: 1399128
Depends on: 1399150
No longer depends on: 1399150
No longer depends on: 1399118
Depends on: 1399363
Depends on: 1399786
Depends on: 1400177
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: