Created attachment 8697649 [details] IMG_6758.PNG Steps to reproduce: * Engage 3DT from homescreen icon, choose New Private Tab * Tap 'Cancel' * Switch to normal tabs on the Tabs Tray, open a tab * URL bar renders white with a black text input background and maintains the purple private tab icon
Assignee: nobody → etoop
Status: NEW → ASSIGNED
tracking-fxios: ? → 2.0+
Created attachment 8699441 [details] [review] Pull request
Comment on attachment 8699441 [details] [review] Pull request Curious, what does 3DT do differently that causes this regression? Wish I had a 3DT device to play around with. Redirecting review to Steph since I think he wrote all this UIAppearance code and is more familiar with how it should work.
Attachment #8699441 - Flags: review?(bnicholson) → review?(sleroux)
Comment on attachment 8699441 [details] [review] Pull request Code looks good to me. Using UIAppearance seems like a mistake for the theming stuff. Having to remove/re-add the URL and toolbars is dirty :(
Attachment #8699441 - Flags: review?(sleroux) → review+
Attachment #8699441 - Flags: ui-review?(randersen) → ui-review+
:sleroux, :bnicholson Would we actually be better off looking at this properly and removing the use of UIAppearance for private tabs styling. This solution was pretty hacky, but the other solution was also pretty hacky as it meant overriding UIAppearance controlled component values, which then ignored subsequent UIAppearance changes. I could remove the UIAppearance setting stuff and just use the forceApplyTheme code to restyle everything on privacy mode change. Or the simplest solution for now, which was my original solution before I changed my mind, would be to just forceApplyTheme every time we change the theme. Which is also hacky.
I filed a bug for doing something other than UIAppearance here: https://bugzilla.mozilla.org/show_bug.cgi?id=1227550 Instead of using UIAppearance would we assign to the properties directly? If that's the case then it doesn't seem like a heavy lift to implement that as a fix for this bug as well.
Yeah, that's the way I imagine it. I'll start working on it....
Created attachment 8700038 [details] [review] Pull request So, I've removed UIAppearance and am setting values explicitly according to Theme. Would be interested in comments and ideas about how I can make the themeing better. I played around with a bunch of ideas around enums with values and describing the entire theme and getting each component to just pick it's values, but decided on having themeing handled wholly inside each component. Am not wedded to this solution if we think there is a better way to do it.
(In reply to Emily Toop (:fluffyemily) from comment #7) > Would be interested in comments and ideas about how I can make the themeing > better. If we're overhauling our theme design, maybe this would be a good time to try out NUI (https://github.com/tombenner/nui)? It's been on my todo list, and it has a promising set of features: it separates styles from the view implementations (like CSS), can change styles post-view-creation, etc...
I've used NUI in the past and had issues with perf when using the stylesheets and autolayout. Maybe things have changed since then? This was probably a year or two ago.
(In reply to Stephan Leroux [:sleroux] from comment #9) > I've used NUI in the past and had issues with perf when using the > stylesheets and autolayout. Maybe things have changed since then? This was > probably a year or two ago. Ah, OK. Too bad, it sounds really promising...but good to know someone has tried it already :)
Comment on attachment 8700038 [details] [review] Pull request Kinda sucks that we have to do all this manually. Would be very happy if we could find some styling framework (NUI or similar) that would allow us to separate the view from the styles.
Attachment #8700038 - Flags: review?(bnicholson) → review+
So, I'm gonna go with this for this bug, but leave the UIAppearance removal issue live with comments on it about looking for a css-like styling alternative for the future
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.