822044 - Improve about:telemetry visual style on mobile

Status: RESOLVED INCOMPLETE

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

Description

[Théo Chevalier]

Attachment: about:telemetry screenshot on mobile

about:telemetry page is great on desktop, but as you can see on the screenshot, not really on Firefox for Android.

First, the button needs to be fixed.

I think we also need padding from left & right, and to fix the font size. One good example is about:support which is ok to read on Desktop and mobile.

This could be fixed, I guess, by moving toolkit/content/aboutTelemetry.css to toolkit/theme/winstripe/global/aboutTelemetry.css

then we could create and fix style for mobile in mobile/android/theme/core/aboutTelemetry.css

To fix the button, we can do like for buttons in about:support (width:100%) then center the "Telemetry is enabled/disabled" description right above the button.

What do you think?

Comment 1

[Théo Chevalier]

Attachment: about:telemetry, patch applied

Here is a proposal, I am open to suggestion.

Comment 2

[Théo Chevalier]

Attachment: Patch V1

Comment 3

[Théo Chevalier]

Attachment: Patch V1.1

Rebased, and forgot to edit some jar.mn files.

Comment 4

[Vladan Djeric (:vladan)]

Comment on attachment 702507 [details] [diff] [review]
Patch V1.1

Thank you for doing this :) 

The patch looks good to me, but you probably want to get this reviewed by someone with more experience with CSS and UX design.

Did you test the new skin in RTL mode? Also, do you have a try build handy that I could try out? I'm curious to see what some of the sections look like when they're not empty.

Comment 5

[Théo Chevalier]

(In reply to Vladan Djeric (:vladan) from comment #4)
> Comment on attachment 702507 [details] [diff] [review]
> Patch V1.1
> 
> Thank you for doing this :) 
> 
> The patch looks good to me, but you probably want to get this reviewed by
> someone with more experience with CSS and UX design.
> 
> Did you test the new skin in RTL mode? Also, do you have a try build handy
> that I could try out? I'm curious to see what some of the sections look like
> when they're not empty.
Here is a try build https://tbpl.mozilla.org/?tree=Try&rev=19080855537b

I’m currently running a build with rtl enable on my own machine. I assume turning en-US in rtl mode is enough for testing?

Comment 6

[Théo Chevalier]

Attachment: about:telemetry v2

As mfinkle told me, I looked at CSS of other pages, and I played a little bit.

Comment 7

[Théo Chevalier]

Attachment: Little glitch

But it remains a little glitch, when an element needs more space, I can't extend the background. (That's a float element) I don't know if we really have to fix it or not.

Comment 8

[Théo Chevalier]

Attachment: Patch v2

Here are two builds if you want to play with. RTL looks good to me, but I couldn't try all section.

http://theochevalier.fr/files/fennec-rtl.apk
http://theochevalier.fr/files/fennec-ltr.apk

Note that on the rtl build, I forgot to enable RTL in the js for histograms, but I checked it's working.

I allowed zooming, I don't know if it's a good idea or not.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 704314 [details] [diff] [review]
Patch v2

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

This looks pretty good overall, though I'd suggest the following tweaks:

- Set the background color for the histograms to white to separate them from the page
- Add a small margin between each histogram so their borders aren't touching
- Perhaps you could take advantage of the CSS3 word-wrap property with "word-break" to handle those long labels? http://www.w3schools.com/cssref/css3_pr_word-wrap.asp
- 100% width elements look fine on small width devices (like phones), but they're awkward on larger screens (like tablets). Maybe you could try setting the max-width property on these elements to confine them to a reasonable width?

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> - Perhaps you could take advantage of the CSS3 word-wrap property with
> "word-break" to handle those long labels?

Sorry, I meant "break-word".

Comment 12

[Théo Chevalier]

Attachment: Screenshots -V2.1

Here is the fixed version. I limited telemetry description and button to 700 px wide at maximum. I assume histogram and other data are fine at 100% wide even on large screen. Or does they need a limit like 1500 px too?
Addressed your comments (background, break-word, margins), thanks for your feedback.
Also updated design to match last UI changes in Firefox (no more textures)
Need to fix a last thing for System Information section and I attach the patch.

Comment 13

[Théo Chevalier]

Attachment: Patch V2.1

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 725807 [details] [diff] [review]
Patch V2.1

I don't have my tablet with me to try this out, but it looks good from the screenshots :)

One thing I noticed -- you deleted aboutTelemetry.css from toolkit/content and recreated it in themes/windows/global:

diff --git a/toolkit/content/aboutTelemetry.css b/toolkit/content/aboutTelemetry.css
deleted file mode 100644

diff --git a/toolkit/themes/windows/global/aboutTelemetry.css b/toolkit/themes/windows/global/aboutTelemetry.css
new file mode 100644

It's better if you simply move the file using "hg mv"; it keeps the revision history around for the file, and it also reduces the patch size.

Comment 15

[Wesley Johnston (:wesj)]

Changes to the toolkit stuff will require review from a toolkit peer. You should look at who originally wrote/owns this stuff.

Comment 16

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #15)
> Changes to the toolkit stuff will require review from a toolkit peer. You
> should look at who originally wrote/owns this stuff.

I had assumed that was vladen (comment 4)?

Comment 17

[Vladan Djeric (:vladan)]

I wrote the original page, ttaubert reviewed it in bug 661881. I can review the changes but I'm not a toolkit peer, so you might want to grab someone from that team.

Btw, as is, the "Browser Hangs" section will always empty. You might want to talk to Jim Chen about integrating his hang detector for mobile with this.

Comment 18

[Théo Chevalier]

(In reply to Brian Nicholson (:bnicholson) from comment #14)
> It's better if you simply move the file using "hg mv"; it keeps the revision
> history around for the file, and it also reduces the patch size.

Sorry, didn't know. Done :)

(In reply to Vladan Djeric (:vladan) from comment #17)
> I wrote the original page, ttaubert reviewed it in bug 661881. I can review
> the changes but I'm not a toolkit peer, so you might want to grab someone
> from that team.

I will ask Nathan Froyd, seems he is a toolkit peer (or ttaubert).

> 
> Btw, as is, the "Browser Hangs" section will always empty. You might want to
> talk to Jim Chen about integrating his hang detector for mobile with this.

Talking about what? See if the style of this section may be broken with my patch?


Also, I did a desktop build and just noticed my patch increased the font size, so I'm fixing it. (Yep, it was already big enough :) )

Comment 19

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #16)
> (In reply to Wesley Johnston (:wesj) from comment #15)
> > Changes to the toolkit stuff will require review from a toolkit peer. You
> > should look at who originally wrote/owns this stuff.
> 
> I had assumed that was vladen (comment 4)?

Ahh. Just saw that no one else's name was on the review queue, and I know we've had problems before when we've not looped the right people in. Looks like it was good we checked anyway!

Comment 20

[Vladan Djeric (:vladan)]

(In reply to Théo Chevalier [:tchevalier] from comment #18)
> > Btw, as is, the "Browser Hangs" section will always empty. You might want to
> > talk to Jim Chen about integrating his hang detector for mobile with this.
> 
> Talking about what? See if the style of this section may be broken with my
> patch?

No, your patch hasn't broken anything afaict, but the current "browser hangs" section simply isn't hooked up to the Android hangs data. You can talk to Jim about hooking up the Android hangs feature he is implementing with this page.

Comment 21

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.