962103 - Progress bar visual refinements

Status: RESOLVED FIXED

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

Description

[Ian Barlow (:ibarlow)]

OMG guys we have a progress bar and it's amazing! This bug is to give it a few last visual tweaks to make it look a little nicer. 

Screenshot: http://cl.ly/image/0d2t06353x2L

Changes listed in the screenshot:

* Improve visibility
** Make bar 50% thicker
** Move bar down so that only the top 1/3rd overlaps the title bar
* Make it pretty
** Add light orange to dark orange gradient to bar
** Add shadow “bulb” to right edge of bar, to look just a little more Holo-y

Comment 1

[Ian Barlow (:ibarlow)]

Note: I have some comments on the way the progress bar animates as well. That will be filed in a separate bug.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

I suspect that these changes will require us to move the progressbar out of the browser_toolbar.xml layout. We would add the shadow back to browser_toolbar too.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: screenshot

Ian, what do you think of this screenshot?

Build here: https://dl.dropboxusercontent.com/u/35559547/fennec/progressbar-new.apk

I used the images directly from the stock browser (linked below), only I tinted them orange. They actually use a lighter color near the bulb since it creates a kind of glow effect. I tried adding a gradient effect to make the bulb darker, but they didn't look like great (I'm far from a GIMP/Photoshop pro), so I left that out that.

In case it's easier for you to just create these assets yourself and upload them, here are the Android ones I'm basing this off of:

http://androidxref.com/4.4.2_r1/xref/packages/apps/Browser/res/drawable-hdpi/progress.9.png
http://androidxref.com/4.4.2_r1/xref/packages/apps/Browser/res/drawable-mdpi/progress.9.png
http://androidxref.com/4.4.2_r1/xref/packages/apps/Browser/res/drawable-xhdpi/progress.9.png

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Progress bar visual refinements

We can wait on Ian's feedback to see if the assets should be updated, but there shouldn't be any additional code changes.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 8371913 [details] [diff] [review]
Progress bar visual refinements

Just discovered that this doesn't work well with the dynamic title bar. We'll have to figure out some way to keep them together.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: screenshot with scrolling bug

Screenshot of scrolling the page with this patch applied.

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #5)
> Comment on attachment 8371913 [details] [diff] [review]
> Progress bar visual refinements
> 
> Just discovered that this doesn't work well with the dynamic title bar.
> We'll have to figure out some way to keep them together.

I think we should try a bit harder to keep the progress bar "in the toolbar" (not necessarily as a direct child of the toolbar layout). Moving it out will just cause us all sorts of headaches.

A couple of things to explore:
- Surround current toolbar with a layout that contains the current toolbar contents and and the progress bar.
- Play with margins/paddings/background drawable so that you can create the intended "effect" (progress bar partially overlapping the toolbar bottom edge) without moving it out.

The second option is the ideal one as we wouldn't be adding any extra views/layouts.

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Lucas Rocha (:lucasr) from comment #7)
> I think we should try a bit harder to keep the progress bar "in the toolbar"
> (not necessarily as a direct child of the toolbar layout). Moving it out
> will just cause us all sorts of headaches.
> 
> A couple of things to explore:
> - Surround current toolbar with a layout that contains the current toolbar
> contents and and the progress bar.
> - Play with margins/paddings/background drawable so that you can create the
> intended "effect" (progress bar partially overlapping the toolbar bottom
> edge) without moving it out.
> 
> The second option is the ideal one as we wouldn't be adding any extra
> views/layouts.

Thinking about this more, I'm starting to lean toward an approach that other browsers use: don't hide the toolbar if the page is still loading. IMO, this makes sense from a UX perspective as it keeps the current status in view, and this would solve any dynamic toolbar related issues since it remains in a static position.

If we go with that approach, it won't be critical to keep the progress bar inside of browser toolbar (even though we probably should if we can find a way, which I haven't yet).

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Good point. I guess some input from Ian would be good. As well as a test build?

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: 0001-dynamic-toolbar.patch

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Good point. I guess some input from Ian would be good. As well as a test
> build?

Asked Ian over IRC, but he wasn't happy with the idea; he says he likes being able to scroll the page without worrying about the current page's load state.

