Closed Bug 779861 Opened 8 years ago Closed 7 years ago

Fix styling of Font Inflation box

Categories

(Firefox for Android :: Theme and Visual Design, defect)

17 Branch
ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 17
Tracking Status
firefox17 --- verified
firefox18 --- verified

People

(Reporter: ibarlow, Assigned: mcomella)

References

(Depends on 1 open bug)

Details

Attachments

(6 files, 11 obsolete files)

51.14 KB, image/png
Details
48.58 KB, image/png
Details
128.84 KB, image/png
Details
205.95 KB, image/jpeg
Details
114.26 KB, image/png
ibarlow
: feedback+
Details
21.28 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
Attached image screenshot
Our current implementation in Nightly needs a few adjustments to make it look more polished like the original mockup http://www.flickr.com/photos/patrykdesign/6427428933/in/photostream

* Create a border around the sample box
* Remove "Text size" header
* Add a few dp of padding to the sample text
* Remove button backgrounds for font resizing controls

A screenshot of the current implementation is attached.
* Fix background and text colour? (Nightly 08/02)
(In reply to Ian Barlow (:ibarlow) from comment #0)
> * Remove "Text size" header

While I probably should have asked rather than assuming, I chose to include the "text size" header because every other preference that spawns a dialog has a header. Are you sure you want me to remove it?
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
OS: Mac OS X → Android
Hardware: x86 → ARM
Version: unspecified → Firefox 17
I am sure
Attached patch Patch (obsolete) — Splinter Review
Patch without button changes.
Attached image Dialog after patch (obsolete) —
After some testing, it seems making the "A" buttons' backgrounds transparent for one state is not possible if we wish to keep the other device default states. This is because in order to override the button style, we must override the style in ALL states and the Android device resources are private so we cannot recreate the remaining states.

The current options I can see are:
1) Use the default buttons
2) Override the button styles entirely (note that all devices will appear the same)
3) Remove the buttons and replace them with a new UI (a slider, perhaps?)

My personal recommendation is to use #1 and eventually shift into #2 application wide. I hear we were looking to override the default Android UI styles and replace them with Firefox-esque ones so we can just wait until that point and replace the buttons then.

