Investigate replacing FloatingHintEditText w/ the design library's TextInputLayout

RESOLVED FIXED in Firefox 47

Status

()

Firefox for Android
General
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcomella, Assigned: ahunt)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 47
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

Probably less performant due to the additional layout but it'd be good to have less code to maintain.

Investigate to make sure this is worthwhile! Can it save us APK size?

TextInputLayout: https://developer.android.com/reference/android/support/design/widget/TextInputLayout.html
(Assignee)

Comment 1

2 years ago
This would be useful for the new full-page edit bookmark dialog - I'm going to have a go at doing this.

https://bugzilla.mozilla.org/show_bug.cgi?id=1232439#c12
Assignee: nobody → ahunt
Priority: -- → P1
(Assignee)

Updated

2 years ago
Blocks: 1232439
(Assignee)

Comment 2

2 years ago
Created attachment 8707015 [details]
MozReview Request: Bug 1227325 - Part 5: add design support library to b2gdroid r?nalexander

Review commit: https://reviewboard.mozilla.org/r/30557/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30557/
Attachment #8707015 - Flags: review?(michael.l.comella)
(Assignee)

Comment 3

2 years ago
Created attachment 8707016 [details]
MozReview Request: Bug 1227325 - Part 2: Upgrade PromptInput to use TextInputLayout r=mcomella

Review commit: https://reviewboard.mozilla.org/r/30559/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30559/
Attachment #8707016 - Flags: review?(michael.l.comella)
(Assignee)

Comment 4

2 years ago
Created attachment 8707017 [details]
MozReview Request: Bug 1227325 - Part 3: Upgrade GeckoPreferences to use TextInputLayout r=mcomella

Review commit: https://reviewboard.mozilla.org/r/30561/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30561/
Attachment #8707017 - Flags: review?(michael.l.comella)
(Assignee)

Comment 5

2 years ago
Created attachment 8707018 [details]
MozReview Request: Bug 1227325 - Part 4: Remove (now unused) FloatingHintEditText r=mcomella

Review commit: https://reviewboard.mozilla.org/r/30563/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30563/
Attachment #8707018 - Flags: review?(michael.l.comella)
(Assignee)

Comment 6