I can prevent the dynamic toolbar from resizing using this patch, but this includes some problems of its own. Namely, after the page has loaded and the user scrolls around, the progress bar jumps around -- though I'm sure we tweak dynamic toolbar to fix these issues.

Test build here: https://dl.dropboxusercontent.com/u/35559547/fennec/progressbar-new-dynamic-toolbar.apk

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Make progress bar overlap content

Here's a patch that keeps the progress bar inside of BrowserToolbar. Unfortunately, this has problems too: it shows orange artifacts on the page when scrolling (screenshot: https://dl.dropboxusercontent.com/u/35559547/fennec/progress-scroll-artifacts.png).

At this point, I'm convinced that we can either have an overlapping progress bar, or a progress bar that scrolls with the dynamic toolbar, but not both.

Comment 12

[Brian Nicholson (:bnicholson)]

Ian, given the two options in comment 11, which would you prefer?

Comment 13

[Ian Barlow (:ibarlow)]

Well... neither, if I'm honest. 

But if those are the only two options, I would probably go with the overlapping progress bar that keeps the title bar on screen until a page is loaded. It looks better and a little more Android like.

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Ian Barlow (:ibarlow) from comment #13)
> Well... neither, if I'm honest. 
> 
> But if those are the only two options, I would probably go with the
> overlapping progress bar that keeps the title bar on screen until a page is
> loaded. It looks better and a little more Android like.

Sorry for the bad news -- I'll let you know if I come across a way to fix this.

Just to cover all bases: kats, would you happen to know what's causing the artifacts in the screenshot: https://dl.dropboxusercontent.com/u/35559547/fennec/progress-scroll-artifacts.png ? This is from a build using the "Redesigned progress bar inside BrowserToolbar" patch which makes the progress bar partially overlap the LayerView. This glitch happens when scrolling the dynamic toolbar up.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Not sure unfortunately.

Comment 16

[Ian Barlow (:ibarlow)]

Friendly ping -- we want to do a UX sign-off on the progress bar next Tuesday, and this is one of our last remaining blockers (if not *the* last). Any chance we can land this baby? :)

Comment 17

[Brian Nicholson (:bnicholson)]

(In reply to Ian Barlow (:ibarlow) from comment #16)
> Friendly ping -- we want to do a UX sign-off on the progress bar next
> Tuesday, and this is one of our last remaining blockers (if not *the* last).
> Any chance we can land this baby? :)

Yeah, I'm finally getting around to working on this now...this will likely require some dynamic toolbar work to fix the issues in the build in comment 10.

Comment 18

[Brian Nicholson (:bnicholson)]

Comment on attachment 8372522 [details] [diff] [review]
Part 1: Make progress bar overlap content

This patch from comment 11 should be good to go as-is. Alone, this patch will produce the artifacts mentioned in that comment. But if we lock the dynamic toolbar whenever progress is shown, that shouldn't be an issue.

The alternative to this patch to move the progress bar outside of BrowserToolbar, as in the patch in comment 4. Either patch will work as long as we lock the dynamic toolbar, but I went with this one since it keeps the progress bar contained.

Comment 19

[Brian Nicholson (:bnicholson)]

I've been testing this patch pretty extensively today, and for whatever reason, the graphical glitches from comment 11 no longer seem to be appearing. It's possible something landed that fixed this, but I'm still skeptical and leaning more toward this just being an intermittent issue. In any case, I can't reproduce it for now, so I guess we can move forward with this patch if this is what we want. If we later find regressions -- either the glitches from comment 11 or others -- we can still use the pinned dynamic toolbar behavior I've been working on.

Here's a build with just this patch applied: http://people.mozilla.org/~bnicholson/fennec_builds/progress-bar-1.apk

Ian, what do you think of this build? This should be the behavior you wanted: a progress bar that scrolls with the dynamic toolbar. If the dynamic toolbar is scrolled of the screen, this also shows half the progress bar peeking from the top, which was something we discussed on IRC.

To anyone trying this build: please keep your eyes peeled for any orange artifacts that get left on the screen when scrolling: https://dl.dropboxusercontent.com/u/35559547/fennec/progress-scroll-artifacts.png