We can talk on IRC about this if you have questions.
Attachment #648556 - Flags: feedback?(ibarlow)
(In reply to Michael Comella (:mcomella) from comment #5)
> My personal recommendation is to use #1 and eventually shift into #2
> application wide. I hear we were looking to override the default Android UI
> styles and replace them with Firefox-esque ones so we can just wait until
> that point and replace the buttons then.
> 

That seems reasonable to me.

Any chance we can add an scroll "fade" to the top and bottom of the sample text box, so that text isn't cut off abruptly when you scroll? Either that or just let the text show all the way to the top and bottom edges.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #648548 - Attachment is obsolete: true
Attached image Dialog after patch v2 (obsolete) —
Attachment #648556 - Attachment is obsolete: true
Attachment #648556 - Flags: feedback?(ibarlow)
Due to something funky (probably the way I find the dialog height), I was only able to get text fading working on the top of the textview. As such, I removed the padding from the bottom so that text at the bottom of the textview touches the edge and while the text at the top will fade out. See both screenshots for an example.
Attachment #648891 - Flags: feedback?(ibarlow)
Hm. Seems a little weird to do it in one place and not the other. Perhaps lose the fading then.
Attached patch Patch v3 (obsolete) — Splinter Review
Dialog after patch v2 screenshot same as dialog after patch v3.
Attachment #648888 - Attachment is obsolete: true
Attached image Dialog (mid scroll) after patch v3 (obsolete) —
Attachment #650171 - Flags: feedback?(ibarlow)
I removed all padding at the bottom border and kept the padding at the top. Most users probably won't scroll down so they won't see the inconsistency, however, it keeps the initial screen close to your mocks. What do you think?

(See the other screenshots for other scrolling states)
Attachment #648891 - Attachment is obsolete: true
Attachment #648891 - Flags: feedback?(ibarlow)
Attachment #650173 - Flags: feedback?(ibarlow)
Sorry mcomella, I guess I wasn't quite clear with my feedback. We shouldn't see a white bar at the top when you scroll up. See mockup - http://cl.ly/image/140I1e0K1Y2q
Attached image Dialog, medium text, no top padding (obsolete) —
I think I'm actually the one being unclear, sorry. :\

If you remove the padding from the top of the textview, the text will sit uncomfortably close to the top of the view in an unscrolled state (see attached image as well as "Dialog, large text, no top padding"). This deviates from the mock but it's the only reasonable way to get the scroll to touch the top of the screen.

I *think* it's possible to get the design you want by embedding another empty view to act as padding inside the textview, however, it'll require a lot of extra time for a dialog that will be single use (if even) for most users.

Since there's no perfect way to do it without over-engineering it, I think the last patch (v3) is the best approach - padding at the top but no padding at the bottom (since most users won't scroll). If we take this approach, I will add a followup bug so that when someone has more time, they may be able to match the mock more closely.
Attachment #650206 - Flags: feedback?(ibarlow)
(In reply to Michael Comella (:mcomella) from comment #16) 

> Since there's no perfect way to do it without over-engineering it, I think
> the last patch (v3) is the best approach - padding at the top but no padding
> at the bottom (since most users won't scroll). If we take this approach, I
> will add a followup bug so that when someone has more time, they may be able
> to match the mock more closely.

Ok, that's fine.
Attachment #650206 - Flags: feedback?(ibarlow) → feedback-
Attachment #650173 - Flags: feedback?(ibarlow) → feedback+
Attachment #650171 - Flags: feedback?(ibarlow) → feedback+
Attachment #650203 - Attachment is obsolete: true
Attachment #650203 - Attachment is patch: false
Attachment #650206 - Attachment is obsolete: true
Attachment #650170 - Attachment is patch: true
Attachment #650170 - Flags: review?(bnicholson)
What if you just put the TextView inside of a ScrollView, set all margins to 0, and then put whatever padding you needed on the TextView? Since it's inside of a ScrollView, the padding should scroll with it. You could probably just set the background color on the ScrollView rather than the TextView, and leave the TextView background transparent.

I think we should figure out this issue with the background color not working properly in XML;  I created a mini Android app with a TextView, and setting the background property in XML worked fine there.
(In reply to Brian Nicholson (:bnicholson) from comment #18)
> What if you just put the TextView inside of a ScrollView, set all margins to
> 0, and then put whatever padding you needed on the TextView? Since it's
> inside of a ScrollView, the padding should scroll with it. You could
> probably just set the background color on the ScrollView rather than the
> TextView, and leave the TextView background transparent.

Yeah, this looks like it could work. I'm afraid of unforeseen complications with the way we set the height of the dialog but I think it can be done.

> I think we should figure out this issue with the background color not
> working properly in XML;  I created a mini Android app with a TextView, and
> setting the background property in XML worked fine there.

It was working initially with XML but stopped working in Nightly around 8/2 (Aaron's post is the first time I saw this). As this code did not change, this suggests it was a change in someone else's code. We could probably do a little regression hunting to find the root of this.
Duplicate of this bug: 782550
Attached patch Patch v4 (obsolete) — Splinter Review
Under Sriram's recommendation to try to move the dialog all to XML, I did that. I think it's much cleaner. XLarge have a preset size and everything else fills the screen. I need to test on a Gingerbread tablet before we push this though – fill_parent on width may cause problems.

The XLarge and all others using different outer layout types because I can't get one to work in both. Ping me if you want to know more.

There is still an issue with the smallest font size on tablets where the textview shrinks slightly (maybe, 1px?) causing the dialog to shrink by the same amount. Have any idea why? It's not related to reinflation – I commented that out.

White background is still broken in XML.
Attachment #650170 - Attachment is obsolete: true
Attachment #650170 - Flags: review?(bnicholson)
Attachment #651938 - Flags: review?(bnicholson)
Attachment #648889 - Attachment is obsolete: true
Attachment #650171 - Attachment is obsolete: true
Attachment #650173 - Attachment is obsolete: true
I rewrote the way the dialog is created which let me move back to your original design. Let me know what you think. (Feel free to talk to me in person if you want to see it moving).
Attachment #651943 - Flags: feedback?(ibarlow)
Comment on attachment 651938 [details] [diff] [review]
Patch v4

+        // Trying to set the color in xml turned the background transparent so we set it in code.
+        mScrollingContainer.setBackgroundColor(Color.WHITE);

File a bug about the XML background color problem and include the bug # in the comment.
Attachment #651938 - Flags: review?(bnicholson) → review+
Attached patch Patch v5Splinter Review
Moved r+. Added comment as Brian asked for and removed unused variable. Bug filed for XML issue is bug 783597.
Attachment #651938 - Attachment is obsolete: true
Attachment #653541 - Flags: review+
Got ibarlow's approval on IRC, adding checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/26d631be3337
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
On mozilla-inbound (08/21), on my Galaxy Nexus the background of the preview is still black (black background on black text).
The mozilla-central build that has this patch has the correct white background. I suspect the inbound build was an incremental build instead of a clobber build, and didn't pick up the resource file changes properly.
This issue is verified fixed on the latest Nightly. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-08-28)
Device: Galaxy Note
OS: Android 4.0.4
Status: RESOLVED → VERIFIED
Comment on attachment 651943 [details]
Dialog (phone, scrolled) after patch v4

Clearing ooooold flags
Attachment #651943 - Flags: feedback?(ibarlow) → feedback+
You need to log in before you can comment on or make changes to this bug.