1232017 - URL bar UI incorrectly renders after opening New Private Tab from 3DT [regression]

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Robin Andersen [:tecgirl]]

Attachment: 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

Comment 1

[Emily Toop (:fluffyemily)]

Attachment: Pull request

Comment 2

[Brian Nicholson (:bnicholson)]

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.

Comment 3

[Stephan Leroux [: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 :(

Comment 4

[Emily Toop (:fluffyemily)]

: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.

Comment 5

[Stephan Leroux [:sleroux]]

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.

Comment 6

[Emily Toop (:fluffyemily)]

Yeah, that's the way I imagine it. I'll start working on it....

Comment 7

[Emily Toop (:fluffyemily)]

Attachment: 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.

Comment 8

[Brian Nicholson (:bnicholson)]

(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...

Comment 9

[Stephan Leroux [:sleroux]]

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.

Comment 10

[Brian Nicholson (:bnicholson)]

(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 11

[Brian Nicholson (:bnicholson)]

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.

Comment 12

[Emily Toop (:fluffyemily)]

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