2 years ago
(In reply to Michael Comella (:mcomella) from comment #0)
> Investigate to make sure this is worthwhile! Can it save us APK size?

I'm not sure I'm measuring correctly, but my (debug-, gradle built) apk goes from 41607703 to  41606138 bytes after applying all the patches - slight win, but not really significant?

I think it's definitely worth going down this route since (A) we don't have to maintain FloatingHintEditText, (B) we get to use floating error messages as can be seen in [1] - which we'll use in Bug 1232439 and (C) we get consistent behaviour with the rest of the system / apps.

[1] Scroll down to the second image at https://www.google.com/design/spec/components/text-fields.html#text-fields-single-line-text-field
(Assignee)

Comment 7

2 years ago
Sorry, the current patches don't apply cleanly to fx-team (Part 4 has some conflicts), so I'll be repushing to review in a few minutes if everything goes well.
(Assignee)

Comment 8

2 years ago
Another nice benefit: the floating hint is orange (as opposed to blue) after the conversion, which is what :antlam is using in his designs.
(Assignee)

Comment 9

2 years ago
Comment on attachment 8707015 [details]
MozReview Request: Bug 1227325 - Part 5: add design support library to b2gdroid r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30557/diff/1-2/
(Assignee)

Comment 10

2 years ago
Comment on attachment 8707016 [details]
MozReview Request: Bug 1227325 - Part 2: Upgrade PromptInput to use TextInputLayout r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30559/diff/1-2/
(Assignee)

Comment 11

2 years ago
Comment on attachment 8707017 [details]
MozReview Request: Bug 1227325 - Part 3: Upgrade GeckoPreferences to use TextInputLayout r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30561/diff/1-2/
(Assignee)

Comment 12

2 years ago
Comment on attachment 8707018 [details]
MozReview Request: Bug 1227325 - Part 4: Remove (now unused) FloatingHintEditText r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30563/diff/1-2/
Comment on attachment 8707015 [details]
MozReview Request: Bug 1227325 - Part 5: add design support library to b2gdroid r?nalexander

https://reviewboard.mozilla.org/r/30557/#review27791
Attachment #8707015 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8707016 [details]
MozReview Request: Bug 1227325 - Part 2: Upgrade PromptInput to use TextInputLayout r=mcomella

https://reviewboard.mozilla.org/r/30559/#review27793

::: mobile/android/base/java/org/mozilla/gecko/prompts/PromptInput.java:12
(Diff revision 2)
> +import android.support.design.widget.TextInputLayout;

nit: this should be in the block below.

::: mobile/android/base/java/org/mozilla/gecko/prompts/PromptInput.java:91
(Diff revision 2)
> +            inputLayout.setHintAnimationEnabled(true);

Did you test this on GB? I wonder if the reason this is off by default is because it has poor perf on older devices.

Or if it isn't off by default, don't call these methods! :P
Attachment #8707016 - Flags: review?(michael.l.comella) → review+
Attachment #8707017 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8707017 [details]
MozReview Request: Bug 1227325 - Part 3: Upgrade GeckoPreferences to use TextInputLayout r=mcomella

https://reviewboard.mozilla.org/r/30561/#review27797

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:9
(Diff revision 2)
> +import android.support.design.widget.TextInputLayout;

nit: Move this (and the import above) to the other android section.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java:1237
(Diff revision 2)
> +        TextInputLayout layout = new TextInputLayout(this);

nit: `final`
Comment on attachment 8707018 [details]
MozReview Request: Bug 1227325 - Part 4: Remove (now unused) FloatingHintEditText r=mcomella

https://reviewboard.mozilla.org/r/30563/#review27799

Clean up is great!
Attachment #8707018 - Flags: review?(michael.l.comella) → review+
(Assignee)

Comment 17

2 years ago
https://reviewboard.mozilla.org/r/30559/#review27793

> Did you test this on GB? I wonder if the reason this is off by default is because it has poor perf on older devices.
> 
> Or if it isn't off by default, don't call these methods! :P

It seems to be enabled by default - so no need for me to enable it!
(Assignee)

Comment 18

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/a26c5c9ab77dc3c9eb843b19e672c11ed305aeea
Bug 1227325 - Part 1: Upgrade EditBookmarkDialog to use TextInputLayout r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/d868de0c4779f073934b728ca8ddf7f2d661ac43
Bug 1227325 - Part 2: Upgrade PromptInput to use TextInputLayout r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/d50bbdbc596b327ca18c8edabd4f59a2947ec7ea
Bug 1227325 - Part 3: Upgrade GeckoPreferences to use TextInputLayout r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/5a6812d11eeca1e13d73b063f96f18a49796c512
Bug 1227325 - Part 4: Remove (now unused) FloatingHintEditText r=mcomella
Backed out part 4 in 49f7dd0ab773 - unfortunately, mobile/browser/ didn't use sufficient protection, and it contracted a parasite called "B2GDroid" which copy-pasted something you didn't remove for it, so you broke it, https://treeherder.mozilla.org/logviewer.html#?job_id=6781004&repo=fx-team
Keywords: leave-open
Sadly, the parasite has its hooks in deeper than I feared, so just that one wasn't enough. Backed out 1/2/3 in https://hg.mozilla.org/integration/fx-team/rev/49f7dd0ab773
Keywords: leave-open
(Assignee)

Comment 22

2 years ago
(In reply to Phil Ringnalda (:philor) from comment #19)
> Backed out part 4 in 49f7dd0ab773 - unfortunately, mobile/browser/ didn't
> use sufficient protection, and it contracted a parasite called "B2GDroid"
> which copy-pasted something you didn't remove for it, so you broke it,
> https://treeherder.mozilla.org/logviewer.html#?job_id=6781004&repo=fx-team

It turns out I forgot that there was an (unnecessary) hintAnimationEnabled remaining in Part 1 (which I removed from the other parts) - I'm going to remove that there too which should hopefully fix things.

hintAnimationEnabled defaults to true (hence there's no need for us to set it to true) - and as far as I can tell it wasn't added until v23 of the Android Design Support Library [1] - I suspect b2gdroid is being built against an older version of that library, hence the error.

[1] https://code.google.com/p/android/issues/detail?id=179776#c10
(Assignee)

Comment 23

2 years ago
Comment on attachment 8707015 [details]
MozReview Request: Bug 1227325 - Part 5: add design support library to b2gdroid r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30557/diff/2-3/
(Assignee)

Comment 24

2 years ago
Comment on attachment 8707016 [details]
MozReview Request: Bug 1227325 - Part 2: Upgrade PromptInput to use TextInputLayout r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30559/diff/2-3/
(Assignee)

Comment 25

2 years ago
Comment on attachment 8707017 [details]
MozReview Request: Bug 1227325 - Part 3: Upgrade GeckoPreferences to use TextInputLayout r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30561/diff/2-3/
(Assignee)

Comment 26

2 years ago
Comment on attachment 8707018 [details]
MozReview Request: Bug 1227325 - Part 4: Remove (now unused) FloatingHintEditText r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30563/diff/2-3/
(Assignee)

Comment 27

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/2c299a7561531829cbcb83d1cf8a3c885c16bc76
Bug 1227325 - Part 1: Upgrade EditBookmarkDialog to use TextInputLayout r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/46769f82cbca699d5ed1a2b178680480fac30b71
Bug 1227325 - Part 2: Upgrade PromptInput to use TextInputLayout r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/b17049f75b6b04cad1013bfec1217816748cd43b
Bug 1227325 - Part 3: Upgrade GeckoPreferences to use TextInputLayout r=mcomella

https://hg.mozilla.org/integration/fx-team/rev/0fa26d9b2b8f947356bd746cf5dc4c42785a06a2
Bug 1227325 - Part 4: Remove (now unused) FloatingHintEditText r=mcomella
(Assignee)

Comment 28

2 years ago
(In reply to Andrzej Hunt :ahunt from comment #27)
> https://hg.mozilla.org/integration/fx-team/rev/
> 2c299a7561531829cbcb83d1cf8a3c885c16bc76
> Bug 1227325 - Part 1: Upgrade EditBookmarkDialog to use TextInputLayout
> r=mcomella
> 
> https://hg.mozilla.org/integration/fx-team/rev/
> 46769f82cbca699d5ed1a2b178680480fac30b71
> Bug 1227325 - Part 2: Upgrade PromptInput to use TextInputLayout r=mcomella
> 
> https://hg.mozilla.org/integration/fx-team/rev/
> b17049f75b6b04cad1013bfec1217816748cd43b
> Bug 1227325 - Part 3: Upgrade GeckoPreferences to use TextInputLayout
> r=mcomella
> 
> https://hg.mozilla.org/integration/fx-team/rev/
> 0fa26d9b2b8f947356bd746cf5dc4c42785a06a2
> Bug 1227325 - Part 4: Remove (now unused) FloatingHintEditText r=mcomella

These commits now seem to build on b2gdroid (after removing the last traces of hintAnimationEnabled), hence I'm pushing them again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0448d60e6db
(Assignee)

Comment 29

2 years ago
Comment on attachment 8707015 [details]
MozReview Request: Bug 1227325 - Part 5: add design support library to b2gdroid r?nalexander

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30557/diff/3-4/
Attachment #8707015 - Attachment description: MozReview Request: Bug 1227325 - Part 1: Upgrade EditBookmarkDialog to use TextInputLayout r=mcomella → MozReview Request: Bug 1227325 - Part 5: add design support library to b2gdroid r?nalexander
Attachment #8707015 - Flags: review?(nalexander)
(Assignee)

Updated

2 years ago
Attachment #8707016 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8707017 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8707018 - Attachment is obsolete: true
(Assignee)

Comment 30

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/cd75145fe6d740ff0f77e867c6c03b0b1867e26a
Bug 1227325 - Part 5: add design support library to b2gdroid r=mcomella
Comment on attachment 8707015 [details]
MozReview Request: Bug 1227325 - Part 5: add design support library to b2gdroid r?nalexander

This has landed.
Attachment #8707015 - Flags: review?(nalexander)

Updated

2 years ago
Depends on: 1250900
You need to log in before you can comment on or make changes to this bug.