Comment 20

[Brian Nicholson (:bnicholson)]

Ian, here's a second build to compare: http://people.mozilla.org/~bnicholson/fennec_builds/progress-bar-2.apk

Like other browsers, this build keeps the dynamic toolbar visible whenever progress is showing.

Comment 21

[Brian Nicholson (:bnicholson)]

Attachment: (WIP) Part 2: Refactor dynamic toolbar code

And here are the patches used to make the build in comment 20, for reference. Not going to flag for review unless Ian prefers comment 20's build over comment 19's build.

Comment 22

[Brian Nicholson (:bnicholson)]

Attachment: Part 3: Pin the dynamic toolbar whenever progress is displayed

Comment 23

[Lucas Rocha (:lucasr)]

Comment on attachment 8372522 [details] [diff] [review]
Part 1: Make progress bar overlap content

Review of attachment 8372522 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/resources/layout/browser_toolbar.xml
@@ +107,5 @@
>  
>      <org.mozilla.gecko.toolbar.ToolbarProgressView android:id="@+id/progress"
>                                                     android:layout_width="fill_parent"
> +                                                   android:layout_height="16dp"
> +                                                   android:layout_marginBottom="-8dp"

Wow, 16dp? I tested the APK and it does look a bit too fat. I'd probably make slightly thinner. It's ibarlow's call though.

::: mobile/android/base/resources/layout/gecko_app.xml
@@ +17,5 @@
>           android:id="@+id/main_layout"
>           android:layout_width="fill_parent"
>           android:layout_height="fill_parent"
> +         android:background="@android:color/transparent"
> +         android:clipChildren="false">

Even in the main layout? Weird.

@@ +78,5 @@
>          <org.mozilla.gecko.widget.GeckoViewFlipper android:id="@id/browser_actionbar"
>                  android:layout_width="fill_parent"
>                  android:layout_height="@dimen/browser_toolbar_height"
>                  android:clickable="true"
> +                android:clipChildren="false"

Add comment explaining why this clipChildren is needed.

Comment 24

[Ian Barlow (:ibarlow)]

(In reply to Brian Nicholson (:bnicholson) from comment #19)
> If we later find regressions -- either
> the glitches from comment 11 or others -- we can still use the pinned
> dynamic toolbar behavior I've been working on.
> 
> Here's a build with just this patch applied:
> http://people.mozilla.org/~bnicholson/fennec_builds/progress-bar-1.apk
> 
> Ian, what do you think of this build? This should be the behavior you
> wanted: a progress bar that scrolls with the dynamic toolbar. If the dynamic
> toolbar is scrolled of the screen, this also shows half the progress bar
> peeking from the top, which was something we discussed on IRC.


I LOVE IT

I wasn't able to break it or find any glitches either, so I would suggest we land it and get more people trying it out. 

\o/

Comment 25

[Ian Barlow (:ibarlow)]

Well. I knew that if I left a comment as enthusiastic as that, I would invariably return with some actual feedback.

Lucas and I were talking, Brian how hard would it be to make the progress bar a little bit thinner? Like 1-2dp thinner?

Comment 26

[Brian Nicholson (:bnicholson)]

After talking to ibarlow and testing different assets, landed with the following changes:
1) Progress bar is quite a bit thinner
2) Use a dark bulb instead of a glow at the tip of the bar

https://hg.mozilla.org/integration/fx-team/rev/1eebddd614eb

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/1eebddd614eb

Comment 28

[Ed Morley [:emorley]]

Backed out for bug 947550 comment 73:
remote:   https://hg.mozilla.org/integration/fx-team/rev/da753add9433

eg:
https://tbpl.mozilla.org/?tree=Fx-Team&jobname=robocop-5&tochange=d4c7d7ed2196&fromchange=dc386405bf17

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Merge of backout.
https://hg.mozilla.org/mozilla-central/rev/da753add9433

Comment 30

[Brian Nicholson (:bnicholson)]

Attachment: Make progress bar overlap content, v2

