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

RESOLVED FIXED

Status

()

Firefox for iOS
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tecgirl, Assigned: fluffyemily)

Tracking

unspecified
Other
iOS
Dependency tree / graph

Firefox Tracking Flags

(fxios2.0+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

176.02 KB, image/png
Details
48 bytes, text/x-github-pull-request
bnicholson
: review+
Details | Review | Splinter Review
(Reporter)

Description

3 years ago
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

Updated

3 years ago
Blocks: 1226571
tracking-fxios: --- → ?
Assignee: nobody → etoop
Status: NEW → ASSIGNED
tracking-fxios: ? → 2.0+
(Assignee)

Comment 1

3 years ago
Created attachment 8699441 [details] [review]
Pull request
Attachment #8699441 - Flags: ui-review?(randersen)
Attachment #8699441 - Flags: review?(sarentz)
Attachment #8699441 - Flags: review?(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.
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+
(Reporter)

Updated

3 years ago
Attachment #8699441 - Flags: ui-review?(randersen) → ui-review+
(Assignee)

Comment 4

3 years ago
: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.
(Assignee)

Updated

3 years ago
Flags: needinfo?(sleroux)
Flags: needinfo?(bnicholson)
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.
Flags: needinfo?(sleroux)
(Assignee)

Comment 6

3 years ago
Yeah, that's the way I imagine it. I'll start working on it....
(Assignee)

Comment 7

3 years ago
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.
Attachment #8699441 - Attachment is obsolete: true
Attachment #8699441 - Flags: review?(sarentz)
Attachment #8700038 - Flags: review?(sleroux)
Attachment #8700038 - Flags: review?(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...
Flags: needinfo?(bnicholson)
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+
(Assignee)

Comment 12

3 years ago
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
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Depends on: 1234266
Depends on: 1234267
Attachment #8700038 - Flags: review?(sleroux)
You need to log in before you can comment on or make changes to this bug.