For reasons unknown, using clipChildren on 2.3 breaks the layout. This disables clipping on GB and earlier. The progress bar is cut off on these devices, but it still looks decent (close to what we currently have in Nightly). Screenshot at http://people.mozilla.org/~bnicholson/screenshots/fennec_progress_gb.png.

Comment 31

[Brian Nicholson (:bnicholson)]

Note that testAboutHomePageNavigation, the reason this was backed out, has been disabled in bug 979038. That means we shouldn't have to worry about in failures this time around.

Also, I don't have any Honeycomb tablets, so I don't know whether Honeycomb needs this fix too. Can someone in QA verify that Honeycomb works as expected? Build here: http://people.mozilla.org/~bnicholson/fennec_builds/progress-bar-6.apk

Comment 32

[Lucas Rocha (:lucasr)]

Comment on attachment 8385150 [details] [diff] [review]
Make progress bar overlap content, v2

Review of attachment 8385150 [details] [diff] [review]:
-----------------------------------------------------------------

Ok.

Comment 33

[Aaron Train [:aaronmt]]

(In reply to Brian Nicholson (:bnicholson) from comment #31)
> Note that testAboutHomePageNavigation, the reason this was backed out, has
> been disabled in bug 979038. That means we shouldn't have to worry about in
> failures this time around.
> 
> Also, I don't have any Honeycomb tablets, so I don't know whether Honeycomb
> needs this fix too. Can someone in QA verify that Honeycomb works as
> expected? Build here:
> http://people.mozilla.org/~bnicholson/fennec_builds/progress-bar-6.apk

Comment 34

[u421692]

(In reply to Brian Nicholson (:bnicholson) from comment #31)
> Note that testAboutHomePageNavigation, the reason this was backed out, has
> been disabled in bug 979038. That means we shouldn't have to worry about in
> failures this time around.
> 
> Also, I don't have any Honeycomb tablets, so I don't know whether Honeycomb
> needs this fix too. Can someone in QA verify that Honeycomb works as
> expected? Build here:
> http://people.mozilla.org/~bnicholson/fennec_builds/progress-bar-6.apk

Verified this on Acer Iconia Tab A500(Android 3.2.1), with the "progress-bar-6.apk" build, and the progress bar looks and feels fine, there is no issue with the build.

Comment 35

[Brian Nicholson (:bnicholson)]

Thanks, Mihai!

Let's hope it sticks this time: https://hg.mozilla.org/integration/fx-team/rev/4b5d9d01c52f

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/4b5d9d01c52f

Comment 37

[Brian Nicholson (:bnicholson)]

Backed out in bug 981676 due to bug 981783 and bug 981676.

Comment 38

[Brian Nicholson (:bnicholson)]

Keeping the progress bar inside the dynamic toolbar has resulted in several regressions, so I think it's time to give up on that approach and go back to the original plan (lock the dynamic toolbar in place during page load).

Comment 39

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Make progress bar overlap content

This takes us back to the original approach of putting the toolbar in gecko_app.xml.

Comment 40

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Make progress bar overlap content, v2

Rebased on top of fixed backout (https://bugzilla.mozilla.org/show_bug.cgi?id=981676#c8).

Comment 41

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Pin the dynamic toolbar whenever progress is displayed

This addresses the issue from https://bugzilla.mozilla.org/attachment.cgi?id=8371916 by preventing the dynamic toolbar from moving during page load.

Comment 42

[Lucas Rocha (:lucasr)]

Comment on attachment 8390809 [details] [diff] [review]
Part 1: Make progress bar overlap content, v2

Review of attachment 8390809 [details] [diff] [review]:
-----------------------------------------------------------------

How are you going to ensure that the progress bar view is not displayed *while* the toolbar is sliding in? Try these steps:

1. Navigate to any page, scroll down so that toolbar slides off screen.
2. Tap on a link.

I haven't seen any code in these two patches that will stop the progress bar from showing before the toolbar is fully visible on screen. Am I missing something?

Comment 43

[Lucas Rocha (:lucasr)]

Comment on attachment 8390823 [details] [diff] [review]
Part 2: Pin the dynamic toolbar whenever progress is displayed

Review of attachment 8390823 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review for now to get some more background information about the expected behaviour.

Comment 44

[Lucas Rocha (:lucasr)]

Wondering: have you considered translating the progressbar whenever the toolbar slides so that they move around together as if the progress bar was still part of the toolbar?

Comment 45

[Brian Nicholson (:bnicholson)]

Attachment: Make progress bar overlap content, v3

(In reply to Lucas Rocha (:lucasr) from comment #44)
> Wondering: have you considered translating the progressbar whenever the
> toolbar slides so that they move around together as if the progress bar was
> still part of the toolbar?

That's a good idea. I got mcomella to test the smearing issue with this build, and he confirmed that it doesn't appear.

Build here: http://people.mozilla.org/~bnicholson/fennec_builds/progress-bar-8.apk

Comment 46

[Lucas Rocha (:lucasr)]

Comment on attachment 8391543 [details] [diff] [review]
Make progress bar overlap content, v3

Review of attachment 8391543 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this is a lot simpler. I'd like to see an updated patch before giving r+. Please test this on Gingerbread devices to make sure the translation bits work fine.

::: mobile/android/base/BrowserApp.java
@@ +883,5 @@
>          ThreadUtils.postToUiThread(new Runnable() {
>              public void run() {
> +                final float translationY = marginTop - toolbarLayout.getHeight();
> +                ViewHelper.setTranslationY(toolbarLayout, translationY);
> +                mProgressView.setTranslationY(translationY);

This will crash in pre-Honeycomb. You should use ViewHelper.setTranslationY(mProgressView, translationY) instead.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ -207,5 @@
>          mMenuIcon = (GeckoImageView) findViewById(R.id.menu_icon);
>          mActionItemBar = (LinearLayout) findViewById(R.id.menu_items);
>          mHasSoftMenuButton = !HardwareUtils.hasMenuButton();
>  
> -        mProgressBar = (ToolbarProgressView) findViewById(R.id.progress);

Just do this:

mProgressBar = (ToolbarProgressView) getRootView().findViewById(R.id.progress);

And avoid the separate the progress bar setter altogether.

Comment 47

[Brian Nicholson (:bnicholson)]

Attachment: Make progress bar overlap content, v4

(In reply to Lucas Rocha (:lucasr) from comment #46)
> This will crash in pre-Honeycomb. You should use
> ViewHelper.setTranslationY(mProgressView, translationY) instead.

Oops, fixed.

> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ -207,5 @@
> >          mMenuIcon = (GeckoImageView) findViewById(R.id.menu_icon);
> >          mActionItemBar = (LinearLayout) findViewById(R.id.menu_items);
> >          mHasSoftMenuButton = !HardwareUtils.hasMenuButton();
> >  
> > -        mProgressBar = (ToolbarProgressView) findViewById(R.id.progress);
> 
> Just do this:
> 
> mProgressBar = (ToolbarProgressView)
> getRootView().findViewById(R.id.progress);
> 
> And avoid the separate the progress bar setter altogether.

I don't think this is a good idea. Having BrowserToolbar reach outside of its layout and referencing an ID it doesn't own unnecessarily increases coupling. BrowserApp will still need to hold a reference to the progress bar because of the translation, so removing the setter doesn't buy us much anyway.

Note that this is essentially the same design we use for HomeBanner, where we also have a setter: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1661.

Comment 48

[Lucas Rocha (:lucasr)]

Comment on attachment 8392314 [details] [diff] [review]
Make progress bar overlap content, v4

Review of attachment 8392314 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Brian Nicholson (:bnicholson) from comment #47)
> > ::: mobile/android/base/toolbar/BrowserToolbar.java
> > @@ -207,5 @@
> > >          mMenuIcon = (GeckoImageView) findViewById(R.id.menu_icon);
> > >          mActionItemBar = (LinearLayout) findViewById(R.id.menu_items);
> > >          mHasSoftMenuButton = !HardwareUtils.hasMenuButton();
> > >  
> > > -        mProgressBar = (ToolbarProgressView) findViewById(R.id.progress);
> > 
> > Just do this:
> > 
> > mProgressBar = (ToolbarProgressView)
> > getRootView().findViewById(R.id.progress);
> > 
> > And avoid the separate the progress bar setter altogether.
> 
> I don't think this is a good idea. Having BrowserToolbar reach outside of
> its layout and referencing an ID it doesn't own unnecessarily increases
> coupling. BrowserApp will still need to hold a reference to the progress bar
> because of the translation, so removing the setter doesn't buy us much
> anyway.

Oh, yes, somehow missed the fact that BrowserApp needs a reference to it. But if that wasn't the case, I wouldn't worry too much about your point about coupling because the owner of the progress bar is effectively BrowserToolbar.  

> Note that this is essentially the same design we use for HomeBanner, where
> we also have a setter:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.
> java#1661.

Yeah, and I'm not a fan of this pattern because it doesn't guarantee the caller will do the right thing.

Comment 49

[Brian Nicholson (:bnicholson)]

Attachment: Fix progress bar visibility on pre-Honeycomb devices

Inevitably, I found more issues trying this on a GB found: the progress bar never disappears once it reaches the end. Adding clearAnimation to setVisibility makes it disappear, but it reappears when setAnimation is called (triggered by AnimatorProxyPreHC). So in addition to clearing the animation when hiding, we also need to prevent future calls to setAnimation if the view isn't shown.

Comment 50

[Lucas Rocha (:lucasr)]

Comment on attachment 8392509 [details] [diff] [review]
Fix progress bar visibility on pre-Honeycomb devices

Review of attachment 8392509 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/toolbar/ToolbarProgressView.java
@@ +102,5 @@
> +        // On GB/Froyo, setting the visibility to GONE/HIDDEN alone does not
> +        // work with translations. Calling clearAnimation acts as a workaround.
> +        if (PRE_HONEYCOMB && visibility != VISIBLE) {
> +            clearAnimation();
> +        }

I have a slight preference for clearing the animation *before* calling super.setVisibility().

Comment 51

[Brian Nicholson (:bnicholson)]

Here we go again:

https://hg.mozilla.org/integration/fx-team/rev/80a937654878
https://hg.mozilla.org/integration/fx-team/rev/3d168edebd57

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80a937654878
https://hg.mozilla.org/mozilla-central/rev/3d168edebd57

Comment 53

[Brian Nicholson (:bnicholson)]

Comment on attachment 8392314 [details] [diff] [review]
Make progress bar overlap content, v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: Progress bar is less attractive
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): Fairly low; past versions have caused regressions and were backed out, but there haven't been any reports of this breaking anything.
String or IDL/UUID changes made by this patch: None

Comment 54

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8392314 [details] [diff] [review]
Make progress bar overlap content, v4

Approving because we are very early in the beta cycle. FYI, for new features, we prefer to see them land in aurora.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2bdbf9223e1d
https://hg.mozilla.org/releases/mozilla-aurora/rev/c26670dd4bd6

https://hg.mozilla.org/releases/mozilla-beta/rev/6745acf573bd
https://hg.mozilla.org/releases/mozilla-beta/rev/4b6c759a5cad

Comment 56

[Kevin Brosnan [Ex-Mozilla]]

To the best of my knowledge this code has not been tested on varying devices, form factors and Android versions. The previous iteration of this code needed to be disabled before 29 went to beta. I am not happy that this code is on beta without those checks. Our nightly audience trends towards very new devices.

Comment 57

[Sylvestre Ledru [:Sylvestre]]

Kevin, sorry about that.
Do you think we should revert this change?

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Not sure what that was directed at me. I'm not the one who approved it.

Comment 59

[Kevin Brosnan [Ex-Mozilla]]

If we do it means we need to spin another beta candidate. :-( I guess we'll "just do it live". Though there is a chance of needing a backout/respin cycle.

Comment 60

[Sylvestre Ledru [:Sylvestre]]

OK. Thanks. Adding the tracking then.

Comment 61

[Kevin Brosnan [Ex-Mozilla]]

Going to call this verified with https://wiki.mozilla.org/QA/Fennec/29/Beta/2/progressbar-results and my testing on aurora & nightly.

Comment 62

[Kevin Brosnan [Ex-Mozilla]]

Bah this did not make it into beta 2.